Correctly handle case where register.name has "[%s]" and no dimIndex

[jamesmunns]

No description provided.

[therealprof]

Fantastic, that was a big problem for me with the ATSAMD20 SVD files. Will check it out later...

[japaric]

Thanks, @jamesmunns

@homunkulus r+

Some builds will likely timeout but I'll test the problematic test suites locally.

[homunkulus]

📌 Commit d0a2259 has been approved by japaric

[homunkulus]

⌛ Testing commit d0a2259 with merge 5eaf83f...

[therealprof]

Hm, weird:

$ svd2rust -i ATSAMD21E15A.svd | rustfmt >out.rs
error: Register COMPCTRL%s has no dim_index field
note: run with RUST_BACKTRACE=1 for a backtrace

Where ATSAMD21E15A.svd is from https://github.com/posborne/cmsis-svd.git and svd2rust at commit 5eaf83f from the auto branch.

[jamesmunns]

@therealprof thats actually (sadly) expected. My patch only covers registers that look like DEVICEID[%s] (notice the []s). You can see a bigger chunk here: https://gist.github.com/jamesmunns/3de64577afb9c45edee8ec597c9a2e34#file-gistfile1-txt-L25

I don't know the correct way to handle FOO%s, and the linked documentation from keil only says dimIndex should not be used with the bracketed version.

If you want to test it, you can try hacking up this line to be more general: 5eaf83f#diff-6755e3579c3e8158c7d672a1ddebb24dR569

[jamesmunns]

@therealprof here is a link to the documentation I referenced: http://www.keil.com/pack/doc/CMSIS/SVD/html/elem_registers.html

[homunkulus]

💔 Test failed - status-travis

[jamesmunns]

@therealprof
From the documentation:

name

String to identify the register. Register names are required to be unique within the scope of a peripheral. You can use the placeholder %s, which is replaced by the dimIndex substring.

That reads to me like dimIndex should be mandatory if you use FOO%s as a name. I'm guessing the Atmel SVD doesn't follow the standard?

You could probably use the new code path I added (but maybe add a STDERR warning like "using non-standard dim_index"?), but you would need to confirm it generates what you want it to generate.

[jamesmunns]

@japaric Not sure what that failure was. Something about a missing msp430 crate? The build output looks a little funky when generating that Cargo.toml.

[therealprof]

@jamesmunns Yeah, don't know. I modified it and checked the result but it doesn't look quite right. That's fine.

The Atmel guys are full of crap anyway, not only generating one SVD file per footprint but also per Flash/RAM configuration and don't even manage to get their own spelling and register names consistent within those and the SVD files for sibling MCUs. Apart from that horror show their clocking system and peripherals are really disgusting so I'm happy to take this as an excuse not to waste any more time with those MCUs. 😉

[japaric]

@homunkulus r+

[homunkulus]

📌 Commit 550982e has been approved by japaric

[homunkulus]

⌛ Testing commit 550982e with merge fc26bd2...

[jamesmunns]

@therealprof Aw, thats sad to hear. I don't have any experience with the Atmel Cortex-Ms, but had generally heard good things. Hopefully they only suck at making SVDs :)

[therealprof]

@jamesmunns Can't say what's worse, their peripheral implementation or their documentation. Probably correlated... Everything is asynchronous in their implementation (so you're doing busy waits all the time and everywhere) and you have mind boggling numbers of clocks to configure to make even the most simple things work and if you miss any important (and most likely undocumented or wrongly documented) step or bit in a register you're just busy waiting forever and have no idea why... and who needs sample code anyway?!?

Those ATSAM are pretty much the exact opposite of the perfectly documented and easy to use ATTiny/Mega chips. No way in hell I'm ever going near them again if I don't absolutely have to.

[kevinmehall]

The ARM version of the SVD spec includes the wording:

By default, <dimIndex> is a value starting with 0.
Remark: Do not define <dimIndex> when using the placeholder [%s] in <name> or <displayName>.
Occurrance: 0..1

Which reads to me that the it defaults to sequential numbering whether it uses %s or [%s], but specifically requires that it is not used with [%s]. This is what I implemented in #168.

[jamesmunns]

@kevinmehall I just tested, and your branch produces the same output (for me) as mine does. Yours appears to be better implemented than mine, I'm okay with @japaric closing this PR and merging yours instead.

Thanks for chiming in! I hadn't seen your previous PR.

[homunkulus]

💔 Test failed - status-travis

[japaric]

Closing in favor of #168