Closed teburd closed 4 years ago
Sorry, but this is one, very large commit! I think we only changed an 8 to a 5 in the imxrtral.py
script, then re-generated the RAL. Is that all? If there's more, consider separating the commits for easier review.
From my quick evaluation:
v2
. It's not entirely obvious that usb_analog_v2.rs
corresponds to the 1050 series, but we can find that in the module's comments.1051_1052_1061_1062_1064
.The instance consolidation looks nifty. The RAL continues to re-export instances under their sane names. As a HAL designer, when I write something like
use ral::usbphy::Instance;
it's not immediately clear that the Instance
comes from the usbphy_1011_1015_1021
module instead of the usbphy_1051_1052_1061_1062_1064
module. But, since I know how the RAL works, I can set up conditional compilations that group these chips together, just as they're grouped together by module names. That's how we're expected to create the common HAL, correct?
Is there anything specific you'd like feedback on?
I'm looking at how the RAL script generates the LPUART instances. Here's what it looks like:
$ cd imxrt-ral
$ find . -iname "*lpuart.rs"
./src/imxrt/imxrt1015/lpuart.rs
./src/imxrt/instances/lpuart.rs
./src/imxrt/imxrt1011/lpuart.rs
./src/imxrt/imxrt1021/lpuart.rs
It looks like it's putting the "most general" instances together in instances
. Then, it's separating the 1011, 1015, and 1021 instances out because it's identified them as unique.
I'd be curious to see if the 1021 could be part of that "most general" grouping, or if there's a strong reason to keep it separate. And, I'm curous to see if there are dramatic differences between 1011 and 1015.
From a previous, cursory reference manual study, the 1011 and 1015 only have 4 instances; every other variant has 8. When comparing the 1011 to the 1062, I did not notice a difference in register block layout or fields. The auto-generated code might confirm that; both the 1011 and "most general" grouping reference the lpuart_v1
register block.
This might be due to the buggy SVDs...
Yes, the only thing I've changed is in python the family prefix match from :8 to :5 (imxrt), the patch is huge I agree. Mostly to the positive side (lots of removed code).
Some of the peripheral variants (v1/v2/etc) are probably easily unified with some small svd patching and I think that's a worthwhile goal if this is accepted.
Peripheral drivers (the iomuxc, upcoming usb, common hal) can group the feature flag compiles together based on what peripheral type is support by which chips.
IE: in your above example, a future imxrt-usb driver may expose all chips as feature flags, then internally map those to imxrt1062 means use v2 of the register block. The fact that the ral now produces two variants of some peripheral register blocks makes it I think easier to understand which chip uses which version, as its easy to see in the comments.
The thing that keeps this from being feature additive crate is the pub use lines in lib.rs that imxrtral.py generates for us, I'm not super inclined to change that yet, I think that would require some additional changes to how imxrtral.py generates code.
You could ignore the feature flags and directly use the chip specific Instance static's, but that's probably more of hassle than a help.
This is a simple change, I plan on iterating on this idea more after we move it out of this repo, perhaps with an effort to port the whole thing to rust
Looking for feedback on the results of this primarily.
I'm amazed the checks all passed.