Skip to content

device feature gates #261

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

Closed
wants to merge 12 commits into from

Conversation

Crzyrndm
Copy link
Contributor

@Crzyrndm Crzyrndm commented Oct 16, 2021

#29
This PR introduces device specific cargo features, renaming the old L4 line features to be clear these are private/internal groupings and adding internal groupings for the odd/even product sets, AES support and SHA-256 support. Once features are updated to the specific device instead of line in a downstream crate (i.e. change stm32L4x2 to stm32l432 in the features list of this crate), no functional changes should occur due to this PR. The purpose of this PR is to give a clear path to contributing device specific changes (prompted by #257 & #258. There was no clear path to merging a change that was specific to L47/L48 products. #29 makes it sound like you want the complete set of changes for a part all at once which is way more than I set out to do when debugging why the temperature readings were off and so led to that fix not making it into the PR)

Note: Since the L4 family is split across 3 reference manuals (L41-L46, L47-L4A, L4+), a combination of checking feature tables on the ST website (example) and searching for defined(stm / defined (stm in the C HAL seems likely to be the most productive way to find the majority of the differences

@Crzyrndm
Copy link
Contributor Author

Crzyrndm commented Oct 17, 2021

Reconsidering b2abfcb and the private naming for the PAC features as that would then make someone with an un / indirectly supported MCU (i.e. L4+) have to do something obviously wrong (a completely different MCU in the features list)

  • Maybe change PAC feature naming generic_pac_* instead of private_pac_* and add those to the list of potential required feature selections. The name change would still lead to (easily fixed) compiler errors so people aren't silently using the generic feature when they don't have to.
    • Should probably add a compile error for someone doing something like --features="stm32l432 generic_pac_stm32l4x6". Something somewhere will likely conflict, but on the off chance...
  • Another option (not exclusive to ^^) would probably be to uncomment the L4+ series features and have them activate the closest PAC feature-set for now (assuming this crate and/or the L4 PAC intends to add support for L4+ at some time). I'm not sure which set is closest to the 7/9 though. This would give most dependants a one time compile error with easy fix, with better support added over time (as opposed to a compiler error pointing to generic_pac_* or having to set the incorrect MCU and then not getting any compiler prompt if/when better support is added)

This allows for PAC specialisation (like is done for L412 and L4r9) AND
grouping in source. The `generic_pac_*` features now enable both the
private and dependency features
@Crzyrndm
Copy link
Contributor Author

Crzyrndm commented Oct 17, 2021

Added generic_pac_* features. Decided to leave the private features because it allowed for PAC specialisation (generic now activates the private and PAC features. At a later time, specific devices can skip generic for a specialised PAC and keep the private_line_* feature if appropriate for making crate internal decisions)

Note that while the PAC has specilisations for the L412, that is not enabled by this PR as it does require functional changes. The intended diff would be

- stm32l412 = [ "generic_pac_stm32l4x2", "private_product_L41_L42" ]
+ stm32l412 = [ "stm32l4/stm32l412", "private_line_stm32l4x2", "private_product_L41_L42" ]

@korken89
Copy link
Collaborator

Wow, it turned out that this can of worms was a lot larger than I expected.
Generally I'd not like the HAL to have this many features, it will get quite difficult to maintain in the future.

Do you see any other way of achieving this? Like base feature + extension only available on MCU xxx.

@Crzyrndm
Copy link
Contributor Author

Crzyrndm commented Oct 24, 2021

I must say I agree that this is ugly, but at the same time I don't see a lot of good options here. From what I've looked at Rust doesn't really provide a way to shorthand stm32l434 into L43x/L43_L44 and L4x4 and those are the lines down which almost everything is divided (:/) leaving you with:

  • derived features as seen here
  • listing out all of the possible options in code as in the C HAL (sounds like a recipe for subtle typos to me)
  • user specify features L43x and L4x4 rather than stm32l434 (I can't see anyone being appreciative, and a specialised PAC would be very difficult)
  • ??

The idea here was that a crate depending on this one only uses the chip list. Internally only the private_* features are used except in exceptional cases (e.g. errata, should probably add a comment to that end somewhere...).

I have quite deliberately proposed the "over the top" solution here because I see it having the least impact outside this crate (1 time very easy to resolve feature update) while minimising opportunity for errors internally and making it easy to contribute changes (could probably do with some docs describing the recommended approach to take with gated code as well. I don't particularly like what happened with some of the RTC changes and a single recommended style would be useful once one is established)

I'm very open to suggestions/discussion on alternative approaches (not tied to this at all, just need to start somewhere).

PS
the generic_* features can definitely be removed. That was probably a step too far...

@Crzyrndm
Copy link
Contributor Author

#266 as a counterpoint to the heavy use of derived features here. Unlike this PR it does make some functionality changes (took the time to act on comments along the lines of "this should be gated on...")

@DrTobe DrTobe mentioned this pull request Nov 18, 2021
@Crzyrndm Crzyrndm closed this Nov 18, 2021
@Crzyrndm Crzyrndm deleted the #29-device-feature-gates branch December 16, 2021 01:33
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