embedded-sdmmc-rs icon indicating copy to clipboard operation
embedded-sdmmc-rs copied to clipboard

Published 20 hours ago • verified publisher icon rust-embedded-community

Support for asynchronous I/O

Open TilBlechschmidt opened this issue 3 years ago • 25 comments

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:

  1. DMA related SPI store/fetch operations
  • You give it a tx+rx buffer, it at some point interrupts you once everything has been sent/received
  • Allows the user/application to do other work in the meantime
  • Currently unsupported by the SdMmcSpi struct as it solely uses byte-by-byte synchronous send/receive methods
  1. Asynchronous block devices
  • Most disk I/O is inherently asynchronous as the CPU waits for the device
  • In a majority of use cases microcontrollers might not have much else to do
  • In some cases, other, time-critical tasks need to run (e.g. handling wireless transmissions)

My 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).

TilBlechschmidt avatar Feb 21 '22 21:02 TilBlechschmidt

  1. personally it doesn’t appeal, but someone could, sure.
  2. No, not really. async is pervasive in a codebase.

thejpster avatar Jul 06 '22 19:07 thejpster

Hi, I've been experimenting with implementing this and have the following to report.

  • First, the good news. I got async file reads working and it's running about 8x faster than the blocking version on my nrf52480 chip using Embassy and DMA. The added benefit being that it does not hold up the mcu which was the reason why I needed the functionality in the first place (I'm working with audio).
  • Now the bad news. This pretty much duplicates almost all of the codebase to the point where putting this in a separate repo or separate member of the same repo makes more sense to me than polluting this repo with feature flags in an effort to reuse some of the code. As mentioned previously, async is indeed very pervasive and duplicating everything like this is distasteful. I can't really see a way around this unfortunately.
  • Dependency wise, the change essentially swaps embedded-hal with embedded-hal-async which is alpha right now but it is a member of the embedded-hal repo so it should be standard. This means that a nightly toolchain is needed with #![feature(async_fn_in_trait, impl_trait_projections)] features enabled.
  • This is a proof of concept and I just wanted to judge the appetite for eventually merging something like this to the main develop branch. It works for my use case but I am happy to put in the time to finish it up properly and welcome any criticism or suggestions. I only really optimised the bulk read because that's where most of the time was spent for me. Hell, if you guys think this is one big bad idea then I won't be offended!
  • I experimented with a few approaches, most notably sharing the filesystem module because that has nothing to do with IO but I found myself making things public that were meant to be private and I also thought that anyone looking at these changes would appreciate being able to simply do a folder compare against the blocking and async versions to see what the differences were instead of wading through my effort to deduplicate things.
  • Here is the fork: https://github.com/ninjasource/embedded-sdmmc-rs/tree/add-async-support
  • Here is an example project using Embassy as the runtime: https://github.com/ninjasource/audio-i2s-sdmmc-nrf52840-async

ninjasource avatar Jan 11 '23 10:01 ninjasource

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.

thejpster avatar Jan 12 '23 18:01 thejpster

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.

ninjasource avatar Jan 12 '23 22:01 ninjasource

To join in the discussion, I like the approach which some other crates take on blocking vs async api - the default module async api with a second blocking module. Maybe this would make more sense for the crate as well, but in the other way around? i.e. blocking api by default and async one when a feature is enabled. This would leave both options on the table for crate users.

@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

0.000912 INFO  Init SD card...
└─ netbox_hardware::micro_sd::async_api::acquire_device::{async_fn#0} @ src/micro_sd.rs:103
0.001855 ERROR panicked at 'assertion failed: `(left == right)`'
diff < left / right >
<6
>1
└─ embassy_rp::spi::{impl#3}::transfer_inner::{async_fn#0} @ /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/fmt.rs:24
0.001937 ERROR panicked at 'explicit panic', /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/lib.rs:367:5
└─ panic_probe::print_defmt::print @ /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/panic-probe-0.3.0/src/lib.rs:91
────────────────────────────────────────────────────────────────────────────────
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: lib::inline::__udf
        at ./asm/inline.rs:181:5
   2: __udf
        at ./asm/lib.rs:51:17
   3: cortex_m::asm::udf
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/cortex-m-0.7.7/src/asm.rs:43:5
   4: rust_begin_unwind
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/panic-probe-0.3.0/src/lib.rs:72:9
   5: core::panicking::panic_fmt
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/panicking.rs:65:14
   6: core::panicking::panic
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/panicking.rs:114:5
   7: __defmt_default_panic
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/lib.rs:367:5
   8: defmt::export::panic
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/export/mod.rs:133:14
   9: embassy_rp::spi::Spi<T,embassy_rp::spi::Async>::transfer_inner::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:388:9
  10: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  11: embassy_rp::spi::Spi<T,embassy_rp::spi::Async>::transfer::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:378:50
  12: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  13: embassy_rp::spi::eha::<impl embedded_hal_async::spi::SpiBus for embassy_rp::spi::Spi<T,embassy_rp::spi::Async>>::transfer::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:589:39
  14: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  15: embedded_sdmmc_async::sdmmc::SdMmcSpi<SPI,CS>::card_command::{{closure}}
        at /mnt/Disk/Projects/lechev.space/embedded-sdmmc-async/embedded-sdmmc-async/src/sdmmc.rs:320:13
  16: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  17: $t.148
  18: embassy_executor::raw::TaskStorage<F>::poll
  19: core::cell::Cell<T>::get
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/cell.rs:452:18
  20: embassy_executor::raw::timer_queue::TimerQueue::update
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:36:12
  21: embassy_executor::raw::Executor::poll::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/mod.rs:388:17
  22: embassy_executor::raw::run_queue::RunQueue::dequeue_all
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/run_queue.rs:69:13
  23: core::cell::Cell<T>::get
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/cell.rs:452:18
  24: embassy_executor::raw::timer_queue::TimerQueue::retain
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:72:16
  25: embassy_executor::raw::timer_queue::TimerQueue::next_expiration
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:49:9
  26: embassy_executor::raw::Executor::poll
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/mod.rs:395:39
  27: embassy_executor::arch::Executor::run
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/arch/cortex_m.rs:55:17
  28: write_to_sd_card_async::__cortex_m_rt_main
        at src/bin/write_to_sd_card_async.rs:21:1
  29: main
        at src/bin/write_to_sd_card_async.rs:21:1
  30: Reset
  (HOST) ERROR the program panicked

elpiel avatar Jan 28 '23 16:01 elpiel

To join in the discussion, I like the approach which some other crates take on blocking vs async api - the default module async api with a second blocking module. Maybe this would make more sense for the crate as well, but in the other way around? i.e. blocking api by default and async one when a feature is enabled. This would leave both options on the table for crate users.

@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 embassy-rp requires for transfer that both read and write to be of equal length, although embedded_hal_async documentation states that they can be of different length - https://github.com/embassy-rs/embassy/issues/1181

0.000912 INFO  Init SD card...
└─ netbox_hardware::micro_sd::async_api::acquire_device::{async_fn#0} @ src/micro_sd.rs:103
0.001855 ERROR panicked at 'assertion failed: `(left == right)`'
diff < left / right >
<6
>1
└─ embassy_rp::spi::{impl#3}::transfer_inner::{async_fn#0} @ /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/fmt.rs:24
0.001937 ERROR panicked at 'explicit panic', /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/lib.rs:367:5
└─ panic_probe::print_defmt::print @ /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/panic-probe-0.3.0/src/lib.rs:91
────────────────────────────────────────────────────────────────────────────────
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: lib::inline::__udf
        at ./asm/inline.rs:181:5
   2: __udf
        at ./asm/lib.rs:51:17
   3: cortex_m::asm::udf
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/cortex-m-0.7.7/src/asm.rs:43:5
   4: rust_begin_unwind
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/panic-probe-0.3.0/src/lib.rs:72:9
   5: core::panicking::panic_fmt
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/panicking.rs:65:14
   6: core::panicking::panic
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/panicking.rs:114:5
   7: __defmt_default_panic
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/lib.rs:367:5
   8: defmt::export::panic
        at /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/defmt-0.3.2/src/export/mod.rs:133:14
   9: embassy_rp::spi::Spi<T,embassy_rp::spi::Async>::transfer_inner::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:388:9
  10: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  11: embassy_rp::spi::Spi<T,embassy_rp::spi::Async>::transfer::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:378:50
  12: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  13: embassy_rp::spi::eha::<impl embedded_hal_async::spi::SpiBus for embassy_rp::spi::Spi<T,embassy_rp::spi::Async>>::transfer::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-rp/src/spi.rs:589:39
  14: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  15: embedded_sdmmc_async::sdmmc::SdMmcSpi<SPI,CS>::card_command::{{closure}}
        at /mnt/Disk/Projects/lechev.space/embedded-sdmmc-async/embedded-sdmmc-async/src/sdmmc.rs:320:13
  16: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/future/mod.rs:92:19
  17: $t.148
  18: embassy_executor::raw::TaskStorage<F>::poll
  19: core::cell::Cell<T>::get
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/cell.rs:452:18
  20: embassy_executor::raw::timer_queue::TimerQueue::update
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:36:12
  21: embassy_executor::raw::Executor::poll::{{closure}}
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/mod.rs:388:17
  22: embassy_executor::raw::run_queue::RunQueue::dequeue_all
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/run_queue.rs:69:13
  23: core::cell::Cell<T>::get
        at /rustc/b7bc90fea3b441234a84b49fdafeb75815eebbab/library/core/src/cell.rs:452:18
  24: embassy_executor::raw::timer_queue::TimerQueue::retain
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:72:16
  25: embassy_executor::raw::timer_queue::TimerQueue::next_expiration
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/timer_queue.rs:49:9
  26: embassy_executor::raw::Executor::poll
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/raw/mod.rs:395:39
  27: embassy_executor::arch::Executor::run
        at /home/elpiel/.cargo/git/checkouts/embassy-9312dcb0ed774b29/aea5a0f/embassy-executor/src/arch/cortex_m.rs:55:17
  28: write_to_sd_card_async::__cortex_m_rt_main
        at src/bin/write_to_sd_card_async.rs:21:1
  29: main
        at src/bin/write_to_sd_card_async.rs:21:1
  30: Reset
  (HOST) ERROR the program panicked

elpiel avatar Jan 28 '23 17:01 elpiel

So after applying a fix to the branch to mitigate the wrong impl in embassy-rp I managed to make it work.

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 panic from embassy and I have yet to look into it or what caused it.

RTT error: Error communicating with probe: A core architecture specific error occurred
────────────────────────────────────────────────────────────────────────────────
Error: A core architecture specific error occurred

Caused by:
    0: Failed to read register DRW at address 0x0000000c
    1: An error specific to the selected architecture occurred
    2: Target device did not respond to request.

elpiel avatar Jan 29 '23 10:01 elpiel

I've steered away from the async version and we might use the littlefs2 crate instead.

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.

elpiel avatar Mar 18 '23 11:03 elpiel

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

ninjasource avatar Mar 29 '23 14:03 ninjasource

So Henrik did this already: https://github.com/kalkyl/sdmmc-embassy

thejpster avatar May 06 '23 19:05 thejpster

@thejpster this is a very old remote that you've shared and embassy doesn't have this traits crate anymore. The API should be based on embedded-hal-async IMO and be part of this crate. I don't see much benefit in splitting blocking and async API and fixing bugs on both at the same time.

@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:

  1. Bug that was fixed in SPI impl of embassy-rp
  2. Probe software

elpiel avatar May 06 '23 19:05 elpiel

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.

thejpster avatar May 07 '23 07:05 thejpster

Hi @elpiel,

@ninjasource has the most up-to-date branch that I amanged to test and it was much slower that the blocking version.

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!

ninjasource avatar May 09 '23 11:05 ninjasource

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 File handles are not automatically closed on drop, since that would require the drop impl to be async. Instead that must be done manually. It also has experimental support for multi-block writes through const-generics.

peterkrull avatar Mar 28 '24 00:03 peterkrull

I'll just leave this here: https://github.com/fMeow/maybe-async-rs So only the backend bus implementations will need to be made separately, the rest of the code could be async but downgraded to blocking with a feature flag.

ilya-epifanov avatar Jun 21 '24 10:06 ilya-epifanov

I've been working on separating the SDCard implementation to 2 separate impls using a known pattern in Rust. Ofc this basically duplicates all the code and slaps async/await everywhere but the next step would be to remove the duplication using maybe-async or a similar crate for this.

You can see how the API looks like in this branch: https://github.com/LechevSpace/embedded-sdmmc-rs/pull/new/feat/async-feature

Additions or ideas for the implementation, feature name and how to use maybe-async to remove duplication are welcome!

PS: I personally would like to see the SdCard struct worked on first to figure out how to do it for the rest of the interfaces (like fat) as well.

elpiel avatar Jun 26 '24 10:06 elpiel