Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 7, 2023
Merged

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

merged 1 commit into from
Oct 7, 2023

Conversation

VincentDary
Copy link
Member

@VincentDary VincentDary commented May 27, 2023

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
Copy link

ckudera commented Jul 17, 2023

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
Copy link
Member Author

@ckudera Thank you very much for your review.

Copy link
Collaborator

@posborne posborne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good to me. I think there's probably an approach we could employ that's more efficient than this if it were important but that isn't really a primary aim of this parser anyhow and I do like that this solves a few of the frequently encountered problems and is a less hacky way of making the tree easier to deal with on the front end.

@VincentDary
Copy link
Member Author

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

@posborne
Copy link
Collaborator

posborne commented Oct 7, 2023

@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
Copy link
Member Author

VincentDary commented Oct 7, 2023

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

@VincentDary VincentDary requested review from a team and removed request for a team October 7, 2023 08:58
@VincentDary
Copy link
Member Author

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

@VincentDary VincentDary merged commit 39681ec into cmsis-svd:main Oct 7, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants