Issues blocking `update.sh` on master

[gkelly]

I recently started adding support for ATSAMD51P19A. While I was doing this I ran update.sh and noticed that at least one other ATSAMD51 doesn't actually build at head. I started digging in to this and discovered a few issues.

• svd2rust 0.17.0 doesn't support dimable names (CHANNEL[%s], for example). This is because in December of 2019 the svd-parser package was patched to enforce naming rules, but it uses the pre-CMSIS 1.3.3 naming rules.
  • Fixed in Support dimableIdentifierType-style names rust-embedded/svd#119.
• svd2rust is missing adding some use declarations in some modules.
  • Fixed in Cluster name qualification rust-embedded/svd2rust#444.
• Some register reset values generated by svd2rust are bigger than their containing types. This doesn't get caught until after svd patch, svd2rust, and form are run. This doesn't block getting update.sh running again, but it's a quality-of-life improvement that would help catch breakages early.
  • Fixed in Enforce register value constraints rust-embedded/svd#120.
• Update update.sh to install the release of svd2rust that includes the above changes.
  • Proposed Use bleeding-edge svd2rust #197 to address.
• Add SVD patch yamls for devices that are failing to generate.
  • ATSAMD5X-series devices.
    • Fixed in Patch invalid SVDs for several ATSAMD5Xs #196.
  • ATSAMD11C14A. USB.DEVICE cluster has wrong register size.
• Run update.sh and merge results as a PR.

[jessebraham]

Thanks for opening this issue. I am currently working on at least some related problems, just haven't had a ton of time for this project lately unfortunately. Probably should have opened an issue myself, so thanks for taking care of that.

I have a few patches to be applied to the SVDs locally, however I need to patch svdtools one more time to allow modifying nested clusters; once there is support for that, the SVDs can be fully patched (as far as I know) and that will help at least some issues.

Regarding the clusters, AFAIK you are correct in that svd2rust has some issues processing them currently, so we're sort of blocked on that too.

I will try to list out what has been done and what needs to be done (other than what you've posted) later tonight or this week when I get a bit more time.

[gkelly]

Thanks for opening this issue. I am currently working on at least some related problems, just haven't had a ton of time for this project lately unfortunately. Probably should have opened an issue myself, so thanks for taking care of that.

I have a few patches to be applied to the SVDs locally, however I need to patch svdtools one more time to allow modifying nested clusters; once there is support for that, the SVDs can be fully patched (as far as I know) and that will help at least some issues.

I know that svd patch has a hard time touching the descriptions of clusters, but svd-parser at tip can parse empty descriptions fine. I think getting svd-parser fixed to handle in-spec names (CHANNEL[%s] is valid in CMSIS 1.3.3+) means not having to modify clusters, doesn't it?

Regarding the clusters, AFAIK you are correct in that svd2rust has some issues processing them currently, so we're sort of blocked on that too.

I will try to list out what has been done and what needs to be done (other than what you've posted) later tonight or this week when I get a bit more time.

[gkelly]

I tracked down the cluster type name generation issue in svd2rust and have put up a PR that hopefully addresses it: rust-embedded/svd2rust#444.

[gkelly]

Those changes are merged. There are still SVD patches needed (I believe) to make things work with tip svd2rust and tip svd-parser. I don't know what the release cadence is for those tools, but hopefully it won't be too long before those changes are in a release and can be referenced from update.sh. I'll update the checklist to capture that.

[gkelly]

The only error I haven't been able to svd patch out with a tip svd2rust and tip svd-parser is ATSAMD11C14A's:

[ERROR svd2rust] In device `ATSAMD11C14A`                                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                                                            
    Caused by:                                                                                                                                
        0: In peripheral `USB`                                                                                                                
        1: In cluster `DEVICE`                                                                                                                
        2: In register `FSMSTATUS`                                                                                                            
        3: Parsing field #0                                                                                                                   
        4: In field `FSMSTATE`                                                                                                                
        5: Value 64 out of range 0..64

It's in the DEVICE cluster, which I'm not sure if svdtools can currently get inside very well.

[wez]

I don't know what the release cadence is for those tools, but hopefully it won't be too long before those changes are in a release and can be referenced from update.sh. I'll update the checklist to capture that.

Hopefully they're better these days, but in the past it wasn't great; I had a PR that took a year to get merged and shipped, so I used to maintain a fork for atsamd. I'd suggest pointing to your fork (or we can make a fork under the atsamd-rs org if you don't want to own that repo) if you feel like it is taking a while and getting in the way of the stuff you really want to work on; you can see how we used to do this in the history of update.sh.

[gkelly]

I don't know what the release cadence is for those tools, but hopefully it won't be too long before those changes are in a release and can be referenced from update.sh. I'll update the checklist to capture that.

Hopefully they're better these days, but in the past it wasn't great; I had a PR that took a year to get merged and shipped, so I used to maintain a fork for atsamd. I'd suggest pointing to your fork (or we can make a fork under the atsamd-rs org if you don't want to own that repo) if you feel like it is taking a while and getting in the way of the stuff you really want to work on; you can see how we used to do this in the history of update.sh.

Seems reasonable to me, thanks for the advice. With those two patches I just put up things are very close to building. The last broken thing I haven't figured out yet is ATSAMD11C14A, where the SVD error is inside the USB.DEVICE cluster. That might take an svdtools patch to fix. @jessebraham do you have any tricks up your sleeve on that front, or were you looking at a different part?

[jessebraham]

Sorry for the delay, I've had a very busy week. Thank you for putting the time and effort into this, I do appreciate it!

I'm glad to hear you got a PR merged in svd2rust. I know the release cycle on some of the rust-embedded crates can be quite lengthy, so hopefully we don't need to wait too long for your changes to be released. I do think that (at least temporarily) maintaining our own fork probably isn't the worst idea, as we could potentially be waiting quite some time and it's reasonably easy to revert back to the upstream version at a later date. I'll leave that decision up to you/others though :)

You are correct that svdtools does not handle clusters especially well in its current state. I did get a PR merged which enabled modifying clusters, but it has yet to released. It might help, might not. I think there are also some nested clusters in our SVDs which cause further problems, and I don't know how well (or if at all) they're handled by svdtools unfortunately.

If I missed anything or you need anything else just let me know, I should be mostly available throughout this weekend.

[gkelly]

Sorry for the delay, I've had a very busy week. Thank you for putting the time and effort into this, I do appreciate it!

No worries, thanks for taking the time to review and respond!

I'm glad to hear you got a PR merged in svd2rust. I know the release cycle on some of the rust-embedded crates can be quite lengthy, so hopefully we don't need to wait too long for your changes to be released. I do think that (at least temporarily) maintaining our own fork probably isn't the worst idea, as we could potentially be waiting quite some time and it's reasonably easy to revert back to the upstream version at a later date. I'll leave that decision up to you/others though :)

I've updated #197 to use a pinned fork of svd2rust.

You are correct that svdtools does not handle clusters especially well in its current state. I did get a PR merged which enabled modifying clusters, but it has yet to released. It might help, might not. I think there are also some nested clusters in our SVDs which cause further problems, and I don't know how well (or if at all) they're handled by svdtools unfortunately.

Using the gkelly/svd2rust@2bbb605 version of svd2rust and having #196 landed I believe only ATSAMD11C14A fails to generate, and it's because of the wrong register size inside of the USB.DEVICE cluster. I'll take a look to see if your merged svdtools change can handle that modification.

If I missed anything or you need anything else just let me know, I should be mostly available throughout this weekend.

Thanks, I appreciate the reviews and responses!