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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add async support #175

wants to merge 5 commits into from

Conversation

vhdirk
Copy link
Contributor

@vhdirk vhdirk commented Nov 6, 2023

Hi,

I'm adding support for async runtimes this driver. My project uses embassy so this is quite the necessity for me.
It doesn't seem all that difficult from the get go; just a lot of work. Mainly, the idea is to use the SpiDevice type from embedded-hal-async and make a bunch of methods async.

Before I start with the grunt of the work, I'd like some opinions on code structuring. The options that I currently see are:

keep the code structure mostly as is, replacing sync versions with async where needed

This would look something like the following:

#[cfg(not(feature="async"))]
impl<SPI, BUSY, DC, RST, DELAY> Epd1in54<SPI, BUSY, DC, RST, DELAY>
where
    SPI: embedded_hal::spi::SpiDevice,
    BUSY: InputPin,
    DC: OutputPin,
    RST: OutputPin,
    DELAY: DelayUs,
{
}
#[cfg(feature="async")]
impl<SPI, BUSY, DC, RST, DELAY> Epd1in54<SPI, BUSY, DC, RST, DELAY>
where
    SPI: embedded_hal_async::spi::SpiDevice,
    BUSY: InputPin,
    DC: OutputPin,
    RST: OutputPin,
    DELAY: DelayUs,
{
}

The drawback here is that sync and async cannot co-exist. I don't believe that too big of an issue since it would be unlikely to need both versions at the same time. However; I'm not sure how cargo test --all-features would behave.

For each module, create a nested async module

Inspired by this: https://github.com/esp-rs/esp-wifi/blob/a20545ca8f8463192cb84aeba573d8e68e144cc9/esp-wifi/src/wifi/mod.rs#L1535

I would still have to duplicate all common traits, too. The drawback here is that there will be even more code duplication than option 1 since ever type that is generic over SpiDevice would need an async version.

Fork this repo and keep it wholly separated

The pinnacle of code duplication, all for the purpose of clarity.

Make everything async and provide a blocking wrapper

This is how https://docs.rs/reqwest/latest/reqwest/ does it


Unfortunately, any approach that I can think of will involve a fair amount of code duplication.
Do you see any more options? Which would have your preference?

@auto-assign auto-assign bot requested a review from caemor November 6, 2023 07:40
@caemor
Copy link
Owner

caemor commented Jan 11, 2024

Thanks for your work!
Do you know if there is already an async blocking wrapper for an embedded driver?
I think I would prefer option 4 or option 1, if 4 is more difficult or different than I would expect.
2 and 3 sound like too much duplication and therefore later code divergence to me.
What is your favorite?

@vhdirk
Copy link
Contributor Author

vhdirk commented Jan 12, 2024

I think option 1 would be best, even though 4 makes way more sense from a developer point of view.
Thing is that async has more stack space requirements than sync. IIRC, that was mentioned to me by https://github.com/Dirbaio.
Therefore, I think 4 will come with its own issues which not a lot of users may expect.

That being said, my project https://github.com/vhdirk/papertrain is pretty much done, so I won't spend much time on this driver anymore. Feel free to take whatever you need from my async fork and papertrain, though.

@caemor
Copy link
Owner

caemor commented Jan 12, 2024

Thanks 👍

@newAM newAM mentioned this pull request Jan 22, 2024
@avsaase
Copy link

avsaase commented May 29, 2024

In case this is still being worked on, another option would be to use a crate like maybe-async to avoid duplicate the whole api.

@vhdirk
Copy link
Contributor Author

vhdirk commented May 29, 2024

Yes, I've picked this up again last week. I've been rebasing on master, but I have yet to complete and push it. It might take me a while, I'm quite busy at the moment.

@vhdirk vhdirk changed the title WIP: add async support Add async support May 30, 2024
@vhdirk vhdirk marked this pull request as draft May 30, 2024 06:56
@kaaax0815
Copy link

whats the status on this?

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.

4 participants