Fix SVD schema violations

[Rahix]

We should verify that output from atdf2svd is valid in regards to the SVD schema: https://www.keil.com/pack/doc/CMSIS/SVD/html/schema_1_2_gr.html

[Gasman2014]

Present SVD files generated by atdf2svd do not conform to the CMSIS schema and the SVDConv validator fails.

In particular, the SVD files lack a '<CPU>' section as outlined here https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_cpu.html

There are some errors of the form '<TBD>' which are parsed as unclosed '<TBD>' tags rather than simply 'TBD'.

Unfortunately SVDConv is not very tolerant of malformed SVD files and simply segfaults, so testing compliance is tedious.

[Gasman2014]

Sorry, the incorrect 'TBD tags' lookalike this - <TBD> (GitHub 'corrected' it for me)!

[Rahix]

Thanks a lot for taking a look. I think the best way forward is for me to fix these two issues so you can then rerun the tool on the "improved" SVDs. Does that sound alright to you?

[Gasman2014]

Yes, that's a good plan. My aim was to be able to use the peripherals view in PlatformIO. If I hack the SVDs around to include the CPU directive etc and edit out the tag problems I still run into problems so there are certainly further issues.
Nevertheless, I have then stripped out all but two or three registers and got it to pass SVDConv and it loads correctly in PlatformIO. I think you also need to adjust the XML directive on the header - the Keil reference has an example. Whether the registers are actually correct is still to be determined!

[Rahix]

Hm, according to https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_device.html, the <cpu> element is optional... I guess I can add it anyway, but there's more inconsistencies going on here than just what atdf2svd is doing ^^

[Gasman2014]

OK, once I had figured out how to use Cargo with a git checkout, I popped the Attiny85 file in and generated a new SVD. When I tested this it segfaulted immediately :( .

However, if I strip out most of the the peripherals, the file at least now parses. I stripped out everything apart from the Analog Comparator (First peripheral section) and now we get this;

CMSIS-SVD SVD Consistency Checker / Header File Generator V3.3.35
Copyright (C) 2010 - 2020 ARM Ltd and ARM Germany GmbH. All rights reserved.

Arguments: "attiny85.svd" 

*** ERROR M327: attiny85.svd (Line 10) 
  NVIC Prio Bits not set or wrong value, must be 2..8. Using default (4)

*** WARNING M362: attiny85.svd (Line 60) 
  EnumeratedValue description 'Reserved': 'RESERVED' items must not be defined.

*** INFO M347: attiny85.svd (Line 77) 
  Field 'ACIE' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 83) 
  Field 'ACI' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 89) 
  Field 'ACO' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 95) 
  Field 'ACBG' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 101) 
  Field 'ACD' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 115) 
  Field 'ACME' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 129) 
  Field 'AIN0D' (width < 6Bit) without any <enumeratedValue> found.

*** INFO M347: attiny85.svd (Line 135) 
  Field 'AIN1D' (width < 6Bit) without any <enumeratedValue> found.

*** WARNING M306: attiny85.svd
  Schema Version not set for <device>.

*** WARNING M223: attiny85.svd (Line 2) 
  Input File Name 'attiny85' does not match the tag <name> in the <device> section: 'ATtiny85'

*** WARNING M350: attiny85.svd (Line 20) 
  Peripheral 'AC' (@0x00000023) is not 4Byte-aligned.

*** INFO M356: attiny85.svd
  No Interrupt definitions found.

*** INFO M317: attiny85.svd (Line 2) 
  Device <description> not set.


Found 1 Error(s) and 4 Warning(s).

There are are few easy issues but more extensive debugging might take a while.

[Rahix]

Thanks! Pushed a fix for

*** ERROR M327: attiny85.svd (Line 10) 
  NVIC Prio Bits not set or wrong value, must be 2..8. Using default (4)

to #28 already, going to look at the other errors now.

[Rahix]

The segfault seems to be caused by the following blocks:

          <writeConstraint>
            <range>
              <minimum>0</minimum>
              <maximum>65535</maximum>
            </range>
          </writeConstraint>

As far as I can tell those are correct by the specification (https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_registers.html#elem_writeConstraint) but SVDConv certainly isn't happy about them. I'm not sure what's best - we could add a flag to atdf2svd to stop emitting them, maybe?

Edit: For reference, the following patch disables their generation entirely:

diff --git a/src/svd/restriction.rs b/src/svd/restriction.rs
index ae7d8e275abe..767254bea2a3 100644
--- a/src/svd/restriction.rs
+++ b/src/svd/restriction.rs
@@ -17,7 +17,7 @@ pub fn generate(
                 range.child_with_text("minimum", 0.to_string());
                 range.child_with_text("maximum", (2usize.pow(width as u32) - 1).to_string());
                 el.children.push(range);
-                vec![el]
+                vec![]
             }
         }
         chip::ValueRestriction::Range(lo, hi) => {

[Rahix]

Cc: @evilmav, @maxgerhardt, this issue might interest you as well.

[jeandudey]

The segfault seems to be caused by the following blocks:
...
As far as I can tell those are correct by the specification (https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_registers.html#elem_writeConstraint) but SVDConv certainly isn't happy about them. I'm not sure what's best - we could add a flag to atdf2svd to stop emitting them, maybe?

I feel like it's easier to just ignore the writeConstraint block at all in that case as it's not required in that case, the software that uses the SVD file should generate a correct bit mask from the bitRange for the max and min values that it can read/write.

---

On a separate topic, maybe we could also use the crates from rust-embedded/svd to encode the SVD files to help with schema correctness, @Rahix I can do the work if you're fine with it.

[Rahix]

I feel like it's easier to just ignore the writeConstraint block at all in that case as it's not required in that case, the software that uses the SVD file should generate a correct bit mask from the bitRange for the max and min values that it can read/write.

IIRC the writeConstraint was necessary to make svd2rust generate the correct API that we want. Maybe this has changed, though...

On a separate topic, maybe we could also use the crates from rust-embedded/svd to encode the SVD files to help with schema correctness, @Rahix I can do the work if you're fine with it.

This sounds like a very good idea. If you're willing to work on this, please, go ahead :)

[jeandudey]

IIRC the writeConstraint was necessary to make svd2rust generate the correct API that we want. Maybe this has changed, though...

Just tested on a recent svd2rust and works perfectly without the writeConstraint, and it's not required by the schema to be present so we can go ahead with it.

[Rahix]

No, it does make a difference. Applying my patch from above right now, the generated code changes as follows:

diff --color -Naur src/devices-original/atmega32u4/adc/admux.rs src/devices/atmega32u4/adc/admux.rs
--- src/devices-original/atmega32u4/adc/admux.rs	2022-09-28 19:55:52.133458014 +0200
+++ src/devices/atmega32u4/adc/admux.rs	2022-10-11 21:23:49.049998459 +0200
@@ -55,7 +55,7 @@
 impl<'a> MUX_W<'a> {
     #[doc = r"Writes raw bits to the field"]
     #[inline(always)]
-    pub fn bits(self, value: u8) -> &'a mut W {
+    pub unsafe fn bits(self, value: u8) -> &'a mut W {
         self.w.bits = (self.w.bits & !0x1f) | (value as u8 & 0x1f);
         self.w
     }
diff --color -Naur src/devices-original/atmega32u4/cpu/clksel0.rs src/devices/atmega32u4/cpu/clksel0.rs
--- src/devices-original/atmega32u4/cpu/clksel0.rs	2022-09-28 19:55:52.943468209 +0200
+++ src/devices/atmega32u4/cpu/clksel0.rs	2022-10-11 21:23:49.079998842 +0200
@@ -163,7 +163,7 @@
 impl<'a> EXSUT_W<'a> {
     #[doc = r"Writes raw bits to the field"]
     #[inline(always)]
-    pub fn bits(self, value: u8) -> &'a mut W {
+    pub unsafe fn bits(self, value: u8) -> &'a mut W {
         self.w.bits = (self.w.bits & !(0x03 << 4)) | ((value as u8 & 0x03) << 4);
         self.w
     }
@@ -189,7 +189,7 @@
 impl<'a> RCSUT_W<'a> {
     #[doc = r"Writes raw bits to the field"]
     #[inline(always)]
-    pub fn bits(self, value: u8) -> &'a mut W {
+    pub unsafe fn bits(self, value: u8) -> &'a mut W {
         self.w.bits = (self.w.bits & !(0x03 << 6)) | ((value as u8 & 0x03) << 6);
         self.w
     }

The write-constraints are used to signal to svd2rust that writing any value to this register/field is fine so the bits() method may be safe.

Fine print: It only works for fields, for registers there is a very recent PR rust-embedded/svd2rust#666.

[jeandudey]

I'll re-add the constraints on #34 . Though it's counterintuitive to mark it unsafe when a field has no write constraints, I would interpret that not as a lack of write constraint information but as freedom to write any value.

[Rahix]

With #34 having been merged a long time ago, this issue is resolved. If we have any more SVD schema violations, they're now problems to be solved by svd-rs :D