Add initial Cluster support #182

jamesmunns · 2018-02-27T14:33:37Z

Add cluster support to svd2rust. I am calling it initial, because I have only tested it against the nRF52 svd file.

Note, this is based on #180, so I would prefer that PR be merged first. If that PR is rejected, I can instead rebase these changes on japaric/master (just let me know).

Edit: All changes relevant to clusters are squashed into the last commit of this PR, so if you want to see the relevant changes, check out 6de6de7.

This is based on the work of @brandonedens earlier #149, and I believe would close #107 and related issues.

CC @japaric @Emilgardis @therealprof

Edit 2: Here are some handy references:

Emilgardis

24b76d5 makes this very hard to review but I took a quick look and it seems valid.

As for 6de6de7 more comments in the code for cluster expanding to clarify what is happening will make maintaining this easier.

Emilgardis · 2018-02-27T14:56:19Z

src/generate/peripheral.rs

    Ok(quote! {
        /// Register block
        #[repr(C)]
-        pub struct RegisterBlock {
+        pub struct #name {


Is this compatible with frameworks like RTFM?

At least in my testing with nrf52, this did not cause any problems with rtfm. This is an artifact of #149, and I admit I do not fully understand it. I can try testing without this change if you would like.

I think I'm thinking of the older RTFM where (I think) RegisterBlock was directly used. Now we just deref "into" it so this shouldn't change anything.

@Emilgardis I actually just went and figured out what this does. It controls the name of structs that are clusters, such as https://gist.github.com/jamesmunns/03fd0d7d3595cd0816dbfa8daca0a553#file-ficr-diff-L56. Without this, the other structs inside of this scope would all have the name RegisterBlock, which would be a problem.

At this point, it is implicit in the code that name is always Some(...) in a cluster, and None when not in a cluster. I will add comments that improve that (now that I understand this 😄)

Emilgardis · 2018-02-27T15:03:01Z

src/generate/peripheral.rs

+    let name_to_ty = |name: &String, ns: Option<&str>| -> syn::Ty {
+        let ident = if let Some(ns) = ns {
+            Cow::Owned(
+                String::from("self::") + &ns.to_sanitized_snake_case() + "::"


I don't understand why this change is needed, care to explain?

This change allows for generation of ~~names~~ types like this: https://gist.github.com/jamesmunns/03fd0d7d3595cd0816dbfa8daca0a553#file-ficr-diff-L58

Since clusters are nested namespaces, it is necessary for the outer (cluster) structures to refer to types inside of the nested namespaces.

jamesmunns · 2018-02-27T15:28:20Z

I have also now tested this against ATSAMD21E15A.svd, and that has revealed that there are some additional problems that need to be fixed. One of these issues is fixed in d275d6f.

Another problem is that the ATSAMD svd has clusters with repeating names. This is currently not handled by my changes. See this snippet:

       <cluster>
        <name>USART</name>
        <description>USART Mode</description>
        <alternateCluster>I2CM</alternateCluster>
        <headerStructName>SercomUsart</headerStructName>
        <addressOffset>0x0</addressOffset>
        <register>
          <name>BAUD</name>
          <description>USART Baud Rate</description>
          <alternateGroup>DEFAULT_MODE</alternateGroup>
          <addressOffset>0x0C</addressOffset>
          <size>16</size>
          <fields>
            <field>
              <name>BAUD</name>
              <description>Baud Rate Value</description>
              <bitOffset>0</bitOffset>
              <bitWidth>16</bitWidth>
            </field>
          </fields>
        </register>
        <register>
          <name>BAUD</name>
          <description>USART Baud Rate</description>
          <alternateGroup>FRAC_MODE</alternateGroup>
          <addressOffset>0x0C</addressOffset>
          <size>16</size>
          <fields>
            <field>
              <name>BAUD</name>
              <description>Baud Rate Value</description>
              <bitOffset>0</bitOffset>
              <bitWidth>13</bitWidth>
            </field>
            <field>
              <name>FP</name>
              <description>Fractional Part</description>
              <bitOffset>13</bitOffset>
              <bitWidth>3</bitWidth>
            </field>
          </fields>
        </register>
        <register>

jamesmunns · 2018-02-27T15:31:16Z

@Emilgardis - apologies for basing this on my "refactoring" branch. It took me a while to understand what components had to do with which parts of the generation process, and the refactoring branch was me organizing things based on my learning while trying to tackle the cluster feature. As previously mentioned, I would be happy to rebase this on top of master (but I am hoping the other PR is merged).

Additionally I agree that I need to improve the situation with comments. I will plan to add those in the near future.

jamesmunns · 2018-02-27T16:08:16Z

I think #182 (comment) is not technically a blocker to merging this, though clusters that contain multiple registers at the same offset (like ATSAMD21E15A.svd shown above) will not (yet) be able to take advantage of the cluster support, until there is support for union'd types (I think covered by issue #16).

Devices (like the nrf52) that do not have union'd types may take advantage of this cluster support without a solution for #16 landing.

therealprof · 2018-02-28T14:24:31Z

Don't worry about the FUBAR ATSAMD too much, that'll only make your life miserable but doesn't bring any benefit to the cause. The chat I had with a Microchip guy yesterday at the Embedded World was very revealing, too.

bors · 2018-02-28T19:12:06Z

✌️ Emilgardis can now approve this pull request

japaric

This LGTM. I agree with @Emilgardis that some comments would be useful.

Another problem is that the ATSAMD svd has clusters with repeating names.

Is this even allowed by the SVD spec? I doesn't seem to say anything about this. It says:

"String to identify the register. Register names are required to be unique within the scope of a peripheral."

(emphasis mine)

It says something similar about cluster names within a peripheral but it doesn't say anything about register names within a cluster namespace ...

I'd be fine with postponing supporting this odd use case and landing what this PR implements. (We would need to support unions in svd2rust in any case.)

@Emilgardis could you take the review from here? I'll probably be rather busy over the next few days.

bors delegate=Emilgardis

@jamesmunns if Emilgardis is also busy feel free to r+-ing this yourself after adding more comments -- you have bors / reviewers rights.

japaric · 2018-02-28T18:56:56Z

src/generate/peripheral.rs

+            size: register_size,
+        }),
+        Register::Array(ref info, ref array_info) => {
+            let sequential_addresses = register_size == array_info.dim_increment * BITS_PER_BYTE;


This whole array_convertible logic repeats in expand_cluster and here. Could it be refactor into a function that takes array_info as argument?

japaric · 2018-02-28T19:02:47Z

src/util.rs

@@ -238,3 +239,25 @@ impl U32Ext for u32 {
        })
    }
 }
+
+/// Return only the clusters from the slice of either register or clusters.
+pub fn only_clusters(ercs: &[Either<Register, Cluster>]) -> Vec<&Cluster> {


off-topic: this looks like something that could be provided by the either crate. It looks a bit like Iterator::partition but that method returns two Vecs of the same type.

I opened an issue in rayon-rs/either#20 regarding this

At least right now, I won't be implementing this, because I can't figure out how to get itertools to figure out the vec of refs. Open to PRs in the future :)

jamesmunns · 2018-03-02T14:55:01Z

Hey, quick status update on this:

Currently register offsets inside of clusters are not being calculated correctly
I have a very ugly brute force fix for it, and I am working on cleaning it up a bit
My laptop fan just died, so I need to get that fixed before I can do any serious compilation or testing, so I expect a delay of a day or two.

I just wanted to follow up in case anyone other than me was trying to use this branch :). If you want to try out my ugly fixes, I can push those to another branch on my fork, just @ me.

therealprof · 2018-03-02T18:40:08Z

@jamesmunns I just tried a few SVD and it seems to work great. No regressions with simple ones and nrf51 gets a few more components by using this.

Few nits:

I'm getting lint errors due to gratuitous use of use vcell::VolatileCell; where not needed.
The formatting after rustfmt is slightly different, even worse than it was before, e.g. docstrings right after opening braces: pub enum SRCR {# [ doc = "Internal 32KiHz ...

jamesmunns · 2018-03-03T11:03:08Z

@therealprof Be careful with the current branch. In the generated output, the #[doc = ""] comments have the correct offsets, but extra _reservedxx fields are being generated if your clusters do not have an explicit <size> field. You can verify this in GDB by looking at the addresses of the fields.

Regarding formatting, yeah, its pretty rough. I wonder what we are doing that makes cargo fmt upset. Probably worth an improvement issue.

Regarding VolatileCell, I can probably take that out of the generated code, and we can just use vcell::VolatileCell; once at the top of the file (in fact, I think we already do).

therealprof · 2018-03-03T11:07:11Z

@jamesmunns I've just done some basic regression testing and worked for my curiosity (since you mentioned you require that for the nrf52 I was curious to see whether the nrf51 also has some additional hidden components -- yes it does!). No concrete use case at the moment.

jamesmunns · 2018-03-03T11:30:51Z

@therealprof I don't expect this to break anything that already worked before, but the "new stuff" (e.g. the peripherals/registers that were cluster based) I would expect not to work in some cases.

Were you testing the new peripherals? Or your existing nrf51-hal code?

therealprof · 2018-03-03T14:59:31Z

No, just checking that a swapped out version would compile. I haven't even noticed that (albeit very few) peripherals were missing so obviously I didn't those so far.

jamesmunns · 2018-03-04T21:55:35Z

Fixed size rendering (as far as I can tell).

bors r+

@brandonedens

182: Add initial Cluster support r=jamesmunns a=jamesmunns Add cluster support to svd2rust. I am calling it initial, because I have only tested it against the `nRF52` [svd file](https://github.com/jamesmunns/nrf52/blob/master/nrf52.svd). Note, this is based on #180, so I would prefer that PR be merged first. If that PR is rejected, I can instead rebase these changes on `japaric/master` (just let me know). Edit: All changes relevant to clusters are squashed into the last commit of this PR, so if you want to see the relevant changes, check out 6de6de7. This is based on the work of @brandonedens earlier #149, and I believe would close #107 and related issues. CC @japaric @Emilgardis @therealprof Edit 2: Here are some handy references: * [SVD snippet with clusters](https://gist.github.com/jamesmunns/c4e53fe5bd74dca81fdbff6bb1798ddd) * [code generated before](https://gist.github.com/jamesmunns/d854a6c2665cca59edb88143d82a19c6) * [code generated after](https://gist.github.com/jamesmunns/7558667e34c33124c60b3aaaf679a196) * [diff of code generated](https://gist.github.com/jamesmunns/03fd0d7d3595cd0816dbfa8daca0a553)

bors · 2018-03-04T23:07:17Z

Build failed

continuous-integration/travis-ci/push

jamesmunns · 2018-03-04T23:27:02Z

Looks like this failed due to the excess "VolatileCell" mentioned above: https://travis-ci.org/japaric/svd2rust/jobs/349051903 - I'll try and find a clever fix, otherwise I'll hit it with an #[allow(unused)].

jamesmunns · 2018-03-08T22:17:26Z

bors r+

@brandonedens

182: Add initial Cluster support r=jamesmunns a=jamesmunns Add cluster support to svd2rust. I am calling it initial, because I have only tested it against the `nRF52` [svd file](https://github.com/jamesmunns/nrf52/blob/master/nrf52.svd). Note, this is based on #180, so I would prefer that PR be merged first. If that PR is rejected, I can instead rebase these changes on `japaric/master` (just let me know). Edit: All changes relevant to clusters are squashed into the last commit of this PR, so if you want to see the relevant changes, check out 6de6de7. This is based on the work of @brandonedens earlier #149, and I believe would close #107 and related issues. CC @japaric @Emilgardis @therealprof Edit 2: Here are some handy references: * [SVD snippet with clusters](https://gist.github.com/jamesmunns/c4e53fe5bd74dca81fdbff6bb1798ddd) * [code generated before](https://gist.github.com/jamesmunns/d854a6c2665cca59edb88143d82a19c6) * [code generated after](https://gist.github.com/jamesmunns/7558667e34c33124c60b3aaaf679a196) * [diff of code generated](https://gist.github.com/jamesmunns/03fd0d7d3595cd0816dbfa8daca0a553)

bors · 2018-03-09T00:46:41Z

Timed out

jamesmunns · 2018-03-09T01:23:56Z

@japaric I'm a little confused - CI passed but Bors timed out? Is this okay to merge?

jamesmunns · 2018-03-09T02:09:53Z

Merging as tests have passed.

japaric · 2018-03-10T00:54:16Z

@jamesmunns I increased the timeout to 2 hours the other day. If it still takes longer than that to run the tests feel free to increase it.

istankovic · 2018-03-18T21:47:36Z

@therealprof:

Don't worry about the FUBAR ATSAMD too much, that'll only make your life miserable but doesn't bring any benefit to the cause. The chat I had with a Microchip guy yesterday at the Embedded World was very revealing, too.

Care to share the details? Also, I'd be curious to know whether you consider previously-Atmel chips FUBAR or just the SVDs. Thing is, I have an ATSAMD21 board that I'd like to bring up and play with the peripherals and the register cluster issue has been blocking, as far as I understand.

therealprof · 2018-03-18T23:37:38Z

@istankovic For whatever reason the engineers at (formerly) Atmel decided that it would be a great idea to have lots of totally asynchronously running clocks driving the individual peripherals. While (in theory) this yields a ton of freedom tweaking all the clocks to run at just the perfect speed for the job, it actually means that you can never be sure that data arrives at the right place at the right time due to clock drift so whenever you have peripherals working in lockstep (as one pretty much always does) you have to sprinkle costly busy wait loops all over the map in your peripheral access code. Not to mention that getting the clocks set up in the first place is a major PITA and can easily drive one mad because the chips will simply lock up somewhere in the initialisation code and you've no idea why... That combined with the fact that the documentation is in large portions conflicting, incorrect or even absent and Atmel seems to consider examples to be for wussies only is a bit of a buzzkill.

I was asking the Microchip guy at the Embedded World for new advancements in the BLE/802.15.4 area to which he frankly replied that they somewhat underestimated the demand for that so they're still playing catchup in their design teams. And when I nudged him a bit about my experience with the ATSAM chips and the crazy clocks he just nodded and said that this probably wasn't the brightest hour of the engineering team.

The SVDs are just another sad part of the story...

Wez Furlong is working on ATSAMD21 support and if you want to look at something "working" (although definitely not pretty) you might want to have a look at my https://github.com/therealprof/atsamd20e15a which implements something "shiny" for the lowlier but otherwise mostly compatible brother.

istankovic · 2018-03-19T13:21:51Z

@therealprof thanks for the elaboration. Yes, regarding the Atmel parts, clock configuration can sometimes really be painful and I spent many hours debugging non-functioning peripherals. However, I'd say that once one gets used to it, it isn't such a big deal and it has its advantages (lower power, speed optimization etc.) Regarding the documentation, I'm not sure about the current state, but back when I was actively working with Atmel parts, AVRs upto Cortex-Ms, I considered Atmel docs to be one of the better in the industry, honestly. One could actually build a system solely by following a datasheet and perhaps consulting a few app notes, without needing to spend a great deal of time searching the net for answers/advice. Maybe that changed.

Thanks for the pointer, I'll check out your repo.

therealprof · 2018-03-19T13:27:17Z

@istankovic

it isn't such a big deal and it has its advantages (lower power, speed optimization etc.)

That's what they claim, I haven't seen that in real life though.

Atmel parts, AVRs upto Cortex-Ms, I considered Atmel docs to be one of the better in the industry, honestly.

The AVR documentation is indeed really good; I've worked a lot with ATTiny, ATMega and AT90 in the past and those are well documented in stark contrast to the ATSAMD parts; haven't bothered checking other ATSAM parts though.

jamesmunns mentioned this pull request Feb 27, 2018

Missing peripherals in the generated file #181

Closed

Emilgardis reviewed Feb 27, 2018

View reviewed changes

Emilgardis mentioned this pull request Feb 27, 2018

Make EPnR into an array. japaric/stm32f103xx#19

Merged

japaric reviewed Feb 28, 2018

View reviewed changes

Emilgardis mentioned this pull request Mar 1, 2018

Provide an iterator adapter to filtermap/partition left or right branches on I::Item : Either<L,R> rayon-rs/either#20

Closed

jamesmunns added 3 commits March 4, 2018 22:35

Teach svd2rust about clusters

4bc534d

Do not skip peripherals with only clusters

770fc3d

Add comments, clean up remaining unwraps

ef2d9c2

jamesmunns force-pushed the reorg-clusters branch from d275d6f to ef2d9c2 Compare March 4, 2018 21:37

Don't use vcell::VolatileCell, use ::vcell::VolatileCell

f3f4d66

jamesmunns merged commit 631ab3e into rust-embedded:master Mar 9, 2018

Add initial Cluster support #182

Add initial Cluster support #182

Conversation

jamesmunns commented Feb 27, 2018 • edited Loading

Emilgardis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emilgardis Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmunns Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

jamesmunns commented Feb 27, 2018

jamesmunns commented Feb 27, 2018

jamesmunns commented Feb 27, 2018 • edited Loading

therealprof commented Feb 28, 2018

bors bot commented Feb 28, 2018

japaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmunns commented Mar 2, 2018

therealprof commented Mar 2, 2018

jamesmunns commented Mar 3, 2018

therealprof commented Mar 3, 2018

jamesmunns commented Mar 3, 2018

therealprof commented Mar 3, 2018

jamesmunns commented Mar 4, 2018

bors bot commented Mar 4, 2018

Build failed

jamesmunns commented Mar 4, 2018

jamesmunns commented Mar 8, 2018

bors bot commented Mar 9, 2018

Timed out

jamesmunns commented Mar 9, 2018

jamesmunns commented Mar 9, 2018

japaric commented Mar 10, 2018

istankovic commented Mar 18, 2018

therealprof commented Mar 18, 2018

istankovic commented Mar 19, 2018

therealprof commented Mar 19, 2018

jamesmunns commented Feb 27, 2018 •

edited

Loading

Emilgardis Feb 27, 2018 •

edited

Loading

jamesmunns Feb 27, 2018 •

edited

Loading

jamesmunns commented Feb 27, 2018 •

edited

Loading