-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove ambiguous 'x' chip suffix #91
Conversation
I would actually go one step further, and make a feature flag for each chip (eg imxrt1062, imxrt1011), internally you can map chip specific feature flags to another feature flag shared among several chips if it makes sense to do so (eg imxrt1060) |
How granular should we make our feature flags and HALs? This goes back to one of our discussions on #56, and it also relates to #89, which names the HAL 1062. Let's take the 1061 and 1062 parts as an example. In #56, we note that they share a datasheet and reference manual. The 1062 has display and camera components, which are missing in the 1061. As proposed, we could put them behind chip-specific HALs, and make our feature flags align. Or, we could create the [package]
name = "imxrt1060-hal"
# ...
[features]
lcd = ["imxrt-common-hal/imxrt1062-lcd"] # Enable the LCD driver
csi = ["imxrt-common-hal/imxrt1062-csi"] # Enable CSI driver
pxp = [] # Enable pixel pipeline (implemented here, not in common HAL) I'm partial to the approach that maps to the datasheet & reference manual, rather than the product number. It makes it easier for us as maintainers, and I don't think we assume much additional documentation overhead. The 1060 data sheet also list parts like IMXRT106A, IMXRT106F, and MXRT106L. Instead of our team releasing three additional crates, and adding three additional feature flags, a user could continue consuming the 1060 HAL and enable the peripheral features that they want. This approach also helps the 1062 user who does not want to compile anything that isn't available across the chip family. I realize I'm talking about HAL names, but the approach ties back to our feature-flag naming conventions. Are there strong reasons to take that one step further, and create granular feature flags and HALs? |
I'm fine with this idea now that I better understand it, sorry I missed that things tied back to the ref manual which now makes perfect sense. I do think we'll need to ensure we tag/describe/README up things so that the supported chips are plastered all over it, since 1060 isn't a chip and people aren't likely to search for it. I think this idea is great, please apply across the crates as you see fit. |
The "x" suffix is inconsistent with the NXP product naming scheme. Since it diverges, it's not clear what chips are supported for the "imxrt101x" feature. The commit updates the imxrt-iomuxc crate to use product naming conventions. The feature flag migration follows - "imxrt106x" => "imxrt1060" - "imxrt101x" => "imxrt1010" This change is reflected in the filesystem, and in all documentation and code updates.
See the previous commit for more details. This commit updates the HAL to use the NXP product identifies, instead of our custom "x" suffix.
The feature-flag renaming is a breaking change. This commit bumps the imxrt-iomuxc commit to 0.2. The commit updates the HAL to use the new version. The HAL now re-exports a 0.2 imxrt-iomuxc API. Although the symbol names exported from the HAL have not changed, the compiler will still see the types as being incompatible. This means that the HAL has a breaking change. I'm relying on other incoming work in the project (#89) to bump the version for me. Or, we hold off on depending on the imxrt-iomuxc 0.2 release, and nothing breaks.
README still refers to the public imxrt-hal, but it points to our tracking issue for a split HAL. I've refined the HAL's README to describe the crate's capabilities. I updated CONTRIBUTING to define what a chip-specific HAL is intended to support.
f610d1c
to
cacb44c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In #56, we identified that the 'x' suffix for chip identifies is ambiguous. We've used the 'x' suffixes in feature flags, and in naming conventions for split HALs. However, the approach doesn't correlate to NXP product specifications, and we haven't documented what the 'x' means. For example, it's not clear if
"imxrt101x"
describes support for both 1011 and 1015. (It doesn't.)The PR updates the the imxrt-iomuxc crate to use product naming conventions. The goal is to make it clear what support we're providing for a given API, feature flag, or split HAL crate. The migration follows
"imxrt106x"
=>"imxrt1060"
"imxrt101x"
=>"imxrt1010"
This change is reflected in the filesystem, and in all documentation and code updates.
The feature-flag renaming is a breaking change. This commit bumps the
imxrt-iomuxc
crate to 0.2. The commit updates the HAL to use the new version.The HAL now re-exports a 0.2 imxrt-iomuxc API. Although the symbol names exported from the HAL have not changed, the compiler will still see the types as incompatible. This means that the HAL has a breaking change. I'm relying on other work in the project to bump that version for me.