cmsis-svd / cmsis-svd

Aggegration of ARM Cortex-M (and other) CMSIS SVDs and related tools
Apache License 2.0
1.08k stars 270 forks source link

SVD XML tree pre-processing step for tag and properties inheritance #172

Closed VincentDary closed 1 year ago

VincentDary commented 1 year ago

I propose the following pull request to solve globally the tag inheritance issues see annexe (1) and other aspects of the current implementation.

The PR implements a SVD XML tree pre-processing step in order to propagate inherited tags (attribute derivedFrom) through the XML tree (see class SVDXmlPreprocessing._derived_from_*). The tag inheritance propagation follows the constraints and rules describe by the official CMSIS-SVD documentation, these rules are specific to each concerned tag (enumeratedValues, field, register, cluster, peripheral). More information on the following links in annexe (2).

Moreover, the proposed pre-processing step includes a sub step to propagate and override the register properties group to each sub level according the rules describe by the official CMSIS-SVD documentation (class SVDXmlPreprocessing._propagate_register_properties_group). More detail on the following link in annexe (3).

These two pre-processing steps make it nonessential the dynamic attribute resolution (SVDElement._lookup_possibly_derived_attribute) implemented in the model which has been removed. I argue that in this specific case pre-processing is better than dynamic attribute resolution for the following reasons: first, the propagation of the tag inheritance and register properties are formal and determined by rules hard coded in the implementation, after the pre-processing step of a given SVD XML file it is possible to know with certainty the tree and all its nodes, while dynamic resolution leaves this obscure and difficult to predict; secondly: with the actual dynamic attribute resolution it is not possible to deep copy an SVDElement which is a basic operation, this cause a "maximum recursion depth exceeded" error see annexe (4), which is inherent to recursive call to getattr; finally this implementation allows to simplify the code and make it very straightforward to understand and maintain.

To implement the SVD XML pre-processing the built-in python XML library import has been replaced by the lxml library to access to a robust Xpath implementation, useful to process complex XML schema like CMSIS-SVD.

All the tests are passed and tested in Conda environment with Python 3.7.16.

I'm interested in your review (@posborne, @Jegeva, @pengi, @pravic) and in taking your comments into account.

UPDATE (2023-06-23): Rebase PR commit to replace python F string to format() for more Python version compatibility, and other cosmetic stuff.

UPDATE (2023-10-07): Rebase PR commit from cmsis-svd/main.

Annexes:

1 Fixed Issues
2 CMSIS-SVD documentation derivedFrom
3 CMSIS-SVD documentation registerPropertiesGroup
4 SVDElement deepcopy error
>>> import copy
>>> from cmsis_svd.parser import SVDParser
>>> dev = SVDParser.for_packaged_svd('Toshiba', 'M367.svd').get_device()
>>> copy.deepcopy(dev.peripherals[6])
[...]
  File "/src/cmsis-svd/python/cmsis_svd/model.py", line 150, in get_derived_from
    if self.derived_from is None:
  File "/src/cmsis-svd/python/cmsis_svd/model.py", line 146, in __getattr__
    return self._lookup_possibly_derived_attribute(attr)
  File "/src/cmsis-svd/python/cmsis_svd/model.py", line 75, in _lookup_possibly_derived_attribute
    derived_from = self.get_derived_from()
RecursionError: maximum recursion depth exceeded
ckudera commented 1 year ago

I really like the pre-processing approach and in my opinion it's the way to go.

Out of curiosity I created a repository to validate the results of the existing parser against the results of the PR: https://github.com/ckudera/svd-parser-pre-processing-validation

The pre-processing solves multiple issues at once, does not affect the processing time in a noteworthy way, and, based on a random sample validation, works as expected.

@VincentDary Thank you for this amazing work :)

VincentDary commented 1 year ago

@ckudera Thank you very much for your review.

VincentDary commented 1 year ago

@posborne, thank's for your review. I'm open to a better design. what are you thinking about in particular ?

posborne commented 1 year ago

@VincentDary I don't think there's a need for a different design at this time. On the efficiency front, python is probably not the ideal choice anyhow and it would quickly need to be a use-case dependent thing. Any approach that does a mapping to a full in memory representation is unlikely to be the most efficient way to do things for many use cases where only a subset of the data is required. Since SVD tooling is typically just used on the frontend of projects for codegen or debug tooling, this isn't (too my knowledge) really a problem.

VincentDary commented 1 year ago

@posborne ok, thank's for this feedback. I will rebase now, test, push.

VincentDary commented 1 year ago

@posborne, the rebase is ok. I am merging now.