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

Add embedded-hal-bus atomic-device features #607

Merged
merged 12 commits into from
Jul 28, 2024

Conversation

9names
Copy link
Contributor

@9names 9names commented Jun 8, 2024

Add atomic-device and portable-atomic features as described in this comment
The tasks from the comment are copied below for convenience

  • Add a new atomic-device feature, default off
  • Only enable AtomicDevice when either the atomic-device feature is enabled or cfg(target_has_atomic...) is set, so it's by default available on platforms with CAS
  • Add new portable-atomic-critical-section and portable-atomic-unsafe-assume-single-core features which proxy to the corresponding portable-atomic features (so users don't have to depend on portable-atomic directly) and enable our atomic-device feature
  • Document these new features
  • Release as 0.3

Note that the entire util.rs module is only ever used by AtomicDevice and the i2c/spi atomic impls, so we could sensibly rename it to atomic_util.rs and gate that that file instead of everything defined inside it.

Note: untested at this point. Will add comment once testing is done.

Resolves #598

@9names 9names requested a review from a team as a code owner June 8, 2024 12:16
@9names
Copy link
Contributor Author

9names commented Jun 8, 2024

CI errors, because cargo clippy --all-features is definitely not compatible with these new, mutually incompatible features

@Dirbaio
Copy link
Member

Dirbaio commented Jul 26, 2024

perhaps we can instead do the following:

  • Add a feature portable-atomic
  • Document that if you enable it, you have to add a dep on portable-atomic and enable one of the two polyfill mode features on it.
  • Enable AtomicDevice if any(feature = "portable-atomic", target_has_atomic = "8")

Advantages

  • Generalizes better if we add more things requiring atomics in the future (the portable-atomic feature would enable all "things" requiring atomics, if we name the feature atomic-device we'd need one per "thing")
  • Generalizes better if portable-atomic adds more polyfill modes in the future.
  • Avoids non-additive features in embedded-hal-bus.

@9names
Copy link
Contributor Author

9names commented Jul 27, 2024

Certainly looks cleaner @Dirbaio. I've also added require-cas to the enabled features of portable-atomic as per the docs, so that users that fail to add an appropriate polyfill feature on portable-atomic will get a link error:
image

@@ -26,7 +36,7 @@ embedded-hal = { version = "1.0.0", path = "../embedded-hal" }
embedded-hal-async = { version = "1.0.0", path = "../embedded-hal-async", optional = true }
critical-section = { version = "1.0" }
defmt-03 = { package = "defmt", version = "0.3", optional = true }
portable-atomic = {version = "1", default-features = false}
portable-atomic = {version = "1.3", default-features = false, features = ["require-cas"]}
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this need optional = true? otherwise thumbv6 build will fail due to require-cas being enabled if the user doesn't enable any of the features for portable-atomic

Copy link
Contributor Author

@9names 9names Jul 28, 2024

Choose a reason for hiding this comment

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

You are correct.
I've now made portable-atomic optional and done the cleanup required to handle building without it enabled.

I've tested the following on a thumbv6 target:

  • feature disabled - build success, AtomicCell, {spi,i2c}::AtomicDevice not present in docs
  • feature enabled, no polyfill set - link error shown above
  • feature enabled, portable-atomic/unsafe-assume-single-core - build success, AtomicCell, {spi,i2c}::AtomicDevice present in docs

@9names 9names force-pushed the embedded-hal-bus-atomic-device branch from 4eb0ee6 to b556544 Compare July 28, 2024 00:20
@9names 9names force-pushed the embedded-hal-bus-atomic-device branch from b556544 to 2acbd77 Compare July 28, 2024 00:20
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thank you!

@Dirbaio Dirbaio added this pull request to the merge queue Jul 28, 2024
Merged via the queue into rust-embedded:master with commit 987dc68 Jul 28, 2024
8 checks passed
@9names 9names deleted the embedded-hal-bus-atomic-device branch July 29, 2024 08:37
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.

embedded-hal-bus no longer builds with target thumbv6m-none-eabi
2 participants