Skip to content
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

Merged
merged 8 commits into from
Oct 25, 2020
Merged

Remove ambiguous 'x' chip suffix #91

merged 8 commits into from
Oct 25, 2020

Conversation

mciantyre
Copy link
Member

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.

@mciantyre mciantyre requested a review from teburd October 17, 2020 14:10
@teburd
Copy link
Member

teburd commented Oct 18, 2020

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)

@mciantyre
Copy link
Member Author

mciantyre commented Oct 18, 2020

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 imxrt1060-hal, with feature flags for the optional components. These would enable corresponding features in the common HAL, or would enable the implementation directly in the imxrt1060-hal:

[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?

@teburd
Copy link
Member

teburd commented Oct 19, 2020

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.
@mciantyre mciantyre merged commit 91606d8 into master Oct 25, 2020
@mciantyre mciantyre deleted the clear-chip-ids branch October 25, 2020 20:01
Copy link
Member

@teburd teburd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants