search

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

version 1.0: new design, code factoring and improvements #185

Closed VincentDary closed 1 week ago

VincentDary commented 9 months ago

This pull request proposes a new version (1.0) of the cmsis-svd Python package with the following improvements. Note that this new version introduces a new cmsis-svd Python model which breaks the compatibility with the previous versions of this package. The parser interface remains the same.

Any feedback and code review is welcome and will be discussed.

@ckudera, maybe this new version can help you in your future research. This new version brings some improvements as support for nested cluster, type hint and, dim list and sub items naming review.

UPDATE 2024-01-27:

UPDATE 2024-09-30:

UPDATE 2024-10-03:

UPDATE 2024-10-06:

UPDATE 2024-10-12:

UPDATE 2024-10-13:

UPDATE 2024-10-14:

VincentDary commented 9 months ago

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

ccalmels commented 8 months ago

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.

gschwaer commented 8 months ago

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.

ckudera commented 7 months ago

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.

VincentDary commented 7 months ago

@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.

ckudera commented 6 months ago

@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.

jvogl-dev commented 6 months ago

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.

ckudera commented 5 months ago

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, ...).

AJ528 commented 5 months ago

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. ¯\(ツ)\/¯

brainstorm commented 1 month ago

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:
VincentDary commented 3 weeks ago

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:

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
VincentDary commented 3 weeks ago

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 !

VincentDary commented 3 weeks ago

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.

ckudera commented 3 weeks ago

@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?

VincentDary commented 3 weeks ago

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 ?

VincentDary commented 3 weeks ago

@BenBE py.typed is for Packaging Type Information, Thank you for your review, I forgot to fill the package_data with the package name and py.typed filename.

ckudera commented 3 weeks ago

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 ?

Thank you @VincentDary for the thoughtful and fast reply.

You are right, including SVDSuite as submodule would create unnecessary maintenance overhead. Better to directly use the package.

Concerning SVDSuite, process.py is undergoing significant changes in the next three weeks. You're welcome to provide feedback or create issues at any time, but I think it might be more efficient to review the code after these changes are complete, to save you from spending time on parts that are still in flux. That said, if you have any feedback on the processing approach itself, I'd be happy to hear it before I start implementing the major changes. I'm flexible though, so feel free to share feedback whenever suits you best.

Should we create a separate issue to discuss the next steps regarding the SVDSuite integration into cmsis-svd? I feel this pull request becomes hard to follow (sorry, my fault).

posborne commented 3 weeks ago

Hey @VincentDary, sorry for the lack of response over the past few months. I was away for about 6 months doing a thru-hike of the Pacific Crest Trail. I'll try to help move things along for a bit here but agree that it would be great to the project to get more maintainers on board as my interest in the project has waned over the years.

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.

I'll follow up again on pypi. When I created the Github org, I also put in a request with PyPI for an organization account but never received any communication back on the request submitted there, so I may need to pursue other avenues for sharing access to the project.

image

posborne commented 3 weeks ago

For publishing, I did now add the Github OIDC configuration as follows. @VincentDary if you have an account on pypi or can set one up, I will also add you as a maintainer for the project there.

image

VincentDary commented 3 weeks ago

@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.

VincentDary commented 3 weeks ago

@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.

ckudera commented 3 weeks ago

@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 🙂

VincentDary commented 3 weeks ago

Yes of course, thank you.

VincentDary commented 2 weeks ago

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.

ckudera commented 2 weeks ago 
* 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.

brainstorm commented 2 weeks ago

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!

BenBE commented 2 weeks ago

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 … ;-)

VincentDary commented 2 weeks ago

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.

VincentDary commented 2 weeks ago

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

BenBE commented 2 weeks ago

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(
VincentDary commented 1 week ago

@BenBE thank you for this great patch :)