Add SdCardSpiDevice trait #185
Conversation
|
Seems reasonable enough, but I'd like to see some current users of embedded-sdmmc-rs port over to this style to check it's usable in practice. |
|
There's also some overlap with #170, which is about SD Cards in general (i.e. using SD Card Host Controllers with the 4-bit bus) rather than this which is about SPI bus based SD Cards specifically. |
I'll also do some dogfooding by using it in my project. I'm mostly interested in the async API but I think I can do some tests with the block API.
How do you want to move forward with this? There hasn't been any recent activity on that PR. I think it makes sense to first fix the dummy byte issue. |
|
If we're going to abstract the SD card transport it makes sense to do it in a way that both fixes the SPI issue and allows the use of SD Host Controllers. I also have a mild preference for custom types over implementing traits on tuples. I think it makes it clearer what's going on. Maybe something like:
|
|
Personally I don't have the need for a generic |
|
|
||
| - `embedded-hal-bus-03`: adds support for `embedded-hal-bus::spi::ExclusiveDevice`. This is probably the easiest way to use this crate when the SD card is the only device on the bus. | ||
| - `embassy-sync-06`: adds support for using a blocking mutex from the `embassy-sync` crate. This allows sharing the bus between multiple tasks. | ||
|
|
||
| ```rust | ||
| use embedded_sdmmc::{SdCard, VolumeManager, Mode, VolumeIdx}; | ||
| // Build an SD Card interface out of an SPI device, a chip-select pin and the delay object |
There was a problem hiding this comment.
This example needs to be kept in sync with the copy in readme-test.
|
Ok, fair enough. |
|
Is this still planned? I wrote a patch to fix the build test checks and open a PR in @avsaase fork. I'm interested in trying to have the async API merged, so if you need help I will be happy to contribute! |
fix rename error
|
Some (positive) life events happened so I couldn't work on this for a few weeks but I'm still planning on getting this merged. The last thing I did was to quickly whip up an async version of this PR to see how that would work. Unfortunately it did not work at all and consistently gave CRC errors. I haven't had the chance yet to see if this was a problem with this PR or my quick async version of it (it doesn't look like I pushed that code to github). In any case it's probably best to test the code from this PR first to see if the basic approach is correct. If you want to help with that that would be great. |
|
I could test your 2 versions async and blocking next week if you want I would have access to real hardware (rp2040 + some SD cards). I will just need the async code. I'm not sure what is the best way to include the async code since the other PR is still waiting. |
|
I pushed my async compatible version of this PR here: https://github.com/avsaase/embedded-sdmmc-rs/tree/bisync-sd-card-device. |
|
When writing a simple example of just raw reads and writes, I struggle a bit to see how to set the frequency to a high value after the initialization of 400 kHz. We could do this in the previous API : let mut config = embassy_rp::spi::Config::default();
config.frequency = 16_000_000;
sdcard.spi(|dev| dev.bus_mut().set_config(&config));But with the new one I don't see a way of doing that, maybe we should add a bus_mut method to the |
|
I also tried with an |
Yeah that's a tricky one. Providing mutable access to the bus does not allow using different SPI speeds for different SPI devices. In my own project that uses embassy I created a newtype and implemented pub struct SdCardSpiDeviceWithConfig<'a, BUS: SetConfig, CS, M: RawMutex> {
bus: &'a Mutex<M, BUS>,
cs: CS,
config: BUS::Config,
}
impl<'a, BUS: SetConfig, CS: OutputPin, M: RawMutex> SdCardSpiDeviceWithConfig<'a, BUS, CS, M> {
pub fn new(bus: &'a Mutex<M, BUS>, cs: CS, config: BUS::Config) -> Self {
Self { bus, cs, config }
}
}
impl<BUS: SpiBus + SetConfig, CS: OutputPin, M: RawMutex> SdCardSpiDevice
for SdCardSpiDeviceWithConfig<'_, BUS, CS, M>
{
async fn transaction(&mut self, operations: &mut [Operation<'_, u8>]) -> Result<(), SdCardDeviceError> {
let mut bus = self.bus.lock().await;
bus.set_config(&self.config).map_err(|_| SdCardDeviceError::Spi)?;
bus_transaction(&mut *bus, &mut self.cs, operations).await
}
async fn send_clock_pulses(&mut self) -> Result<(), SdCardDeviceError> {
let mut bus = self.bus.lock().await;
send_clock_pulses(&mut *bus, &mut self.cs).await
}
}I just pushed a commit to this branch that makes |
I tested this version on a ESP32S3 with embassy let sclk = peripherals.GPIO12;
let miso = peripherals.GPIO11;
let mosi = peripherals.GPIO13;
let cs = peripherals.GPIO10;
let mut sd_cs = Output::new(cs, Level::High);
let dma_channel = peripherals.DMA_CH0;
let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
let mut spi_bus = Spi::new(
peripherals.SPI2,
spi::master::Config::default()
.with_frequency(400.kHz())
.with_mode(spi::Mode::_0),
)
.unwrap()
.with_sck(sclk)
.with_mosi(mosi)
.with_miso(miso)
.with_dma(dma_channel)
.with_buffers(dma_rx_buf, dma_tx_buf)
.into_async();
let spi_dev = RefCellSdCardDevice::new(spi_bus, sd_cs);
let sdcard = SdCard::new(spi_dev, Delay);
sdcard.init_card().await;
let mut volume_mgr = VolumeManager::new(sdcard, DummyTimesource::default());
static VOL_MGR: StaticCell<
VolumeManager<
SdCard<RefCellSdCardDevice<'_, SpiDmaBus<'_, esp_hal::Async>, Output<'_>>, Delay>,
DummyTimesource,
>,
> = StaticCell::new();
let volume_mgr = VOL_MGR.init(volume_mgr);
let volume0 = volume_mgr.open_volume(VolumeIdx(0)).await.unwrap();It crashes on this When analyzing the bus I see CMD17 as the last command. Is the CS line correct? I think https://github.com/avsaase/embedded-sdmmc-rs/blob/08fc93c092da0c030e9618264c805d0fedea669b/src/inner/sdcard/mod.rs#L371-L373 is where the error is thrown? |
|
Maybe I get something wrong, but I can't see why just exposing a method to get a mutable reference to the underlying impl<'a, BUS, CS> RefCellSdCardDevice<'a, BUS, CS> {
/// Try stuff
pub fn foo(&self) -> RefMut<'_, BUS> {
self.bus.borrow_mut()
}
}However, I also confirmed running read errors on a different target (rp2040). #![no_std]
#![no_main]
#![feature(impl_trait_in_assoc_type)]
use core::cell::RefCell;
use defmt::*;
use embassy_executor::Spawner;
use embassy_rp::gpio;
use embassy_rp::spi::Spi;
use embedded_sdmmc::asynchronous::{SdCard, sdcard::device::RefCellSdCardDevice};
use gpio::{Level, Output};
use {defmt_rtt as _, panic_probe as _};
#[embassy_executor::main]
async fn main(_spawner: Spawner) {
// BEGIN stuff with real peripherals
let p = embassy_rp::init(Default::default());
let mut config = embassy_rp::spi::Config::default();
config.frequency = 400_000;
let spi_bus = Spi::new(
p.SPI1, p.PIN_10, p.PIN_11, p.PIN_12, p.DMA_CH0, p.DMA_CH1, config,
);
let cs = Output::new(p.PIN_13, Level::High);
// END stuff with real peripherals
let delay = embassy_time::Delay;
let bus = RefCell::new(spi_bus);
let spi_device = RefCellSdCardDevice::new(&bus, cs);
let sdcard = SdCard::new(spi_device, delay);
sdcard.init_card().await.unwrap();
info!("init is ok");
let mut config = embassy_rp::spi::Config::default();
config.frequency = 16_000_000;
sdcard.spi(|dev| dev.foo().set_config(&config));
info!("freq is ok");
info!("Card size is {} bytes", sdcard.num_bytes().await.unwrap()); // it panics here!
}EDIT: I found the bad commit is this one 835b2e4 (i.e the last one pushed on the async fork) |
How would you use two devices with differett speeds on the same bus? You need a way to change the speed for every bus transaction and I think the only way to do that properly is in the
That's strange. I and several of my users are using that exact commit without any problem for weeks now. |
Yes, and you can call the set_config method directly, like in my example, by just adding a tiny get_inner method. I don't see why you need to reimplement the trait "only for that". If you need to set the frequency before each transaction you will reimplement it for your custom type to avoid calling get_inner every time, but for using it once it's easier with just exposing the reference to the concrete type. |
Closes #184.
This only adds a blocking trait. Once #176 is merged I can rebase and add the async version. Or this can be merged first and the other PR can add the async version. Either is fine with me.