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

Async sensor API #2

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Async sensor API #2

wants to merge 2 commits into from

Conversation

tazz4843
Copy link

Currently untested, figured I would make the PR now to get some sort of feedback before I move any further. This is by far the easiest way of implementing it, with the mutually exclusive features. Having blocking enabled by default allows this to be a patch release if so desired.

@tazz4843 tazz4843 mentioned this pull request Oct 28, 2023
@tazz4843
Copy link
Author

tazz4843 commented Nov 9, 2023

If you're okay with this implementation I'll go ahead and fix CI and prepare for merge.

@sirhcel
Copy link
Owner

sirhcel commented Nov 10, 2023

Thank you for your proposal! I bet I got the idea and your implementation looks straightforward at a first glance.

Looking at the CI builds, the error

 error[E0554]: `#![feature]` may not be used on the stable release channel
Error:  --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embedded-hal-async-1.0.0-rc.1/src/lib.rs:4:1
  |
4 | #![feature(async_fn_in_trait, impl_trait_projections)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   Compiling quote v1.0.33
For more information about this error, try `rustc --explain E0554`.

looks like the dependency embedded-hal-async could not be build on stable. I would have expected this optional dependency not to be built at all in the actual CI configuration.

Are you building with nightly? Have you encountered this issue and already looked into it?

@tazz4843
Copy link
Author

There's a all-features target in the CI at https://github.com/sirhcel/sht4x/blob/master/.github/workflows/ci.yml#L23-L26 for each one except rustfmt. This would have to be removed and two more branches added for blocking and async features, plus a test on nightly given that embedded-hal-async doesn't function on stable at all.

@sirhcel
Copy link
Owner

sirhcel commented Feb 24, 2024

Please excuse me for my sluggish reply. Thank you for pointing me to the CI! I managed to setup a project for giving it a spin and your changes work like a treat. :)

After some discussion about how to handle a sync and a async version of a crate, I'm currently inclined to release the sync and async variant as separate crates. My argument for this way is that you need either one or the other and this also will result in clean documentation on docs.rs. Since you created this PR, what's your opinion on thes points?

Providing async support through a feature flag does not feel like the first-class citizen it should be. At least with respect to documentation.

@bbustin
Copy link

bbustin commented Mar 13, 2024

I was looking for an async sht4x library. @tazz4843, do you think you'll be publishing this as a different crate?

For now I am just going to set the dependency as your github repo...

@bbustin
Copy link

bbustin commented Mar 13, 2024

To use it, need to set default-features to false. This might be a little confusing to new users of the library. Maybe it would be better not to have any default features selected. Then force the user to select async or blocking?

sht4x = { git = "https://github.com/tazz4843/sht4x-hal", default-features = false, features = [
    "async",
] }

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