Make `BlockSpi` take ownership of `SdMmcSpi`

[jbeaurivage]

The current BlockSpi API makes it quite difficult to pass ownership around because it borrows the underlying SdMmcSpi. This is especially unergonomic e.g. in RTIC, where a Controller could be initialized and is expected to be returned by the init function. See #56.

This PR proposes an alternative where BlockSpi takes ownership of the SdMmcSpi, while maintaining the typelevel guarantees that the card is properly initialized before a BlockSpi can be constructed.

This is achieved by using a try_init method on SdMmcSpi, which returns a Result<InitToken, Error>. This token can then be used in acquire to construct a BlockSpi. Since a succesful call to try_init and try_init_with_opts is the only (safe) way to acquire a InitToken, a BlockSpi is technically guaranteed to hold a correctly initialized card.

Initialization retries is also very easy using this API, because try_init* can be called repeatedly without having to reborrow the SdMmcSpi:

pub fn init_retries<SPI, CS>(mut spi: SdMmcSpi<SPI, CS>) -> BlockSpi<SPI, CS>
where
    SPI: embedded_hal::blocking::spi::Transfer<u8>,
    CS: embedded_hal::digital::v2::OutputPin,
    <SPI as embedded_hal::blocking::spi::Transfer<u8>>::Error: core::fmt::Debug,
{
    let mut maybe_init = spi.try_init();
    loop {
        match maybe_init {
            Ok(t) => return spi.acquire(t),
            Err(_) => maybe_init = spi.try_init(),
        }
    }
}

[thejpster]

I'm afraid I don't quite follow what doesn't work with the current API (it's been a year or two since I last used this crate).

Are you say the advantage with this change is that you can stash the token somewhere and call spi.acquire() again at your leisure?

[jbeaurivage]

Yeah, pretty much. The downside of the current API is that storing a mut reference to the SdMmcSpi makes it really hard to share a BlockSpi between different contexts (ie, interrupt handlers).

In my use case, the SD card is initialized in an RTIC init handler, then shared between multiple tasks. Therefore the initialization flow has to go like this:

• A BlockSpi has to hold a &'static mut SdMmcSpi since RTIC resources can't hold structs that specify non-static lifetimes
• Getting a 'static SdMmcSpi is doable, but definitely not very ergonomic
• Since BlockSpi wants a static reference, if it is dropped (eg, a failed acquire), the SdMmcSpi can't be borrowed again. So you have only one chance at calling acquire. If the card isn't plugged in yet for example, you won't be able to initialize it again.

This makes it not very friendly for cases where you would want to hot-plug a card when it comes online. The proposed approach also makes it much easier to pass a BlockSpi/Controller around, because it's no longer tied to the lifetime of the underlying SdMmcSpi. This is definitely desirable in an embedded context, where the driver may be used in different contexts, interrupt handlers, etc.

In general, I tend to see structs holding references as being much more usable as short-lived constructs. i2c-pio also has the same approach of driver-borrowing-peripheral, and from what I could gather from the RTIC matrix chat, at least one user was struggling with it.

[jonathanpallant]

I think I'm falling over the same issue, which I wrote up as #77

[thejpster]

I made a bunch of changes. Hopefully the new API works better for you?

[jbeaurivage]

@thejpster, I finally found the time to get a prototype up using v0.5. I think it definitely fits the bill better. One thing I think is still missing though: If I try to hotplug a card, the library seems to just loop forever until a card is finally detected. I'd like to be able to handle the failure case where a card isn't plugged in, and try to initialize at a later time. Maybe I'm just not using the API correctly though, but I think having a method that checks whether a card is present and returns a Result would be very beneficial. Or an acquire option that specifies the number of retries before giving up. Same with retrying to init the card (when we mark it as uninit).

If you want, I can close this PR as it seems to be obsolete and start a discussion in a new issue.

[thejpster]

It's definitely not supposed to loop forever, but I accept the number of retries can be adjusted.

In my application, I detect card removal and insertion, so I only activate the library when I know a card is fitted. However, I did also test by inserting a micro-SD-to-full-SD adapter which did not have an micro-SD card in it, so it fooled the system into thinking a card was inserted when it wasn't, and this took a little while to time out, but it did work.

Edit: yes, closing this and opening a new ticket about adjusting retry times makes sense.