should use SpiBus, not SpiDevice

[Dirbaio]

embedded-sdmmc-rs/src/sdcard/mod.rs

Lines 61 to 67 in 710bd34

	/// Built from an [`SpiDevice`] implementation and a Chip Select pin.
	/// Unfortunately, We need control of the chip select pin separately from the [`SpiDevice`]
	/// implementation so we can clock out some bytes without Chip Select asserted
	/// (which is necessary to make the SD card actually release the Spi bus after performing
	/// operations on it, according to the spec). To support this, we provide [`DummyCsPin`]
	/// which should be provided to your chosen [`SpiDevice`] implementation rather than the card's
	/// actual CS pin. Then provide the actual CS pin to [`SdCard`]'s constructor.

If you take a CS pin directly, you have to use SpiBus, not SpiDevice.

Using SpiDevice with a dummy CS pin breaks bus sharing. (the bus looks "free" to the mutex between transactions, so it might grant the mutex to some other driver, which will break because this lib still has the CS pin asserted)

embedded-hal docs explicitly say not to do this https://docs.rs/embedded-hal/latest/embedded_hal/spi/index.html#for-driver-authors

[thejpster]

V6.0 of Part 1: Physical Layer Specification says:

One work around is to change the library to take an SpiDevice and simply write into the docs that before first use, the card must be sent 74 clock cycles at no more than 400 kHz with SPI CS inactive (high). We already require people to change the clock speed out of band, so doing the init clocks out of band isn't too outrageous.

Edit to add: I expect most people to ignore the recommendation and I expect most people to get away with ignoring it

[sky-dragn]

My understanding of the issue being raised is that it's not really about the initial clock cycles required for SPI mode and more about the fact that this library's implementation acts on the bus (i.e. thru asserting CS) while the SpiDevice has no knowledge of this. This leads to a race condition where, if the SpiBus is shared with a mutex or some other method, another task may acquire the bus and begin a transaction to a different device, all while the SD card's CS is still asserted.

[thejpster]

Yes, I know.

I'm just explaining why I wrote it the way I did. And I'm saying we can probably write it the "normal" way instead now.

[kalkyl]

Maybe the "spi initialization" could be enforced by something like this:

    pub struct SdCard<SPI: SpiDevice> {
        spi: SPI
    }

    pub struct UninitSdCard {
        _private: ()
    }

    impl UninitSdCard {
        pub fn init<SPI: SpiDevice>(self, spi: SPI) -> SdCard<SPI> {
            SdCard { spi }
        }
    }
    
    pub fn new<SPI: SpiBus, CS: Output>(spi_bus: &mut SPI, cs: &mut CS) -> UninitSdCard {
        // init procedure goes here
        UninitSdCard { _private: () }
    }

So you can only construct the SdCard from an already initialized bus:

    let sdmmc = sdmmc::new(&mut spi_bus, &mut cs);
    let spi_dev = ExclusiveDevice::new(spi_bus, cs, Delay);
    let sdcard = sdmmc.init(spi_dev);

[FriederHannenheim]

I would like to add that if it is decided to use an SpiDevice instead of an SpiBus, SdCard should not take a delay since SpiDevice already provides a Delay operation using the Transaction method and to construct an ExclusiveDevice for example one should also provide the delay. (The option to do it without a delay exists but comes with a warning that it may break)

I think taking a SpiBus, ChipSelect and the Delay is best since ExclusiveDevice also needs a CS pin where one currently has to provide DummyCSPin.
This is the code to construct a SdCard with esp-hal currently:

    let spi = spi::master::Spi::new(
        peripherals.SPI2, 
        HertzU32::kHz(400),
        spi::SpiMode::Mode0,
        &clocks
    ).with_pins(Some(sck), Some(mosi), Some(miso), NO_PIN);

    let spi_device = ExclusiveDevice::new_no_delay(spi, DummyCsPin).unwrap();

    let sd_card = SdCard::new(spi_device, cs.into_push_pull_output(), delay);

With SdCard taking a SpiBus you could skip the ExclusiveDevice step.

[kalkyl]

The reason we wanna use SpiDevice is (apart from semantics) to allow it to be used on a shared spi bus, which which won't work with the SpiBus trait. SpiBus is only meant to be implemented on the hal peripheral driver end, whereas drivers should use SpiDevice, for this reason.

[kalkyl]

(In my example above you can see i was using the Delay of the ExclusiveDevice, not a separate one)

[kalkyl]

The discussion is more about how to make use of the real SpiDevice cs pin instead of using a dummy pin and an external cs pin, that also breaks the shared bus compatibility. The original problem is that we need a way to initialize the sdmmc with a (temporary) non-standard usage of the cs pin, before it can be used as a normal SpiDevice

[felipebalbi]

I think a solution for this is paramount, specially with the amount of TFT + SD card cage cheap devices out there. Without a solution, we are forced to choose between using the SD card cage or the TFT display. Would it really be a big problem to require manual initialization of the SD card prior to creating a SpiDevice instance? @kalkyl's suggestion of forcing the behavior through the type system looks like a good idea.

[thejpster]

It is worth noting that this software is supplied without warranty, and you and I do not have a contract. I appreciate interest in this package, but nothing here is "paramount" for me.

If someone wants to do the work send a PR, I'd be happy to look at it. If not, I'll get around to it eventually.

If such changes are important to you, you can call Ferrous Systems and ask for a quote to get the work done sooner.

[thejpster]

I thought about using the transaction API to add the short delays in the retry/polling loop but I realised that would cause the CS pin to be active during the delay, which stops any other bus traffic occurring. So I left it taking a separate Delay object.

[thejpster]

Closed by #136