Emit unions for overlapping/overloaded registers #192
Conversation
|
I was hoping this would kick the CI but it didn't; what did I do wrong? |
|
@wez - this change makes sense to me, and seems to work with my existing @Emilgardis or @japaric, any thoughts? It seems like the current "normal" behavior is to reject any overlaps generated by svd2rust. |
|
Another possible option would be to impl #183, and make strong warnings when overlaps are detected, and/or add a command line option like |
|
re: overlap detection, I started adding some logic to collect overlapping regions together so that I can emit unions for them (eg: resolve #16). I think I need to do that to make reasonable progress with the adafruit boards in the long run (eg: USB registers have host and device mode aliased into the same location), so I don't mind blocking landing this code change until I've made some more progress on the union angle. |
|
I've added a commit that shows where I'm headed; the It emits output like this for the atsamd21 in place of the older There's some fishy looking stuff elsewhere in our interpretation of this svd file: The corresponding bit of the SVD is derivedFrom PMUX0_%s and has bitfields: I'm assuming that this has to do with the bit fields but it is also possible that my alternative size calculation might be to blame. Will need to dig in to this a bit more to be sure. |
|
@wez thanks for the update! That output looks promising. Feel free to let me know if I can help with anything, or if you want me to take a look. |
|
@wez Looks good so far! I'll make a proper review once more work has been done. |
|
Thanks; I'll keep plugging away in the meantime. |
|
With the commit I just added, we can now emit code like this for overlapping registers: This code is good enough to pass There are a couple of TODOs/questions in here:
[Edit: I went ahead and did these TODOs] |
wez
changed the title
jamesmunns
added
enhancement
missing svd feature
|
OK, I think this is big enough and (seemingly!) working well enough state that it is ready for code review and running the CI. |
src/generate/peripheral.rs
Outdated
| let idx = self.regions.binary_search_by_key(&new_region.offset, |r| r.offset); | ||
| match idx { | ||
| Ok(idx) => { | ||
| panic!("we shouldn't exist in the vec, but are at idx {} {:#?}\n{:#?}", |
There was a problem hiding this comment.
Function should return Result<()> and this call should be made into an error. You can use error_chain's bail! macro.
src/generate/peripheral.rs
Outdated
| Ok(quote! { | ||
| /// Register block | ||
| #[repr(C)] | ||
| pub struct #name { | ||
| pub #block_type #name { |
There was a problem hiding this comment.
I'm not sure about this change. At first glance it seems fine but I'm not 100% on this change.
src/generate/peripheral.rs
Outdated
| let mut min_len = usize::max_value(); | ||
|
|
||
| for f in &self.fields { | ||
| match f.field.ident { |
There was a problem hiding this comment.
Would prefer an if let else here
src/generate/peripheral.rs
Outdated
| regions.add(reg_block_field); | ||
| } | ||
|
|
||
| let block_is_union = regions.regions.len() == 1 && regions.regions[0].fields.len() > 1; |
There was a problem hiding this comment.
Could you make this into a function on FieldRegion instead to make it look "better"
There was a problem hiding this comment.
I did a run and it looks very promising, however, the description/doc on *_union doesn't feel helpful at all. I propose that we just throw it away if they don't all match.
I also have some comments on the naming of the union.
First, the common prefix algorithm isn't correct.
Second, unions are largely much alike structs and as such should IMO be named appropriately. i.e Prefix_Union or something similar if we go by how we are currently naming our idents.
| // reasonable short description, but it's good enough. | ||
| if f.description != result { | ||
| if result.len() > 0 { | ||
| result.push(' '); |
There was a problem hiding this comment.
This results in some wierd looking documentation
There was a problem hiding this comment.
Status Register overloaded Status Register
for example
src/generate/peripheral.rs
Outdated
| impl Region { | ||
| /// Find the longest common prefix of the identifier names | ||
| /// for the fields in this region, if any. | ||
| fn common_prefix(&self) -> Option<String> { |
There was a problem hiding this comment.
This doesn't find the longest common prefix as explained.
Take two registers fast and foo
This algorithm will say that fas is the prefix which to me doesn't sound correct [playground link]. Even wierder is that if you change the order the prefix becomes foo
I think a better algorithm would be to split each ident at a non-alpha character and calculate where the first mismatch is and use that.
I've implemented just that here (playground link).
There was a problem hiding this comment.
To that implementation there may be a need to add a grouping of numerical and non-alpha characters to make foo1 and foo12 have a common prefix of foo1, however, this would really depend on the context of the register because foo may very well be a valid common prefix.
Right now it outputs foo1or foo depending on which one is first. (Sorting the vec by length of str beforehand would solve this)
|
ping @wez |
|
Do you have suggestions on naming? It's a bit difficult to move forward without some guidance on what you want to see. I can certainly fix the bug in the common prefix code but if you don't want that then there's not a lot of point :-) Thoughts? |
|
I think Right now unions are named |
|
I'm pretty happy with btw @wez I rebased your changes against the current master (because it has the unofficial fast regression tester in it), and kicked off an unofficial CI test to see how things go. Its still running (ETA 25 minutes after I post this), but you can check it out here: https://gitlab.onevariable.com/james/svd2rust/-/jobs/93 - if either of you would like access to this repo so you can run testing there (its a beefy machine I use for builds), make an account on my gitlab server and ping me so I can give you push rights. |
|
@wez it looks like there were some regressions on a number of the Freescale chips (but only those, it looks like). I'd suggest grabbing my rebased version of your branch and checking out the logs and output. You can use the regression tester to run single tests, and it will capture all of the output to log files for you. |
|
Finally got around to looking into this; the regression tester makes this much easier! We don't have a great way to automatically resolve such a conflict so I have a WIP that deals with this just by taking the shortest identifier from the members of the union and basing the union name off that. So in the example I mentioned above, we produce: Does this sound good enough? It has the nice property of guaranteeing that there are no possible conflicts in the names (assuming that the underlying svd file has no conflicting names!), and avoids doing non-trivial mapping and processing for ourselves. In the example above you'd use I haven't tackled the casing of |
|
Rebased and force pushed. The regression tests passed (takes my laptop 2 hours to run them!) except for this one that appears unrelated; I'm assuming that Here's an example of the naming: |
src/generate/peripheral.rs
Outdated
| (Ident::new(format!("{}Union", prefix.to_sanitized_upper_case())), | ||
| Ident::new(prefix)) | ||
| } | ||
| // If we can't find a name, fall back to theregion index as a |
src/generate/peripheral.rs
Outdated
| } | ||
|
|
||
| impl Region { | ||
| fn shortest_ident(&self) -> Option<String> { |
There was a problem hiding this comment.
This makes for some wierd union names. However, with your example of dsr0 and dsr_bcr0 together with dsr1 and dsr_bcr1 we'd have to have this. I'd much prefer my earlier attempt #192 (comment) but that wouldn't solve things for the mentioned case.
One alternative to solve this would be to simply not support the above case, as dsr0_bcr is IMO more accurate than dsr_bcr0. This would mean that we'd have to add some patches for SVDs that have these patterns. We could also add a check (after code gen of a peripheral) for conflicting union type names to clarify that compilation will fail and recommend how to fix it.
This change would mean that for the example mentioned we would still have Dsr0Union and Dsr1Union
Example of a wierd union name (with fixed pascal case)
#[doc = "The Prescale Register stores the Value for the prescaler. The cont event gets divided by this value"]
#[repr(C)]
pub union PrescaleRdUnion {
# [ doc = "0x28 - The Prescale Register stores the Value for the prescaler. The cont event gets divided by this value" ]
pub prescale_wr: PRESCALE_WR,
# [ doc = "0x28 - The Prescale Register stores the Value for the prescaler. The cont event gets divided by this value" ]
pub prescale_rd: PRESCALE_RD,
}
Using PrescaleUnion here makes the most sense.
There was a problem hiding this comment.
I started out by integrating your code from the playground link and added some unit tests for it (there were a couple of minor issues, for example, the dsr case mentioned above would yield an empty identifier), but the conflicts were the limiting factor. It's a shame that https://github.com/rust-lang/rfcs/blob/master/text/2102-unnamed-fields.md isn't available; it only just got merged and tracked in rust-lang/rust#49804
There was a problem hiding this comment.
FWIW, I think it would be a shame if we forced the user to patch the SVD file to generate something usable. I think its fine to be opinionated about naming and maintain a patched SVD, but requiring a patched SVD feels a bit heavyweight to me.
There was a problem hiding this comment.
One solution would be to have two implementations for naming, before code gen we use the fixed common_prefix function and check that there is no name "collision" with other types and that a common prefix exists, if there aren't any problems carry on with code gen, otherwise use the current implementation (shortest_ident) for those types that are affected. I think this would satisfy us both.
src/generate/peripheral.rs
Outdated
| if region.fields.len() > 1 && !block_is_union { | ||
| let (type_name, name) = match region.shortest_ident() { | ||
| Some(prefix) => { | ||
| (Ident::new(format!("{}Union", prefix.to_sanitized_upper_case())), |
There was a problem hiding this comment.
Should be sanitized PascalCase
|
cc @wez are you still working on this? |
|
I'm waiting for feedback; it's not clear what you want to be changed. |
|
I've spoken with @japaric and confirmed that it will work fine. There is still one issue left to resolve which I forgot about, we will need to make unions a nightly-only feature as we are stabilizing embedded on rust (issue rust-embedded/wg#42). To fix this you should add a new opt-in flag .arg(
Arg::with_name("nightly_features")
.long("nightly")
.help("Enable features only available to nightly rustc")
)and change the logic of You can do this easily by adding two functions I'm sorry for the inconvenience :/ |
|
I've rebased, cherry picked in your name resolution, added the Is that what you had in mind? |
See: rust-embedded/svd2rust#192 (this also requires the commits from https://github.com/wez/svd2rust/tree/wezlocal to make the pmux and pincfg bits compile for gpio)
|
Thank you! bors r+ |
192: Emit unions for overlapping/overloaded registers r=Emilgardis a=wez I want to get a complete working build for ATSAMD21 (rust-embedded/wg#61) so I'm taking a stab at that. * Introduced a `FieldRegion` helper to reason about overlapping regions * If overlaps are detected, a `union` container is emitted * If the only item in a `RegisterBlock` is a union, that `RegisterBlock` is emitted as a union * Otherwise: we generate a name for the union field by taking the longest common prefix of the union's alternates, or just pick an artificial name like `u1` if there was no common prefix. * If the starting address offset of elements in a union are not all the same, we don't have a way to emit padding for them today. We will emit a warning (and bad code) in that case (example below). The one example of this I see in ATSAMD21 is due to missing `derivedFrom` support for registers; we're currently generating bad code for these anyway. I have resolved in another branch that I'll turn into a PR once this one is landed. ``` WARNING: field Some(Ident("pmux1_1")) has different offset 177 than its union container 176 WARNING: field Some(Ident("pmux1_2")) has different offset 178 than its union container 176 ``` Examples: ``` #[doc = "Real-Time Counter"] pub mod rtc { #[doc = r" Register block"] #[repr(C)] pub union RegisterBlock { #[doc = "0x00 - Clock/Calendar with Alarm"] pub mode2: MODE2, #[doc = "0x00 - 16-bit Counter with Two 16-bit Compares"] pub mode1: MODE1, #[doc = "0x00 - 32-bit Counter with Single 32-bit Compare"] pub mode0: MODE0, } ``` ``` #[doc = r" Register block"] #[repr(C)] pub struct USART { #[doc = "0x00 - USART Control A"] pub ctrla: self::usart::CTRLA, #[doc = "0x04 - USART Control B"] pub ctrlb: self::usart::CTRLB, _reserved2: [u8; 4usize], #[doc = "USART Baud Rate"] pub baud: baud_union, ... } #[doc = "USART Baud Rate"] #[repr(C)] pub union baud_union { #[doc = "0x0c - USART Baud Rate"] pub baud_usartfp_mode: self::usart::BAUD_USARTFP_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud_fracfp_mode: self::usart::BAUD_FRACFP_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud_frac_mode: self::usart::BAUD_FRAC_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud: self::usart::BAUD, } ``` Refs: #191 #16 Co-authored-by: Wez Furlong <wez@wezfurlong.org> Co-authored-by: Emil Gardström <emil.gardstrom@gmail.com>
Build failed |
I believe that this computes the correct size for a cluster that contains overlapping registers (such as the ATSAMD21G that I've added to the tests in this commit). It comes at the cost of not recognizing the overlapping registers at that stage.
I want to use this information to emit unions. This commit deals solely with recognizing and tracking the overlaps.
If a RegisterBlock is comprised of a single union, then the RegisterBlock is emitted as that union.
This currently triggers for the atsamd21 for these fields:
```
WARNING: field Some(Ident("pmux1_1")) has different offset 177 than its
union container 176
WARNING: field Some(Ident("pmux1_2")) has different offset 178 than its
union container 176
```
the issue there is that we're missing `derivedFrom` support for
registers, and once implemented, that issue goes away.
Rather than trying to compute a smart name and hoping that there is no conflict, this commit switches to a simpler scheme: we now take the shortest identifier from the components of a union and take the one that sorts earliest as the name for the union in its containing RegisterBlock. So long as the names of the registers in that block don't have collisions, we are guaranteed not to collide.
|
force pushed with the |
|
bors r+ |
192: Emit unions for overlapping/overloaded registers r=Emilgardis a=wez I want to get a complete working build for ATSAMD21 (rust-embedded/wg#61) so I'm taking a stab at that. * Introduced a `FieldRegion` helper to reason about overlapping regions * If overlaps are detected, a `union` container is emitted * If the only item in a `RegisterBlock` is a union, that `RegisterBlock` is emitted as a union * Otherwise: we generate a name for the union field by either taking the shortest common prefix of the union's alternates or the shortest register name (depending on type name conflicts). If that doesn't work just pick an artificial name like `u1`. * If the starting address offset of elements in a union are not all the same, we don't have a way to emit padding for them today. We will emit a warning (and bad code) in that case (example below). The one example of this I see in ATSAMD21 is due to missing `derivedFrom` support for registers; we're currently generating bad code for these anyway. I have resolved in another branch that I'll turn into a PR once this one is landed. ``` WARNING: field Some(Ident("pmux1_1")) has different offset 177 than its union container 176 WARNING: field Some(Ident("pmux1_2")) has different offset 178 than its union container 176 ``` Examples: ``` #[doc = "Real-Time Counter"] pub mod rtc { #[doc = r" Register block"] #[repr(C)] pub union RegisterBlock { #[doc = "0x00 - Clock/Calendar with Alarm"] pub mode2: MODE2, #[doc = "0x00 - 16-bit Counter with Two 16-bit Compares"] pub mode1: MODE1, #[doc = "0x00 - 32-bit Counter with Single 32-bit Compare"] pub mode0: MODE0, } ``` ``` #[doc = r" Register block"] #[repr(C)] pub struct USART { #[doc = "0x00 - USART Control A"] pub ctrla: self::usart::CTRLA, #[doc = "0x04 - USART Control B"] pub ctrlb: self::usart::CTRLB, _reserved2: [u8; 4usize], #[doc = "USART Baud Rate"] pub baud: baud_union, ... } #[doc = "USART Baud Rate"] #[repr(C)] pub union baud_union { #[doc = "0x0c - USART Baud Rate"] pub baud_usartfp_mode: self::usart::BAUD_USARTFP_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud_fracfp_mode: self::usart::BAUD_FRACFP_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud_frac_mode: self::usart::BAUD_FRAC_MODE, #[doc = "0x0c - USART Baud Rate"] pub baud: self::usart::BAUD, } ``` Refs: #191 #16 Co-authored-by: Wez Furlong <wez@wezfurlong.org> Co-authored-by: Emil Gardström <emil.gardstrom@gmail.com>
Timed out |
|
interesting, I'm not sure why bors timed out. Merging as tests have technically been passed |
I want to get a complete working build for ATSAMD21 (rust-embedded/wg#61) so I'm taking a stab at that.
FieldRegionhelper to reason about overlapping regionsunioncontainer is emittedRegisterBlockis a union, thatRegisterBlockis emitted as a unionu1.derivedFromsupport for registers; we're currently generating bad code for these anyway. I have resolved in another branch that I'll turn into a PR once this one is landed.Examples:
Refs: #191
#16