Skip to content

Conversation

onestacked
Copy link
Contributor

@onestacked onestacked commented May 5, 2024

For now it is enabled by default, but could probably be disabled in the next major version.

@onestacked onestacked changed the title Make embedded-hal v0.2.0 optional for embassy-time and embassy-embedded-hal Make embedded-hal v0.2 optional for embassy-time and embassy-embedded-hal May 5, 2024
@Dirbaio
Copy link
Member

Dirbaio commented May 5, 2024

We shouldn't make e-h0.2 optional because

  • the cost of making it not-optional is quite small. e-h0.2 is tiny, compiles very fast, and adds no bloat to the binary if not used.
  • there's many drivers out there still using eh0.2. Disabling it makes them not work, which is extra complexity for the users in something that's already complex. Users already struggle with eh0.2/eh1.0 version mismatches, it'd be worse if they also had to take into account Cargo features.
  • We alreadyu have way too many features, each feature increases the complexity of testing (you have to ensure the crate builds with the feature enabled and disabled, multiplied by all the combinations of all other features).

@onestacked
Copy link
Contributor Author

I just find it a bit annoying to have multiple versions of the same package, but I guess it doesn't really matter so I'll close this PR.

@onestacked onestacked closed this May 5, 2024
@ROMemories
Copy link
Contributor

@Dirbaio Now that it is not recommended for drivers to implement embedded-hal v0.2 traits, would you consider a new PR that updates the embassy-embedded-hal crate to only require the embedded-hal v1.0 traits?

I believe currently the BlockingAsync type actually adapts from embedded-hal v0.2 blocking traits to embedded-hal-async v1.0 async traits. I think that makes it unusable on drivers that do not implement the v0.2 traits (e.g., esp-hal has dropped them recently).

I suppose that drivers that only implement v0.2 traits could still be used through embedded-hal-compat.

@onestacked
Copy link
Contributor Author

Sure I can take a look at rebasing this and reopening this PR (or open a new one if that's preferred).

Should the v02 traits be enabled by default?

@Dirbaio
Copy link
Member

Dirbaio commented Feb 5, 2025

I believe currently the BlockingAsync type actually adapts from embedded-hal v0.2 blocking traits to embedded-hal-async v1.0 async traits.

Yes, changing BlockingAsync to adapt from 1.0 blocking to 1.0 async sounds good to me.

I don't think we should drop 0.2 impls elsewhere (or even make them optional with a Cargo feature), they don't hurt users of 1.0.

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.

3 participants