feat: Adds CDC results parsing for Meridian in the CDC flow#237
feat: Adds CDC results parsing for Meridian in the CDC flow#237nsw-lowrisc wants to merge 1 commit into
Conversation
This commit adds parsing of the Meridian CDC reports. The parser is using the meridiancdc.py file in the tools directory, so the parser in the local repo is not required, however, if the report_cmd variable is set in the hjson this will be overridden and the parser called by make. NOTE: the buckets have changed, adding waived and tofix, so the corresponding common_cdc_cfg.hjson file should be updated in line. This required a fix to msg_buckets.py to avoid an exception. Signed-off-by: Neil Webb <[email protected]>
machshev
left a comment
machshev
left a comment
There was a problem hiding this comment.
Thanks @nsw-lowrisc!
Some nits but the general concept seems fine.
| """Class describing lint configuration object.""" | ||
| """Class describing CDC configuration object.""" | ||
|
|
||
| import logging |
There was a problem hiding this comment.
We have a configured logger available as from dvsim.logging import log. This does some extra setup to add new log levels. Could we use that please?
| """Parses Meridian CDC report and dumps filtered messages in hjson format.""" | ||
|
|
||
| import argparse | ||
| import logging |
| # For stand alone | ||
| # Check if this file is the main entry point | ||
| IS_STANDALONE = __name__ == "__main__" | ||
| if IS_STANDALONE: | ||
| # Get the absolute path of the directory one level up (the project root) | ||
| project_root = str(Path(__file__).resolve().parent.parent.parent) | ||
| # Add it to Python's search path if it isn't already there | ||
| if project_root not in sys.path: | ||
| sys.path.insert(0, project_root) |
There was a problem hiding this comment.
Why are we doing this? This looks really hacky... what are you trying to achieve?
There was a problem hiding this comment.
Thanks @machshev for the review.
I think it is best to resolve this point first, as others depend on it.
The other parsers in this directory (lint/RDC/...) are imported from the "in-tree" version of dvsim. The in-tree versions are still run stand alone from the make file. This, the CDC parser, was initial derived from the standalone RDC parser. This is the only one that can now be called either way, from dvsim or the make file. This where the IS_STANDALONE logic comes from and also other things like the logging (I added this in the standalone version, so should be corrected) and parsing the input parameters with argparse.
So, if it is not desirable to keep the possibility of running stand alone then all the extra stuff should be removed. I am OK with either, what ever fits best into the bigger picture.
| from dvsim.utils import subst_wildcards | ||
|
|
||
| # Get the global logger definition | ||
| log = logging.getLogger(__name__) |
There was a problem hiding this comment.
| log = logging.getLogger(__name__) | |
| from dvsim.logging import log |
|
|
||
|
|
||
| # Get the global logger definition | ||
| log = logging.getLogger(__name__) |
There was a problem hiding this comment.
| log = logging.getLogger(__name__) | |
| from dvsim.logging import log |
| category = "" | ||
| severity = "" | ||
| known_rule_names = {} |
There was a problem hiding this comment.
Maybe move these down the function closer to where they are used?
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def extract_rule_patterns(file_path: Path): |
There was a problem hiding this comment.
Could you add typing for the return type... it looks like List[Tuple[str,str]]?
With this added then pyright is a lot more useful.
|
|
||
|
|
||
| # Reuse the lint parser, but add more buckets. | ||
| class CdcParser(LintParser): |
| # Configure Logging log to console if running stand alone | ||
| # logging format layout | ||
| log_layout = "[%(levelname).1s %(asctime)s %(filename)s:%(lineno)d] %(message)s" | ||
| time_layout = "%y%m%d %H:%M:%S" | ||
| console_handler = logging.StreamHandler() | ||
| console_handler.setFormatter(logging.Formatter(f"{log_layout}", datefmt=time_layout)) | ||
| active_handlers = [console_handler] | ||
|
|
||
| logging.basicConfig(level=logging.INFO, handlers=active_handlers) |
There was a problem hiding this comment.
This should be covered if you use the dvsim logger?
| msg = f"Signatures in {k} must be a list of strings" | ||
| raise RuntimeError(msg) | ||
| self.buckets[k].signatures.extend(signatures) | ||
| # check the key is in the bucket list to avoid an exception |
There was a problem hiding this comment.
Do we want to log a warning if it's not? Is this an error case?
There was a problem hiding this comment.
This only to prevent, dvsim crashing out completely with an esoteric error when there a mismatch between the buckets in the hjson and python. The idea is only to smooth transition when adopting the new parser. I would say that long term the way the buckets are managed should be revised, that is only one definition needed.
I can add a warning massage.
2 participants
This commit adds parsing of the Meridian CDC reports. The parser is using the meridiancdc.py file in the tools directory, so the parser in the local repo is not required, however, if the report_cmd variable is set in the hjson this will be overridden and the parser called by make.
NOTE: the buckets have changed, adding waived and tofix, so the corresponding common_cdc_cfg.hjson file should be updated in line. This required a fix to msg_buckets.py to avoid an exception.