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

Implement async Wait traits from embedded-hal-async #218

Merged
merged 12 commits into from
Mar 23, 2023

Conversation

dberlin
Copy link
Contributor

@dberlin dberlin commented Mar 17, 2023

This patch implements the embedded-hal-async Wait trait, which enables
waiting on an level/edge for pins in an async fashion.

It has been tested on multiple esp32s3 devices in production

@dberlin
Copy link
Contributor Author

dberlin commented Mar 17, 2023

Not sure why cargo fmt errors out on the trailing whitespace in the allow, but i fixed it.

I also have a version of this that uses AtomicWaker instead of RefCell<Optional>, which is simpler, and doesn't require disabling interrupts to set the waker.
This is how basically all the other HAL's you will find do it - some form of atomic waker storage.

It is also Sync, and the current solution is only Send because of RefCell.

The downside is it requires the futures crate (or we have to copy the impl or find another).

I would guess as more and more async is implemented, the likelihood you need futures at some point anyway is high.
You can also feature disable basically all of it (we only need async-io feature).

But it's your show :)

Let me know if you want me to update this to the AtomicWaker version or not.

.

Copy link
Member

@MabezDev MabezDev 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! I have a few comments I think that should be addressed before we merge this :).

src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio/wait_async.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/gpio/wait_asynch.rs Outdated Show resolved Hide resolved
src/gpio/wait_asynch.rs Outdated Show resolved Hide resolved
src/gpio/wait_asynch.rs Outdated Show resolved Hide resolved
src/gpio/wait_asynch.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@dberlin
Copy link
Contributor Author

dberlin commented Mar 18, 2023

I've updated to use notifications.

If you make one function in channel-bridge's Notification class public (poll_wait) we can reuse the crate.
It's actually the only one we need, so right now, i've copied your implementation and made that function accessible.

Let me know if you want me to make it not do anything until the first poll.

Copy link
Member

@MabezDev MabezDev 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 this! Looks good to me :)

src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
src/gpio.rs Outdated Show resolved Hide resolved
@ivmarkov
Copy link
Collaborator

@MabezDev This PR is very close to completion but still needs a few final small touches before merging. Given that the submitter might have moved after our strong disagreement what to do on pin drop I might be looking into it, but it will take some time. In any case, no rush - if you have time these days, you can of course also address my last set of comments.

@MabezDev MabezDev force-pushed the async branch 4 times, most recently from 4515f7c to ab38c8e Compare March 23, 2023 16:57
@MabezDev
Copy link
Member

I think I've addressed everything in the latest commit.

@ivmarkov
Copy link
Collaborator

Let's push it then?

@ivmarkov ivmarkov merged commit cbb27de into esp-rs:master Mar 23, 2023
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