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-async support #19

Merged
merged 5 commits into from
Jul 21, 2024
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jun 8, 2024

This commit adds support for the embedded-hal-async crate in addition
to embedded-hal. I've done this by adding a separate AsyncSgp30
type, based on the assumption that most projects won't need to use both
the blocking embedded-hal traits and the embedded-hal-async traits
at the same time, and providing async fn methods on a separate type
with the same names as the blocking ones seemed a bit nicer than having
one type that has both fn measure and async fn measure_async and so
on. I've also factored out some of the no-IO code for packing and
unpacking Rust to/from bytes, so that it can be shared by both the async
and blocking driver types.

Support for embedded-hal-async is gated behind the
embedded-hal-async feature flag, so the dependency is not enabled by
default.

Note that this branch depends on my PR #18, which updates this crate to
use embedded-hal v1.0, and currently contains the commit from that
change as well. Once #18 has merged, this branch will need to be rebased
onto the main branch.

It also depends on my upstream PR adding embedded-hal-async support to
sensirion-i2c-rs, Sensirion/sensirion-i2c-rs#30, which has been
merged, but hasn't been published to crates.io yet. Currently, this
branch adds a Cargo [patch] to use a Git dep on sensirion-i2c-rs.
So, this change cannot be released to crates.io until upstream publishes
a new release of sensirion-i2c-rs. Hopefully they do that soon! :)

@dbrgn
Copy link
Owner

dbrgn commented Jun 30, 2024

Now that #18 is merged, I'll go ahead and will rebase this branch on top of the current main branch.

Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this looks nice!

I generally agree about the choices regarding a separate struct with methods of the same name. However, just to explore the available options: A third approach would be keeping the same shared Sgp30 struct, but implementing both sync and async methods through two separate traits. One (and only one) of them would need to be imported to bring in the desired methods. The separate struct is probably better for ease-of use though. What are your thoughts on that?

Another thing I don't really like is how close-but-not-the-same the implementations are. I guess that's just how colored functions work in Rust. However, the commends are identical copies. Are you aware of a way to "inherit" docs of another method (maybe using a macro)?

README.md Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/async_impl.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Contributor Author

hawkw commented Jul 20, 2024

@dbrgn I believe I've addressed all your feedback, thanks for the review!

@dbrgn
Copy link
Owner

dbrgn commented Jul 21, 2024

Perfect, thanks! I'll rebase your branch on top of main to get a working CI run.

This is now good to merge, but I'd still be interested in your thoughts on #19 (review)

@dbrgn
Copy link
Owner

dbrgn commented Jul 21, 2024

Ah, seems like we need to bump the MSRV. I'll do that in a separate PR.

hawkw and others added 5 commits July 21, 2024 15:09
This commit adds support for the `embedded-hal-async` crate in addition
to `embedded-hal`. I've done this by adding a separate `AsyncSgp30`
type, based on the assumption that most projects won't need to use both
the blocking `embedded-hal` traits and the `embedded-hal-async` traits
at the same time, and providing `async fn` methods on a separate type
with the same names as the blocking ones seemed a bit nicer than having
one type that has both `fn measure` and `async fn measure_async` and so
on. I've also factored out some of the no-IO code for packing and
unpacking Rust to/from bytes, so that it can be shared by both the async
and blocking driver types.

Support for `embedded-hal-async` is gated behind the
`embedded-hal-async` feature flag, so the dependency is not enabled by
default.

Note that this branch depends on my PR dbrgn#18, which updates this crate to
use `embedded-hal` v1.0.

It also depends on my upstream PR adding `embedded-hal-async` support to
`sensirion-i2c-rs`, Sensirion/sensirion-i2c-rs#30, which has been
[merged], but hasn't been published to crates.io yet. Currently, this
branch adds a Cargo `[patch]` to use a Git dep on `sensirion-i2c-rs`.
So, this change cannot be released to crates.io until upstream publishes
a new release of `sensirion-i2c-rs`. Hopefully they do that soon! :)

[merged]: Sensirion/sensirion-i2c-rs@f7b9f3a
@hawkw
Copy link
Contributor Author

hawkw commented Jul 21, 2024

Ah, seems like we need to bump the MSRV. I'll do that in a separate PR.

Ah, whoops, my bad, I didn't notice that --- what change requires the MSRV bump?

@hawkw
Copy link
Contributor Author

hawkw commented Jul 21, 2024

I generally agree about the choices regarding a separate struct with methods of the same name. However, just to explore the available options: A third approach would be keeping the same shared Sgp30 struct, but implementing both sync and async methods through two separate traits. One (and only one) of them would need to be imported to bring in the desired methods. The separate struct is probably better for ease-of use though. What are your thoughts on that?

Personally, I don't love APIs where the core functionality is only present on a trait implementation that is only implemented by one type. I find it a bit of an annoying speed bump for a user to have to import both the type and the trait, and it's kind of easy for a new user to read an example and miss that it also requires a trait being implemented. The fact that you can't use recv() on the futures crate's MPSC channel without also importing StreamExt is an example of this kind of annoyance. With that said, I do think this option would work. I'm not sure if it makes things any better for the user than just having two types, though --- they still have to import one of two different names to indicate whether they want the async or blocking API, and since almost all the methods on the type do I2C transactions, there aren't really any methods that could be shared besides new and destroy AFAICT.

Another thing I don't really like is how close-but-not-the-same the implementations are. I guess that's just how colored functions work in Rust. However, the commends are identical copies. Are you aware of a way to "inherit" docs of another method (maybe using a macro)?

I also don't love the code duplication --- I've tried to, at least, factor out all the shared code that's more complex than just "send this command" into free functions that both versions can use. Regarding docs, I think it would theoretically be possible to have a macro that generates both implementations, allowing the doc comment to only be written out once...but it would be quite complex, as it would need some way to know where the .awaits go in the async version but elide them from the sync version. I didn't think that would be worth the complexity. I'm willing to look into it if you think it would be better, though!

@dbrgn
Copy link
Owner

dbrgn commented Jul 21, 2024

Ah, seems like we need to bump the MSRV. I'll do that in a separate PR.

Ah, whoops, my bad, I didn't notice that --- what change requires the MSRV bump?

The embedded-hal-async crate: https://github.com/dbrgn/sgp30-rs/actions/runs/10028497153/job/27715481332

I didn't think that would be worth the complexity. I'm willing to look into it if you think it would be better, though!

Ah, examples would need to be different, true. But for cases where comments are 100% identical, such a macro might be useful.

In any case, I think this change is now ready to be merged! Thanks for your contribution.

@dbrgn dbrgn merged commit abeb72c into dbrgn:main Jul 21, 2024
5 checks passed
@dbrgn
Copy link
Owner

dbrgn commented Jul 21, 2024

@hawkw I published version 1.0.0-rc.1 of this crate to crates.io. Testing in a real-world project (especially the async part) would be welcome! (I'll upgrade my non-async projects to this version as well.)

@hawkw
Copy link
Contributor Author

hawkw commented Jul 21, 2024

@hawkw I published version 1.0.0-rc.1 of this crate to crates.io. Testing in a real-world project (especially the async part) would be welcome! (I'll upgrade my non-async projects to this version as well.)

I've been using the async code in a personal hobby project via a Git dep since I opened this branch, if that's helpful. I'm not using it in a commercial product, so there's maybe not quite the same level of rigor... I'll definitely switch from the Git dep to the crates.io version and let you know if anything has suddenly stopped working!

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