@posborne, do you have any free time for this review ?

FYI I give your branch a try on my personal project : https://github.com/ccalmels/gdb-dashboard-svd/tree/cmsis_svd_v1.0 and everything seems ok with a small adaptation patch.

Hi, I found an incompatibility with the CMSIS-SVD schema. The schema states for enumeratedValues in fieldType:

<xs:complexType name="fieldType">
...
      <xs:element name="enumeratedValues" type="enumerationType" minOccurs="0" maxOccurs="2">
...
  </xs:complexType>

That is: enumeratedValues can be omitted, occur once, or twice! There are some SVDs that provide enumeratedValues twice, e.g., Texas Instruments MSP432P401Y.svd. E.g., in

peripheral = DMA
register = DMA_USEBURSTSET
field = SET

it has:

                    <field>
                        <name>SET</name>
                        <description>Returns the useburst status, or disables dma_sreq from generating DMA requests.</description>
                        <bitOffset>0x0</bitOffset>
                        <bitWidth>0x20</bitWidth>
                        <access>read-write</access>
                        <enumeratedValues>
                            <name>SET_enum_read</name>
                            <usage>read</usage>
                            <enumeratedValue>
                                <name>SET_0_READ</name>
                                <description>DMA channel C responds to requests that it receives on dma_req[C] or dma_sreq[C].

The controller performs 2R, or single, bus transfers.</description>
                                <value>0</value>
                            </enumeratedValue>
                            <enumeratedValue>
                                <name>SET_1_READ</name>
                                <description>DMA channel C does not respond to requests that it receives on dma_sreq[C].

The controller only responds to dma_req[C] requests and performs 2R transfers.</description>
                                <value>1</value>
                            </enumeratedValue>
                        </enumeratedValues>
                        <enumeratedValues>
                            <name>SET_enum_write</name>
                            <usage>write</usage>
                            <enumeratedValue>
                                <name>SET_0_WRITE</name>
                                <description>No effect. Use the DMA_USEBURST_CLR Register to set bit [C] to 0.</description>
                                <value>0</value>
                            </enumeratedValue>
                            <enumeratedValue>
                                <name>SET_1_WRITE</name>
                                <description>Disables dma_sreq[C] from generating DMA requests.

The controller performs 2R transfers.

Writing to a bit where a DMA channel is not implemented has no effect.</description>
                                <value>1</value>
                            </enumeratedValue>
                        </enumeratedValues>
                    </field>

Currently the second enumeratedValues is silently ignored.

I tried to fix it and it probably worked, but I don't know this code base too well, so here's what I tried:

diff --git a/python/cmsis_svd/model.py b/python/cmsis_svd/model.py
index 4c9fdb4..383812c 100644
--- a/python/cmsis_svd/model.py
+++ b/python/cmsis_svd/model.py
@@ -254,7 +254,7 @@ class SVDField(SVDElement, SVDDimElementGroup):
     modified_write_values: Optional[SVDModifiedWriteValuesType] = field(default=None)
     write_constraint: Optional[SVDWriteConstraint] = field(default=None)
     read_action: Optional[SVDReadActionType] = field(default=None)
-    enumerated_values: Optional[SVDEnumeratedValues] = field(default=None)
+    enumerated_values: Optional[List[SVDEnumeratedValues]] = field(default=None)
     derived_from: Optional[str] = field(default=None, metadata={'type': 'attribute'})
 
     def __post_init__(self) -> None:
@@ -265,7 +265,8 @@ class SVDField(SVDElement, SVDDimElementGroup):
             self.write_constraint.parent = self
 
         if self.enumerated_values:
-            self.enumerated_values.parent = self
+            for enum_values in self.enumerated_values:
+                enum_values.parent = self
 
     @property
     def is_enumerated_type(self) -> bool:
diff --git a/python/cmsis_svd/parser.py b/python/cmsis_svd/parser.py
index 6df8720..3ed2ba9 100644
--- a/python/cmsis_svd/parser.py
+++ b/python/cmsis_svd/parser.py
@@ -482,8 +482,10 @@ class SVDParser:
         dim_element_group = self._parse_dim_element_group(field_node)
 
         enum_values = None
-        if (enum_values_node := field_node.find('./enumeratedValues')) is not None:
-            enum_values = self._parse_enumerated_values(enum_values_node)
+        if (enum_values_nodes := field_node.findall('./enumeratedValues')) is not None:
+            enum_values = []
+            for enum_values_node in enum_values_nodes:
+                enum_values.append(self._parse_enumerated_values(enum_values_node))
 
         mod_w_value = _get_text(field_node, 'modifiedWriteValues')
         mod_w_value = (SVDModifiedWriteValuesType(mod_w_value) if mod_w_value

Btw, this is also broken on main: It just mashes all enumerated_value items from both enumerated_values into one, which results in name clashes.

Just thinking loud, but would it be possible to add some test that parses all .xmls in cmsis-svd-data and writes them out as xml again, then checks if they are equal? I think that would be a nice proof that the version 1.0 code does everything right.

First of all, thank you @VincentDary for your efforts and mentioning me in your initial post. Sorry for my late reply.

My project ARMify was funded, so I can work some time on a SVD parser during my work :) I would love to contribute to cmsis-svd 1.0 and in my opinion it doesn't make any sense to create another/new repository for "my" parser. However, I would suggest a new implementation for the following reasons:

derivedFrom attribute and dim arrays

I still like the pre-processing approach (#172), but I'm afraid I detected an issue with it.

Please checkout Example.zip, which contains an example for a svd file. In this example it's necessary to process the dim arrays first, since multiple derivedFrom attributes depend on array elements. Additionally, some derivedFrom attributes depend on others, so the parser requires some dependency resolving algorithm.

svdconv processes the svd file without any problems: svdconv --generate header --fields enum Github_Example.svd.

Separation of Parsing and Logic Processing

With regard to the above issue, I would suggest a two step approach: the fist step involves the parsing of a SVD file to Python dataclass models. In a second step, we implement the additional logic, e.g. (nested) clusters, dim elements (dimElementGroup attribute), and the inheritance functionality (derivedFrom attribute).

Seperation would also have the advantage that users can access the parsed data classes.

Additional Testing

As @gschwaer suggested, we should implement additional testing against existing SVD files and maybe also against existing parser implementations (e.g. GitHub - rust-embedded/svd: A CMSIS-SVD file parser)

@VincentDary, @posborne and everybody else who is interested in the development, please let me know what you think about a re-implementation and how we should handle it.

@ccalmels, @gschwaer, @ckudera, Thank you very much for your review. Your support and feedback are really welcome.

@gschwaer, I'm going to prepare a patch for the model and parser, to correct the issue pointed out by your comment about fieldType.enumeratedValues. This seems trivial.

@ckudera, I'm going to take a close look at this new design proposal.

About testing. @ckudera, I think that testing this library with another library is not a good hint for the following reasons: cmsis-svd data structure may differ and the diffing may require some adaptaters; the diffing can log a large amount of false positive, which can be complicated to handle to scale the tests (cmsis-svd-data). @gschwaer, your approach seems more elegant, and that seems enough for me, to check in practice.

@ckudera I'm really happy for the funding of your project, the integration of this new version of cmsis-svd in a funded firmware reverse engineering tool would be great for this library: visibility, attracting new users, feedback, and maybe contributions. You have my full support as possible, since I work on related topics.

See you soon.

@VincentDary Just to let you know, I'm almost done with a parser (svd to dataclasses) and xml serializer (dataclasses back to svd), without any logic processing. I will be on vacation the next 1 1/2 weeks and plan to finish and release the parser until the end of April.

If I understand correctly SVDPeripheral.registers is no longer a getter but allows direct access to the registers member, which can be anything from a single SVDRegister to a SVDRegisterArray or SVDRegisterCluster(Array), which is great as I need to access SVDRegisterArray without it being expanded into a list of SVDRegister.

On the other hand SVDPeripheral.get_registers() returns a list of (expanded) SVDRegister, which might be confusing. I suggest to consider renaming SVDPeripheral.get_registers() to SVDPeripheral.get_expanded_registers() or similar.

I know this is not the best place to discuss the development of my parser, but not sure if a new issue would be better. Anyway, I just published the first version of SVDSuite. Currently, it can parse (to dataclasses, no logic processing), manipulate, validate, and generate SVD files. In the next weeks I will work on processing (derivedFrom, clusters, ...).

As someone who is hoping to use this parser in an upcoming project, I vote for the merge to be approved at this point. I think anyone who wants to review the code has had plenty of time, and when I start my project I'd like to start on version 1.0.
It seems like a lot of improvements were made, and I don't see any reason to not move forward with it. Worst case: a couple bugs are introduced, people submit issues, and they get squashed. I don't see any downsides. ¯\_(ツ)_/¯

I vote for the merge to be approved at this point. I think anyone who wants to review the code has had plenty of time, (...)

Well, I just tested it locally and seems to have a syntax error? Has this PR been tested thoroughly?:

(...)
cmsis_svd/parser.py:521: SyntaxWarning: invalid escape sequence '\['
  m = re.search('\[([0-9]+):([0-9]+)\]', field.bit_range)

Easy fix, btw:

diff --git a/python/cmsis_svd/parser.py b/python/cmsis_svd/parser.py
index 6df8720..e52f889 100644
--- a/python/cmsis_svd/parser.py
+++ b/python/cmsis_svd/parser.py
@@ -518,7 +518,7 @@ class SVDParser:
         )

         if field.bit_range is not None:
-            m = re.search('\[([0-9]+):([0-9]+)\]', field.bit_range)
+            m = re.search('\\[([0-9]+):([0-9]+)\\]', field.bit_range)
             field.bit_offset = int(m.group(2))
             field.bit_width = 1 + (int(m.group(1)) - int(m.group(2)))
         elif field.msb is not None:

Hello @brainstorm,

Thank you very much for your contribution, this is very welcome, I have integrated your change in this pull request, and add reference to your name in the AUTHOR file.

About your answer, this is not perfect however this PR has been tested deeply through a CI with Python 3.8, 3.9, 3.10, 3.11 and the cmsis-svd-data repository which contains a large agregation of cmsis-svd files. Take a look at the following links:

• repository CI results: https://github.com/cmsis-svd/cmsis-svd/pull/185/checks
• CI workflow: https://github.com/VincentDary/cmsis-svd/blob/version-1.0/.github/workflows/test.yml
• Python tests: https://github.com/VincentDary/cmsis-svd/blob/version-1.0/python/cmsis_svd/tests/test_parser.py
• cmsis-svd-data/data: https://github.com/cmsis-svd/cmsis-svd-data/tree/main/data

About the bug you found, this is not an error, this is a warning, the regular expression is bugged but It works. The warning message caused by this bug has not been identified because the warning message is displayed since Python 3.12, which is a recent Python version not currently tested in the CI. In addition, this code works and does not trigger any Python exception from Python 3.8 to Python 3.12. See it in practice:

/tmp/bug.py

import re
import platform
m = re.search('\[([0-9]+):([0-9]+)\]', '[3:6]')
print(f'parsed: bit {m.group(1)} to {m.group(2)}')
print(f'Python version: {platform.python_version()}')

In Python 3.11:

$ python  /tmp/bug.py
parsed: bit 3 to 6
Python version: 3.11.8

In Python 3.12, only a warning message, no Python exception:

$ python /tmp/bug.py
/tmp/bug.py:3: SyntaxWarning: invalid escape sequence '\['
  m = re.search('\[([0-9]+):([0-9]+)\]', '[3:6]')
parsed: bit 3 to 6
Python version: 3.12.6

derivedFrom attribute and dim arrays

I still like the pre-processing approach (#172), but I'm afraid I detected an issue with it.

Please checkout Example.zip, which contains an example for a svd file. In this example it's necessary to process the dim arrays first, since multiple derivedFrom attributes depend on array elements. Additionally, some derivedFrom attributes depend on others, so the parser requires some dependency resolving algorithm.

svdconv processes the svd file without any problems: svdconv --generate header --fields enum Github_Example.svd.

Hello @ckudera,

I have reviewed your SVD test case fixtures on ARMify-Project/tests/svd). These use cases look really interesting.

But do you have any real world SVD file which use the SVD spec in this way ? I'm really curious about this.

I'm going to review this topic in depth.

If you are agree to open a pull request on cmsis-svd repository with your code, I am agree to do a full code review and merge it on the main branch ASAP, if everything is ok ! I think it's better to keep this repository than fork, this way you benefit from better feedback and visibility, and the cmsis-svd users a better code quality !

Hi, I found an incompatibility with the CMSIS-SVD schema. The schema states for enumeratedValues in fieldType:

<xs:complexType name="fieldType">
...
      <xs:element name="enumeratedValues" type="enumerationType" minOccurs="0" maxOccurs="2">
...
  </xs:complexType>

That is: enumeratedValues can be omitted, occur once, or twice! There are some SVDs that provide enumeratedValues twice, e.g., Texas Instruments MSP432P401Y.svd. E.g., in

peripheral = DMA
register = DMA_USEBURSTSET
field = SET
...
Currently the second `enumeratedValues` is silently ignored.
...
I tried to fix it and it probably worked, but I don't know this code base too well, so here's what I tried:

Hello @gschwaer

Thank you very much for your contribution, this is very welcome, I have integrated your changes (parser, model) with a test for MSP432P401Y DMA.DMA_USEBURSTSET.SET register field, and add reference to your name in the AUTHOR file.

@VincentDary

I have reviewed your SVD test case fixtures on ARMify-Project/tests/svd). These use cases look really interesting.

But do you have any real world SVD file which use the SVD spec in this way ? I'm really curious about this.

In my opinion, an SVD parser should at least support the functionality of svdconv to ensure the best user experience. As far as I know, the SVD files in CMSIS Packs are tested against svdconv. Other tools, however, implement additional features, such as forward references across different peripherals, whereas svdconv only supports forward referencing within the same scope. Unfortunately, the SVD specification is not always clear about these features, and sometimes the expected implementation is insufficiently documented.

In short, I cannot currently provide you with any "official" SVD files from vendors that use features like those found in my more complex test scenarios, as I have not yet searched through the official vendor SVD files for them. Nonetheless, I am planning to conduct an analysis of the SVD standard, where, among other things, I will examine which features are actually being utilized by manufacturers.

If you are agree to open a pull request on cmsis-svd repository with your code, I am agree to do a full code review and merge it on the main branch ASAP, if everything is ok ! I think it's better to keep this repository than fork, this way you benefit from better feedback and visibility, and the cmsis-svd users a better code quality !

Thank you very much for the offer!

SVDSuite is not quite finished yet. I've identified a few test cases that are currently not being handled correctly. My goal is to finalize these tasks within the next 2-3 weeks. Additionally, I'm working on setting up a dedicated repository that will provide a comprehensive collection of test cases, along with explanations, to support future SVD parser developments, potentially in other programming languages.

I still believe it doesn't make sense to have two Python packages for SVD parsing. SVDSuite has evolved beyond just parsing, and to be honest, I’ve invested so much energy over the summer diving deep into the SVD standard and developing SVDSuite that I’d prefer to keep the project under my care for the time being. However, I do think it would be beneficial to have additional maintainers for the project, so if you’re interested, I’d be happy to collaborate. One major issue I see is that you're not the maintainer of cmsis-svd on PyPI (that’s @posborne), which means no new versions can be pushed.

In my opinion, the most practical solution, also in the best interest of the users, would be to integrate SVDSuite as a submodule within cmsis-svd, wrapping it in such a way that no changes are required for current cmsis-svd users (if that's feasible). What do you think of this proposal?

Hello @ckudera,

Thank's for your quick answer.

After a little review of the issues about derived from [1] [2] [3], I am agree with your feedbacks and proposals. The pre-processing step I have implemented in #172 must be avoid, and a multi stages processing post parsing must be implemented, at least to support derived from another derived item or dim-ed or mixed section. Also, forward references is interesting, since tag order declaration has no semantic significance in XML (2), but it must clearly stated about what is allowed.

About your plan to continue on SVDSuite, I understand your point of view. About the Pypi package, there are way to build and push the package to Pypi with OIDC or API token which will require a minimal effort from @posborne. I don't think there is any benefit to use SVDSuite as submodule in this package, since it requires maintenance for nothing, in this case better to use your package directly. So, I continue the development of cmsis-svd.

I have reviewed your code this night, there are very interesting things, however there are technical choices and implementation details I don't understand very well for the moment. Do you accept code review or pull request on SVDSuite, or since it is under devellopment you prefer to keep it under your full control ?

@posborne,

Congratulations for your hike ! :)

Yes I have an account on Pypi, this is https://pypi.org/user/VincentDary/ , same pattern as my github account, thank you for adding me to the maintainers of the package on Pypi.

And thank's for the OIDC.

@ckudera

Since the status of the cmsis-project is unlocked and your are part of the organisation, I don't know if you want to follow SVDSuite, merge the code of SVDSuite in CMSIS-SVD, or work on the two project at the same time.

Maybe you need time to think about these choices. In any case you have my support.

About my SVDSuite code review, my principal feedback is the following. I understand the complexity of the cases to handle, but in my opinion, for the moment the code of SVDSuite is too complexe to handle derivedFrom attributes, but maybe you need requirement/features I don't know, also as you said your code has probably change this last week. At the oposite my pull request #172 is too naïve and totaly holed to handle your specific cases.

Since yesterday I am working on a POC top of the code base of this pull request to include all your feedbacks via a post processing step. I should be able to test if it works well this week. Before, I need to test some stuff with SVDConv to list the case to handle or not, there are things not clear in the spec about full qualified path, for example derive from nested cluster: PeripheralX.ClusterD1.ClusterD2.ClusterD3, why not ! or more ambiguous path with registers, fields and enumerated values.

@VincentDary

Just a short answer, it's getting late.

We literally are working on the same stuff. I added you to a svdconv fork, where I added some quick & dirty print functionality to make things a little bit easier. Hope it helps 🙂

Yes of course, thank you.

Hello,

I have update this pull request to include some reviews and new features. In particular, SVD file validation with SVD XML Schema Definition, and a SVD XML serializer (A refactored, reviewed and modified version of the SVD XML serializer from SVDSuite).

Since this pull request is open for almost one year and have bring a lot of improvements (too much for just one PR), I proposed to integrate all your feedbacks ASAP about this last commit, and get approval of a maintainer in order to merge the code on the main branch.

@ckudera, @BenBE, @brainstorm, any help or feedbacks is welcome, Thanks in advance.

* In `python/cmsis_svd/parser.py` around line 300ff, the filename should be restricted/sanitized somewhat for `SVDParser::for_packaged_svd`, as the code right now allows for path-traversal issues (cf. CWE-22 and related).

@BenBE nice catch!

@VincentDary Be aware that os.path.join has the following functionality: "If a segment is an absolute path (which on Windows requires both a drive and a root), then all previous segments are ignored and joining continues from the absolute path segment" (source). Therefore, checking if the string contains .. is not enough. os.path.join(package_root, vendor, os.path.basename(filename)) should do the trick.

There's an argument to be had on dropping the usage of os.path over pathlib, but I'm not sure it's a good idea to prolong the review and merging of this PR further?:

https://betterprogramming.pub/should-you-be-using-pathlib-6f3a0fddec7e

I've recently come across unpleasant gotchas with "goold old" os.path that are not an issue with pathlib myself recently (related to PATHLIB/venvs too), but I'd rather get this merged sooner rather than later, I think it's good enough as-is, kudos!

As written in my TL;DR I think it's fine for merging; mostly delayed the full approval to get some feedback on the outstanding findings.

I think, what still should be included (as these items are easy to handle) are the first three of my list (the AUTHORS file missing one member from the team AFAICS, and the (maintainer) remark on my line; the cmsis_svd_specs vs specs subdir suggestion; the two additional SVD spec revisions 1.3.4 and 1.3.8). Also the two minor edits for the README should be easy. You could argue regarding the sub-package for the serializer classes, because those affect the API, but as long as we just merge the PR but hold off with the actual 1.0 release, we can still do this until we actually release 1.0.

Everything else from my review is cosmetic and can be done later.

On the subject of os.path vs pathlib: Let's handle this in a separate PR. That issue has been there even with the old code, thus no need to hurry. We should fix it for the actual release though … ;-)

Thank you @ckudera, @BenBE and @brainstorm for your quick review.

@ckudera, About unit test, I know, there are very slow, but since all svd file are parsed and now serialized, it's even slower, I don't have any workaround, but this new code base move parsing/serializing of all files in test_parser2serializers.py which can be skipped if you specify test to nose2. About the PEP585, I think this is a very good hint, but since this not completely supported by all Python versions and Mypy, I keep this topic for future factoring and Python version support. Also, thank you very much for your quick proposal to fix the path traversal vulnerability.

@brainstorm, as @BenBE said, I left os.path vs pathlib for a separated PR, thanks for the hint.

@BenBE, Thank you very much for your feedbacks, particularly about schema version handling, and for the path traversal vulnerability.

This PR will be merged 2024/10/14 at 12h00 (Paris, France) if no additional comments.

Minor bug in _get_latest_schema_version:
The routine didn't actually raise the exception.
(Found while checking if the version compare worked properly).

Also did some streamlining of that function while at it (second part of the patch).

diff --git a/python/cmsis_svd/parser.py b/python/cmsis_svd/parser.py
index 9644a9b7..da259c4b 100644
--- a/python/cmsis_svd/parser.py
+++ b/python/cmsis_svd/parser.py
@@ -255,23 +255,15 @@ class SVDParser:
     @staticmethod
     def _get_latest_schema_version() -> str:
         if len(SVD_SCHEMA_VERSIONS) == 0:
-            SVDParserValidationError('SVD schema versions not found.')
+            raise SVDParserValidationError('SVD schema versions not found.')
 
-        versions = []
-        for ver in SVD_SCHEMA_VERSIONS:
-            split_ver = [int(part) for part in ver.split('.')]
-            for idx in range(len(split_ver), 3):
-                split_ver.append(0)
-            versions.append(split_ver)
-
-        latest = versions[0]
-        for item_version in versions:
-            for i in range(3):
-                if item_version[i] > latest[i]:
-                    latest = item_version
-                    break
-
-        return '.'.join([str(n) for n in latest])
+        def parse_version(version):
+            return list(map(int, version.split('.')))
+
+        versions = list(map(parse_version, SVD_SCHEMA_VERSIONS))
+        latest = max(versions)
+
+        return '.'.join(map(str, latest))
 
     @classmethod
     def validate_xml_tree(

@BenBE thank you for this great patch :)