add async support using bisync crate

[Be-ing]

This moves the existing API into a blocking module and duplicates the API into a new asynchronous module, using async fn where applicable. This uses the bisync crate rather than maybe_async used by #154. This involved moving everything in the src directory into a new src/inner directory, then exposing blocking/async versions via how they are imported in src/lib.rs.

I think this resolves the concerns brought up in #154: sync and async can both be built at the same time, usage of bisync by other crates in the dependency graph doesn't affect this, and changes to the example and test code are minimal (just adjusting import path).

I moved submodules that have no difference between blocking & async to a new common module. That code gets re-exported under the blocking/asynchronous modules in the same relative path in the module hierarchy as before. There may be opportunities to split more code into that common module, but I have tried to keep changes to a minimum for now to make it easier to review.

I have not added Cargo features for sync/async; they're both built. If requested, I could put each behind a feature gate, though I don't think it's necessary.

Fixes #50

[thejpster]

An interesting approach! I want to dig into the docs in more detail but it seems workable - modulo that AFIT feature-flag that has appeared.

[thejpster]

It might also be good to move the example import renames to another PR to keep this one cleaner so we can see precisely only that which is affected by the async additions.

[Be-ing]

It might also be good to move the example import renames to another PR to keep this one cleaner so we can see precisely only that which is affected by the async additions.

That goes towards a bigger question: do you want to break the public API by moving the existing API into a new blocking module? This can be avoided by making the blocking module private and instead putting pub use blocking::*; in src/lib.rs. I don't think that would be great for downstream users in the long term though. My primary concern is that it would make the documentation confusing to navigate. Here's how the front page of the documentation would look with that:

There's the whole blocking API of the crate, then the asynchronous module... and when you go to the documentation for that, you see the same API:

I think this would be rather confusing. It's difficult to even tell that I clicked a link and navigated to another page because it looks so similar to the documentation's front page.

By moving the existing API into a blocking module (how this branch currently is), the front page of the documentation looks like this:

[Be-ing]

I've avoided running cargo fmt for now to minimize the diff because it's rather big already. If you'd like that to be run now, just let me know, or I could do it later after you've reviewed.

[Be-ing]

I had to use the #[only_sync] macro in a few places. Those are the impl Drop for File and Volume; unfortunately, there is no such thing (yet) in Rust as an async drop, so that's the best we can do. VolumeManager::close_file and VolumeManager::close_file can be invoked manually though. I presume bad things would happen if those aren't closed manually? If so, I can add a note to the documentation that has to be done manually for the async versions.

The other places I used #[only_sync] were just to make the tests simple to implement.

[thejpster]

I presume bad things would happen if those aren't closed manually?

Yes, leaking a file handle means you can't reopen it, and handles are a finite resource. Plus certain context is only written to disk on file close so the disk ends up needing a fsck/scandisk pass.

As long as that only affects the async users that's fine.

Moving the blocking API into the blocking module is fine with me.

[thejpster]

Can you rebase? #177 might make this smaller.

[thejpster]

Is it possible to give the async version a blocking drop? Better than doing nothing.

[Be-ing]

Can you rebase? #177 might make this smaller.

Okay, rebased.

Is it possible to give the async version a blocking drop? Better than doing nothing.

Unfortunately not. That would require this crate to bring in its own async executor and set that up just within impl Drop. It's questionable if that would be desirable if it was even possible. In a library that uses std, you can wrap async functions with a simple async executor like futures_lite::future::block_on or pollster::block_on, but both of those use std modules like std::future and std::sync that aren't available in core.

[thejpster]

Is there no no_std block_on! ? Can't you just spin polling the Future?

[Be-ing]

I just found embassy_futures::block_on, however, I don't think it's a good idea to use it to impl Drop for async Files/Volumes. We can't know if a user would want that and there's no way to opt out of Drop::drop getting called if it's implemented. For example, the user could have strict power consumption requirements that require putting the MCU to sleep on every .await as Embassy and RTIC do.

[thejpster]

But it's probably better than a corrupt file system, and any correct program will close them asynchronously before dropping.

What do other async File APIs do?

[Be-ing]

tokio::fs::File:

A file will not be closed immediately when it goes out of scope if there are any IO operations that have not yet completed. To ensure that a file is closed immediately when it is dropped, you should call flush before dropping it. Note that this does not ensure that the file has been fully written to disk; the operating system might keep the changes around in an in-memory buffer. See the sync_all method for telling the OS to write the data to disk.

async-std::fs::File:

Files are automatically closed when they get dropped and any errors detected on closing are ignored. Use the sync_all method before dropping a file if such errors need to be handled.

impl Drop for File in async-std:

impl Drop for File {
    fn drop(&mut self) {
        // We need to flush the file on drop. Unfortunately, that is not possible to do in a
        // non-blocking fashion, but our only other option here is losing data remaining in the
        // write cache. Good task schedulers should be resilient to occasional blocking hiccups in
        // file destructors so we don't expect this to be a common problem in practice.
        let _ = futures_lite::future::block_on(self.flush());
    }
}

Suggestion: impl Drop for async File/Volume using embassy_futures::block_on, but put that behind an async_drop Cargo feature that is enabled by default. This will Just Work by default, and any project where that little bit of blocking is really a problem has an escape hatch.

[Be-ing]

Suggestion: impl Drop for async File/Volume using embassy_futures::block_on, but put that behind an async_drop Cargo feature that is enabled by default. This will Just Work by default, and any project where that little bit of blocking is really a problem has an escape hatch.

Nevermind, I forgot core::mem::forget exists to opt out of Drop, and of course File/Volume::close already use it. There's no need for a Cargo feature to prevent dropping; that can be opted out of just by calling File/Volume::close.

[Be-ing]

Some blogs about async drop:
https://without.boats/blog/asynchronous-clean-up/
https://sabrinajewson.org/blog/async-drop
https://smallcultfollowing.com/babysteps/blog/2023/03/16/must-move-types/

TL;DR: It's complicated and Rust is a long way from async drop being implemented. Probably not for some more years.

Blocking in Drop is an ugly workaround, but I think it's sufficient for now.

[thejpster]

You'll need to handle dropping Directories too.

[Be-ing]

Directory::drop doesn't make any async calls, so there's nothing to do there. The existing implementation is already enough.

[yanshay]

Hi,
What's the status of this PR? Any plans to merge it? Any known reasons not to use this - I mean, is there any known experience that it's less good than the sync version?
I started using it and as long as I close() things and don't let them drop it seems to work fine.
The 0xFF issue mentioned above seems to be not specific to the async version, right?

[marcfir]

I found a solution to the WriteError problem by changing the way commands are sent to the card. Instead of writing the command bytes in one operation and then doing multiple single byte reads until (result & 0x80) == ERROR_OK, we can do both the write and the read in a single transfer. After the transfer is complete we scan the buffer for the response bytes. For the async API this is probably a performance improvement because the DMA can do a single transfer which can be awaited. A small downside is this likely clocks out a few more 0xFF bytes from the card than strictly necessary but I don't think that's a problem.

According to this Stack Overflow answer (which gave me this idea), cards always start sending their responses within 1-8 bytes after the command bytes are sent. This puts an upper bound on the number of bytes in the transfer (19 to be precise).

I have a commit here that demonstrates this approach. I confirmed that this solves the problem shown in the reproducer. The blocking version also still works fine. The way I changed the card_command return type is not super elegant but this was necessary because now all the response bytes are read in one go, so they need to be passed back to the caller in case it needs to use the bytes after the first. This can probably be made more strongly typed but I wanted to get some feedback on this before spending too much time on it.

I can make a PR against this branch or feel free to cherry pick the commit.

@avsaase The single transfer is good, except for CMD17 and CMD18, since this transfer can contain the DATA_START_BLOCK.
I provided a fix for this issue at Be-ing#2.

[PegasisForever]

@avsaase I believe your Be-ing#1 breaks CMD9 for reading csd (and potentially other commands).

I'm testing with a SDHC card on stm32G4 with embassy framework.

This is the correct csd data the library is supposed to read after issuing CMD9: [64, 14, 0, 50, 91, 89, 0, 0, 28, 223, 127, 128, 10, 64, 0, 215]
After the pr, this is the command response returned from card_command(CMD9, 0): [0, 255, 254, 64, 14], and the read_data(&mut csd.data) following that throws an ReadError because the first byte it got is 223 instead of the expected 0xFE.
You can tell the card_command(CMD9, 0) is eating up some data that is supposed to be received by read_data. (..., 64, 14 part), also 223 is one of the numbers in the actual csd data.

[avsaase]

Did you try @marcfir's linked PR? That seems to address the issues your describe.

The problem here is that if the task gets stalled after sending the command the card can hit a timeout, failing the transfer. At least that is my assumption. Ideally we'd have some way to not returning to the executor after sending the command while we wait for the right response bytes but as far as I know this kid of mixing of sync and async operations is not possible.

[thejpster]

The whole point of having an async SPI transport is that you can do other things whilst the data is being sent over SPI (e.g. using DMA), and if you do an async operation in-between two SPI operations then yes, some other task has an opportunity to run.

It's definitely an argument for making an async-only SD card library - one that is cognizant of these issues and works around them.

[PegasisForever]

Did you try @marcfir's linked PR? That seems to address the issues your describe.

The problem here is that if the task gets stalled after sending the command the card can hit a timeout, failing the transfer. At least that is my assumption. Ideally we'd have some way to not returning to the executor after sending the command while we wait for the right response bytes but as far as I know this kid of mixing of sync and async operations is not possible.

No, that pr does not fix things, as that pr only changes behaviour for CMD17 and CMD18. I wonder how how he got CMD9 working on his cards, maybe its just card-to-card variance.

[yanshay]

Any plans to merge this to the main release?

I'm using this for quite some time in a project and all in all its working great.

Only issue I've seen are two reports where a file ended up being length zero (code wise I couldn't see a flow that could cause this). Could it be due to the async nature or just a potential bug in the library unrelated to async?

[thejpster]

As I understand it, there's still an issue where another async task might intervene during a card transaction and cause the card to timeout the command, which we don't currently handle. That would need to be fixed.

I still also need to decide of the pain of bisync is worth the benefit of 'async support' that I don't personally need or use right now. I'd certainly want some embedded async Rust experts on-board for maintenance, because I don't have any way of testing it.

[thejpster]

There's also always https://github.com/MabezDev/embedded-fatfs/ as an alternative, which does appear to support async operation.

[yanshay]

As I understand it, there's still an issue where another async task might intervene during a card transaction and cause the card to timeout the command, which we don't currently handle. That would need to be fixed.

Could this be the reason for the zero length file? I definitely have many other async tasks, pretty complex application.

If after I write I test the file length and retry in case it's not as expected, will it use the cache for length information or will it check the sd card for real? If cache, any way to clear cache and test?

I still also need to decide of the pain of bisync is worth the benefit of 'async support' that I don't personally need or use right now. I'd certainly want some embedded async Rust experts on-board for maintenance, because I don't have any way of testing it.

First, it's working well so far for me and multiple users of my project. Other than two cases of zero length files after write.

And I don't see how any complex application can be written without async support, maybe there's some technique I'm not aware of but I run tens of tasks simultaneously and without async wouldn't be able to use this library. Just stating that from my POV it's definitely worth any complexity added.
Just my 2 cents.

[zpg6]

As I understand it, there's still an issue where another async task might intervene during a card transaction and cause the card to timeout the command, which we don't currently handle. That would need to be fixed.

Could this be the reason for the zero length file? I definitely have many other async tasks, pretty complex application.

If after I write I test the file length and retry in case it's not as expected, will it use the cache for length information or will it check the sd card for real? If cache, any way to clear cache and test?

I still also need to decide of the pain of bisync is worth the benefit of 'async support' that I don't personally need or use right now. I'd certainly want some embedded async Rust experts on-board for maintenance, because I don't have any way of testing it.

First, it's working well so far for me and multiple users of my project. Other than two cases of zero length files after write.

And I don't see how any complex application can be written without async support, maybe there's some technique I'm not aware of but I run tens of tasks simultaneously and without async wouldn't be able to use this library. Just stating that from my POV it's definitely worth any complexity added. Just my 2 cents.

Hi @yanshay - great work on this! Any chance the zero length file was caused by not flushing in time for the directory entry to be updated (data is there but doesn't reflect when you browse the SD Card contents). That had confused me greatly in the past.

[zpg6]

As I understand it, there's still an issue where another async task might intervene during a card transaction and cause the card to timeout the command, which we don't currently handle. That would need to be fixed.

@thejpster can you be more specific about "another async task might intervene" - do you mean two tasks trying to write to the card? Because other activity on SPI should be allowed by other tasks of course "during sequence of a transaction".

[thejpster]

• Task A sends command to card
• Task A awaits a small delay
• Task B runs and does a lot of data processing that takes a long time
• Task A resumes and tries to read a response from the card, which has forgotten the original request because it was so long ago

For this library to be generally useful for async and RTOS use-cases, it needs to be robust about retrying commands. I never designed it for those use-cases. I designed it for Arduino style blocking code where any interrupts were always super short.

[zpg6]

• Task A sends command to card

• Task A awaits a small delay

• Task B runs and does a lot of data processing that takes a long time

• Task A resumes and tries to read a response from the card, which has forgotten the original request because it was so long ago

For this library to be generally useful for async and RTOS use-cases, it needs to be robust about retrying commands. I never designed it for those use-cases. I designed it for Arduino style blocking code where any interrupts were always super short.

Okay that makes more sense.

When you say "has forgotten the request" - you're referring to the processor in the SDcard itself will have moved on from the transaction after a timeout and has no memory reference of it? Can you point to where in the specification this is described more in detail?

[yanshay]

As I understand it, there's still an issue where another async task might intervene during a card transaction and cause the card to timeout the command, which we don't currently handle. That would need to be fixed.

Could this be the reason for the zero length file? I definitely have many other async tasks, pretty complex application.
If after I write I test the file length and retry in case it's not as expected, will it use the cache for length information or will it check the sd card for real? If cache, any way to clear cache and test?

I still also need to decide of the pain of bisync is worth the benefit of 'async support' that I don't personally need or use right now. I'd certainly want some embedded async Rust experts on-board for maintenance, because I don't have any way of testing it.

First, it's working well so far for me and multiple users of my project. Other than two cases of zero length files after write.
And I don't see how any complex application can be written without async support, maybe there's some technique I'm not aware of but I run tens of tasks simultaneously and without async wouldn't be able to use this library. Just stating that from my POV it's definitely worth any complexity added. Just my 2 cents.

Hi @yanshay - great work on this! Any chance the zero length file was caused by not flushing in time for the directory entry to be updated (data is there but doesn't reflect when you browse the SD Card contents). That had confused me greatly in the past.

I always close the directory after writes. My understanding is that this stores the data to the sdcard. Is there a separate flush operation for the directory?

[yanshay]

• Task A sends command to card
• Task A awaits a small delay
• Task B runs and does a lot of data processing that takes a long time
• Task A resumes and tries to read a response from the card, which has forgotten the original request because it was so long ago

For this library to be generally useful for async and RTOS use-cases, it needs to be robust about retrying commands. I never designed it for those use-cases. I designed it for Arduino style blocking code where any interrupts were always super short.

I can only say from my experience it works pretty well in a pretty complex async application (though load on sdcard is not high but continuous). So maybe it is not far from being robust also for async use cases.

[thejpster]

When you say "has forgotten the request" - you're referring to the processor in the SDcard itself will have moved on from the transaction after a timeout and has no memory reference of it? Can you point to where in the specification this is described more in detail?

I don't think it's mandated in the SD Card specification that a card must forget a previous command, but I think it's reasonable for an SD card to be designed that it will forget a previous command. After all, it needs to accomodate spurious MCU crashes and reboots and other issues.

[robamu]

Hey.

This looks interesting. I'd like to learn lower-level SDMMC and I will probably add embedded-sdmmc support for the Zynq7000 I have here. I think async support would also be very useful for me :) But let's see how far I get with the blocking impl first...

[yanshay]

Hey.

This looks interesting. I'd like to learn lower-level SDMMC and I will probably add embedded-sdmmc support for the Zynq7000 I have here. I think async support would also be very useful for me :) But let's see how far I get with the blocking impl first...

Just keep in mind that if you are using the main crate branch there have been breaking changes afaik and porting to async may not be straightforward. This branch is behind.
If you use what’s on this branch then you are fine later switching to async.

[Be-ing]

I didn't realize there were so many merge conflicts on this branch now. I'm not resolving them.

I still also need to decide of the pain of bisync is worth the benefit of 'async support' that I don't personally need or use right now.

I'm not going to keep maintaining a branch for months/years while conflicting changes get merged to the develop branch, certainly not without unambiguous support from the maintainer. Been there, done that, not doing it again. This PR demonstrates that using bisync would be a maintainable approach to adding async support to this crate. However, this is a large branch touching the entire crate; it is not easy to maintain out of tree. I'm frankly a bit confused about @thejpster's lack of interest in this, particularly considering how many people have asked for this and contributed towards it. As far as I'm concerned, this was ready to merge months ago, even if there were outstanding issues. It could have been merged with warnings in the documentation, or with a Cargo feature flag that defaulted to off, or not exposing async in the public API; any of those would have been better than letting this branch rot. I'd be glad if someone rebased this branch in a new PR or rewrote it on top of the current develop branch. I wouldn't recommend spending your effort on that without @thejpster agreeing in advance that it would be worth merging, or planning to maintain a fork yourself. Or use embedded-fatfs (which I found out existed after I started this branch).

I started this branch as a proof-of-concept. It was useful for testing the hardware I was working with. It turned out there were other blockers besides async SD card I/O for that project, so I put it on the back burner and am focusing on other projects now. So I have no motivation to deal with merge conflicts with no end in sight.

[yanshay]

@Be-ing Sorry to see you go, but thank you very much for the work you've invested in this branch, it has been put to good use.
I had the same experience with at least one other repo I added async to so I keep a fork with the async branch. It seems like when not using async the maintainers have a meaningful barrier to verify the async is working well so they are not fond of merging async stuff. That's a major challenge for the rust embedded community. I actually don't understand how developers develop anything meaningful w/o using async, but I probably miss some basic technique when using blocking to do so.

That said, I think its better, even if you don't plan to invest any more in this, to keep the branch open in case someone needs it. When branch is closed its more difficult to find and the discussion here is very valuable. Anyone who would decide to use it will go over the discussion here and decide if they want to use it or not.

Thanks again!

[robamu]

Oh, it definitely was not my intention to see this closed..

I actually don't understand how developers develop anything meaningful w/o using async

Async Rust seems to be the replacement for a traditional RTOS. Non-blocking C/C++ is either tied to the RTOS (we use a proprietary C FS driver in some other project, which was dependent on FreeRTOS.. blergh) or the API needs to be designed to support blocking (e.g. nb).

It seems like when not using async the maintainers have a meaningful barrier to verify the async is working well so they are not fond of merging async stuff

It would add to the maintenance burden, and the library was never designed for async (as opposed to embedded-fatfs). I know that smoltcp does not use async, but the low-level layers were explicitely designed to support non-blocking DMA, and there is embassy-net which adds an async API on top of smoltcp. However, it might not be that easy to do this here, I think the low level functions here are methods like read/write which are either blocking or async?

I am not an expert with SDMMC, but I know that the Zynq7000 chip has a dedicated SDIO peripheral and various DMA options.
embedded-fatfs seems nice, but I wonder how well maintained it is seeing that the last commits were merged last year..

[yanshay]

embedded-fatfs seems nice, but I wonder how well maintained it is seeing that the last commits were merged last year..

I just tried it out and managed to get it working with a simple application on esp32s3, both with DMA and without. The API seems easier to use. But I also have concerns it's not really maintained. I saw one of the forks has merged all PR's there a few days ago. Some of the PR's seems pretty basic, like alloc feature not compiling.

[Be-ing]

a dedicated SDIO peripheral

This crate only works with SPI (which, I've found out through hard experience, it not really sufficient for glitch free audio playback). embedded-fatfs seems to be designed to work with different low level protocols, though I think only SPI has been implemented so far?

embedded-fatfs has not been maintained for a while, though the author is a very active member of Espressif's Rust team. Maybe he could be motivated to spend a bit of time on embedded-fatfs maintenance with community encouragement.

[thejpster]

maybe he could be motivated to spend a bit of time on embedded-fatfs maintenance with community encouragement

Please do not attempt to "motivate" maintainers. None of us is getting paid to do this maintenance, and the more PRs and "community engagement" we get, the more free work the maintainers have to do. If you'd like to see changes, send small, clean, well tested PRs that either umabigiously solve known issues, or that add features the maintainer is looking to receive and is happy to maintain in the future.

SPI ... not really sufficient for glitch free audio playback

I have had no problem streaming 16-bit 48 kHz stereo PCM audio using embedded-sdmmc over SPI. I even did it on stage live during a conference talk. The bandwidth required is 1.536 Mbit/sec, which even on a very slow 5 MHz SPI link should be no problem whatsoever.

Locking this PR because further discussion here is not useful for this crate.