-
Notifications
You must be signed in to change notification settings - Fork 76
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
Support for asynchronous I/O #50
Comments
|
Hi, I've been experimenting with implementing this and have the following to report.
|
I was thinking of moving the FAT32 code out into another crate, but even then that doesn't help because it does read operations and so has to have both sync and async versions. It may be easier to fork the project until the generic keywords feature comes along. |
Oh wow, this is the first time I've heard of the generics keywords initiative. Sounds very promising indeed! I will try to keep this fork up to date and hopefully others will find it useful for the time being. Agreed, moving the FAT32 code out wouldn't help because of all that IO in the volume module which happens to make up most of the code. The filesystem code can be shared between blocking and async but it would need a little refactoring to not expose the internals outside the crate. I ultimately abandoned my efforts there because it became ugly. It's a pity that pub(crate) is not visible between members of the same project. |
To join in the discussion, I like the approach which some other crates take on blocking vs async api - the default module @ninjasource I tried your branch but currently it fails for me because of #33 I believe but it does a panic instead of getting this error. I took a look at the code and it seems that one of the loops was removed which might be causing this? PS: I'm using embassy
|
To join in the discussion, I like the approach which some other crates take on blocking vs async api - the default module @ninjasource I tried your branch but currently it fails for me because of #33 I believe but it does a panic instead of getting this error. I took a look at the code and it seems that
|
So after applying a fix to the branch to mitigate the wrong impl in It did work for a while although much slower I must add that the blocking api but I will do more research on this part. In my latest test, after ~ 1200 lines written to a single file I got a
|
I've steered away from the async version and we might use the However, I wanted to give a quick update: Writing works and the error I've encountered before is related to the debug probe rather than the crate itself. |
Hi @elpiel, I'm glad you managed to figure out your error. My apologies for the silence but I don't have any use case for writing to the SD card so I couldn't reproduce your issue without doing a whole bunch of work. I personally think that using the generics keyword feature would be a better approach to having separate async and blocking interfaces due to all the duplicate that would entail. It's a long way off though. https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics.html |
So Henrik did this already: https://github.com/kalkyl/sdmmc-embassy |
@thejpster this is a very old remote that you've shared and embassy doesn't have this traits crate anymore. @ninjasource has the most up-to-date branch that I amanged to test and it was much slower that the blocking version. It can be found here https://github.com/ninjasource/embedded-sdmmc-rs/tree/add-async-support The issues that I have encountered were/are realted to:
|
You will always have to fix bugs in both because the blocking and async APIs are fundamentally incompatible due to how Rust is currently designed. The link was given to me by the author yesterday. If it requires updates you are welcome to reach out to the author. |
Hi @elpiel,
I actually thought the async version was faster but if its slower then I highly recommend you use the blocking version as the embedded-hal traits it uses are far more stable. I made the fork and async changes to solve a specific problem I was working on and thought I'd keep it up there for others to use. Btw, the only reason for the performance difference is that the async version uses DMA so bulk data transfers are handed over to a dedicated controller, freeing up cpu to do other stuff. If you re experiencing problems with the async version you are welcome to post them as issues on my branch and I'll take a look. Async and embassy are constantly changing so this is not a stable space I'm afraid. Will get there eventually! |
For anyone who stumbles across this issue in search for async, I have an up to date (at the time of writing) branch which supports async: https://github.com/peterkrull/embedded-sdmmc-rs/tree/develop-async One difference to note is that |
I'll just leave this here: https://github.com/fMeow/maybe-async-rs |
I've been working on separating the SDCard implementation to 2 separate impls using a known pattern in Rust. You can see how the API looks like in this branch: Additions or ideas for the implementation, feature name and how to use PS: I personally would like to see the |
@thejpster I know we've discussed this multiple times but could you take a look at the branch I shared in my previous comment? If you are ok with the API I can take a look at some of the crates to remove the duplication of code and allow both async and blocking impl to be in single implementation and enable one or the other with a feature. The tl;dr: is that for blocking, the macros can remove all the async/await keywords. |
OK so it looks like that branch brings turns on some unstable feature gates when the Looking at Perhaps there are other Rust embedded async API experts who would weigh in on whether this is a good approach or not. |
I understand. I wanted to show you the Public API first and then work on removing duplicates, removing nightly features and merging async and sync impls with a macro. It's still a wip impl in the works |
People keep asking, and the maybe_async approach is plausible so I'm re-opening this issue. I won't be writing the PR, but I'm not against merging the PR as long as it doesn't make the codebase significantly more complex to understand. |
@thejpster I did update my branch with async support to the latest 0.8 version: https://github.com/LechevSpace/embedded-sdmmc-rs/pull/new/feat/async-feature-0.8 Notes on nightly features:
|
If you remove the |
I've been primarily using embassy-embedded-hal which has both blocking and async shared_bus implementations which allow you to get the underlying SPI with by locking the Mutex or use I do see, however, that there's the embedded-hal-bus although I haven't used it, it also allows the use of a Mutex and creating a new SpiDevice to pass to SdCard still allows you to lock the mutex later on and change the clock speed. |
Fair enough. |
I am currently experimenting with the nRF52 and using the embassy project. I intend to use an SD Card in the project. However, this library does not appear to support asynchronous APIs. This includes two levels:
SdMmcSpi
struct as it solely uses byte-by-byte synchronous send/receive methodsMy question here is: Would you be interested in integrating support for asynchronous SPI backends?
Follow-up question: If yes, do you have any thoughts on how to do that besides just "implementing everything twice"
I can definitely see both arguments; for integrating it to have a unified place to look for when using SD cards; against stuffing everything into one crate even though it is basically its own thing (sadly, we can't simply pick and choose between async/sync APIs like we can with traits/generics).
The text was updated successfully, but these errors were encountered: