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 support for embedded_hal_async::digital::Wait #91

Merged
merged 1 commit into from
May 30, 2024

Conversation

cdunster
Copy link
Contributor

This PR adds support for the async Wait trait from the embedded-hal-async crate.

@cdunster cdunster force-pushed the feat/async-pin-wait branch 2 times, most recently from ac8a974 to 122553a Compare September 18, 2023 13:20
@dbrgn
Copy link
Owner

dbrgn commented Nov 23, 2023

@cdunster hi, there are new conflicts introduced by other merges. Would you mind updating your branch? Thanks.

@cdunster
Copy link
Contributor Author

Hi @dbrgn, I've updated my branch. Thanks for looking into this PR.

@avsaase
Copy link

avsaase commented Dec 1, 2023

I gave this PR a try and it works well. Would be possible to add a configurable delay before these async functions resolve? In my code I use timeouts for control flow so I need a way to make these futures not resolve immediately.

EDIT: This is what I had in mind: 7f69d31.

@cdunster
Copy link
Contributor Author

cdunster commented Dec 6, 2023

Hi @avsaase, that looks good to me. I would prefer to make the delay an Option<Duration> so that we could keep wait_for_state and wait_for_edge for use without a delay and then add wait_for_state_with_delay for use with the delay.

The only "problem" I see is that this makes tokio an official dependency of this crate (instead of just a dev-dependency) I don't personally have an issue with this but I'm not sure if it aligns with @dbrgn's goals for this crate? Maybe we could put it behind a feature flag to make it opt-in but I think that's up to @dbrgn.

@dbrgn
Copy link
Owner

dbrgn commented May 30, 2024

@cdunster looking great, thanks!

Regarding the configurable delay: If such a delay would be added, is that something that should be added to other async mock method calls as well, not just digital pins?

On the other hand, this method is explicitly about waiting, so a configurable wait time might make sense. However, the point about the tokio dependency is a valid one. Ideally, we'd have an executor-independent async sleep implementation, but I think that's not currently possible in Rust, right?

In any case, I'll rebase and merge this PR for now. I'll tag a new release soon, but more features (even breaking changes) can always be added. If you're interested in a configurable delay, feel free to open a new PR, where it can be discussed.

Edit: One downside of adding tokio as a dependency would be that it's harder to keep the MSRV. Tokio has a rolling MSRV, we currently have a fixed one.

@dbrgn dbrgn merged commit 989059f into dbrgn:main May 30, 2024
3 checks passed
@cdunster cdunster deleted the feat/async-pin-wait branch June 3, 2024 08:38
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