Open-CMSIS-Pack / Open-CMSIS-Pack-Spec

Common Microcontroller Software Interface Standard - Pack(age) based distribution system
https://open-cmsis-pack.github.io/Open-CMSIS-Pack-Spec/
Apache License 2.0
50 stars 19 forks source link

Fix: unordered DevicePropertiesGroup #270

Closed gauthiergodart closed 4 months ago

gauthiergodart commented 7 months ago

The DevicePropertiesGroup was designed in such a way that a strict order was required for the deprecated, debugconfig, processor and DefaultDevicePropertiesGroup elements (those were defined into a sequence). This requirement is not stated into the spec. So, to comply with the spec, I suggest removing it. I tested the solution against our PDSC, and it seems to work fine, but maybe this ordering requirement is important in some case. I leave that to your appreciation. But if it is, then it would be nice to be explicit about it into the documentation.

jkrech commented 6 months ago

@gauthiergodart, I have enabled running the checks. Running xmllint with the updated PACK.xsd schema against the published packs pdsc files fails. These failures prevent this PR from being accepted. Could you please check how this could be fixed?

gauthiergodart commented 6 months ago

@jkrech You are perfectly right. I've come up with a new alternative, which should solve the issue. Could you run the checks again? Please note that this fix comes with a trade-off: it removes the uniqueness constraint on deprecated and debugconfig elements. But it seems more acceptable to relax this constraint than to impose one on the order, which is not compatible with the specification. The uniqueness constraint was in any case not imposed by the schema on sequences elements, although they are also subject to this constraint in the specification. These uniqueness constraints could, instead, be verified by the parser.

jkrech commented 6 months ago

The Test looks good now from the validation of existing pdsc files. The remaining issues are about version updates in the different locations. Let me review the proposal. Something this test does not check is whether someone could provide multiple instances of the same where this is not expected. Did you test this?

gauthiergodart commented 6 months ago

Yes, actually, as stated in my commit message, this solution removes uniqueness constraints on some elements. I was just updating my previous comment to explain this choice when I saw your answer

jkrech commented 4 months ago

this solution removes uniqueness constraints on some elements I am keen to keep the uniqueness constraint even if it comes at the expense of requiring a fixed order of elements which is not required by the consumers of the file format.

gauthiergodart commented 4 months ago

I can understand that. But then, it seems to me you should put more efforts in clearly mentioning these kinds of implicit ordering requirements into the documentation. This applies to both the PDSC and SVD models.