rust-embedded-community
Add maybe_async
I'm using embassy, and the other repo I found (embedded-fatfs) is not supporting partition tables on the SD card.
Here is my proposal to add maybe_async to this repo, so it can be used "as-is", or asynchronously.
I also needed to make BlockDevice mutable to support DMA on STM32.
All comments are welcomed.
It might be worth looking at #148 where I put a BlockCache in a refcell. That might help you have a mutable block device without making all the &self methods back to &mut self (which forces you to use raw handles if you want to open multiple files at once).
So for this to fly, I would need the examples to build-as is, so that we know we're not breaking existing code (at least more than I have already broken it). But perhaps also an async example would be useful. And it has to build on stable.
That's fair, I'll work on that
Otherwise, I know there is a lot of debate around maybe_async, and I'd understand if this PR is not the way forward for this library. What's your thoughts on this?
I'll judge it by the outcome. If the examples build unchanged on stable (and it works on no_std) then it should continue to work for my use case and I can support the changes.
~Run workflow?~ build-test is successful, formatting is creating lots of diffs
We'll want to find a way to do a build in both async mode and non-async-mode. I think you'll need to mark the existing examples as requiring the is_sync feature, and make a new async example which only builds when that isn't set? Actually I don't even know if that's possible. The async example might have to be in a crate all of its own.
Run workflow?
As this PR is not from an org member, one of us has to manually approve the workflow run every time - in case you submit a change which copies a Github secret key to pastebin or something. Sorry about that - bear with us. Alternatively you can look at the workflow files and get a good idea about what you need to run locally to replicate the CI.
formatting is creating lots of diffs
We just test for cargo fmt --check so you probably just need to run cargo fmt locally.
Expected — Waiting for status to be reported
unfortunately it looks like adding the features to the matrix has renamed the build jobs, and so now the branch protection rule is waiting for a job that doesn't exist. I guess I'll have to manually override that check when this is ready to go, and then update the branch protection rule.
GHA is such a pain to use.
Thanks for the information! For a moment I thought the workflows were triggered by commenting here...
I'll try to put the is_sync feature outside the matrix, that should resolve the renaming of workflow
I just pushed the result of cargo fmt
I had a rustfmt.toml file in the parent directory, limiting the line length to 120. I removed it and it fixed the issue
We'll want to find a way to do a build in both async mode and non-async-mode. I think you'll need to mark the existing examples as requiring the
is_syncfeature, and make a new async example which only builds when that isn't set? Actually I don't even know if that's possible. The async example might have to be in a crate all of its own.
IMO, this is the biggest issue with using the maybe_async crate. It breaks cargo's rules about features being additive. I don't think that it would be inconceivable for a user to need both blocking and non-blocking or one crate to use blocking (say for log flushing which may have a blocking api) and another non-blocking. Ideally the macro would generate separately named functions so that crates can play nicely with each other but alas it doesn't.
We'll want to find a way to do a build in both async mode and non-async-mode. I think you'll need to mark the existing examples as requiring the
is_syncfeature, and make a new async example which only builds when that isn't set? Actually I don't even know if that's possible. The async example might have to be in a crate all of its own.IMO, this is the biggest issue with using the
maybe_asynccrate. It breaks cargo's rules about features being additive. I don't think that it would be inconceivable for a user to need both blocking and non-blocking or one crate to use blocking (say for log flushing which may have a blocking api) and another non-blocking. Ideally the macro would generate separately named functions so that crates can play nicely with each other but alas it doesn't.
I've found a workaround in simply not using the is-sync feature of the maybe-async crate at all, and then also using the duplicate crate which allows you to re-export a single, templatized module twice:
- once, as blocking
- one more time, as async
... so you are effectively ending up simultaneously with two versions of the code, each in its submodule. Modulo the two submodules which of course have different names, the lib otherwise is having a mostly identical api for blocking and async.
A bit like having embedded-hal and embedded-hal-async in the same crate under two different modules.
Note that I've only tried this with smaller drivers where I could fit all "maybe-async" code in a single module. Not sure how it would work for a larger codebase split across many modules.
Here's the relevant comment in the maybe-async crate that I follow myself: https://github.com/fMeow/maybe-async-rs/issues/6#issuecomment-1893918591
Thank you very much for working on this @brunob45 . I wonder if you can also tweak the SdCard impl as I personally want to use the raw read/write support instead of the fat32 one.
Is this still planned to be merged? If so, I'm interested in helping if needed.
Also, I noticed that in the implementation of SdCard there are calls to embedded_hal::delay::DelayNs::delay_us. This is fine for blocking, but maybe for async version we could instead use the equivalent method from embedded-hal-async and enable a sleep with async capabilities. There are maybe other places where we can do better than just forwarding the async/await keywords, like with the maybe_async macro.
I think it still needs the SD card driver changing? And I'm still not entirely persuaded on the merits of this approach as compared to just making a different crate dedicated to async usage. Or just add partition support to that other crate.
Happy to take input from other maintainers.
I want to see this crate support async, but I don't think maybe_async is the way to go. That crate has two huge flaws. The code the macro generates differs based on Cargo features on the macro crate, rather than generating feature-gated code compiled in the crate that the macro is used in. As @ninjasource noted, because Cargo features are additive, separate crates in the dependency graph using maybe_async can break each other. The other issue is that a crate using maybe_async can't be built simultaneously for synchronous and asynchronous usage; they're mutually exclusive because maybe_async uses the presence/absence of a single Cargo feature rather than separate Cargo features for sync and async.
The bisync crate looks like a much better alternative to me. I have opened a new PR that uses that instead of maybe_async: #176
I'm still not entirely persuaded on the merits of this approach as compared to just making a different crate dedicated to async usage.
IMO this crate is too big for duplicate sync and async versions in separate crates to be maintainable. That approach works for embedded_hal & embedded_hal_async, but those are just a few hundred lines of code defining traits with no implementations.
Thank you all for the comments and interest. I'll close this PR for now, as I'll be using embedded-fatfs for my async projects.