Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a generic Transport trait to support multiple SD card protocols #170

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

luojia65
Copy link

Currently, the embedded-sdmmc library only supports SPI-based SD card access. However, certain MCUs and SoC chips feature dedicated SD protocol peripherals that can provide faster and more efficient access. This PR abstracts various SD card protocols into a new Transport trait, enabling future extensions and custom transports by third-party libraries.

/// Abstract SD card transportation interface.
pub trait Transport {
    /// Read one or more blocks, starting at the given block index.
    fn read(&mut self, blocks: &mut [Block], start_block_idx: BlockIdx) -> Result<(), Error>;
    /// Write one or more blocks, starting at the given block index.
    fn write(&mut self, blocks: &[Block], start_block_idx: BlockIdx) -> Result<(), Error>;
    /// Determine how many blocks this device can hold.
    fn num_blocks(&mut self) -> Result<BlockCount, Error>;
    /// Return the usable size of this SD card in bytes.
    fn num_bytes(&mut self) -> Result<u64, Error>;
    /// Can this card erase single blocks?
    fn erase_single_block_enabled(&mut self) -> Result<bool, Error>;
    /// Check the card is initialised.
    fn check_init(&mut self) -> Result<(), Error>;
    /// Mark the card as requiring a reset.
    ///
    /// The next operation will assume the card has been freshly inserted.
    fn mark_card_uninit(&mut self);
    /// Get the card type.
    fn get_card_type(&self) -> Option<CardType>;
    /// Tell the driver the card has been initialised.
    unsafe fn mark_card_as_init(&mut self, card_type: CardType);
}

This pull request includes:

  • Refactor SPI-based SD card logic: moves existing SPI-specific functionality into a dedicated module, introduces SpiTransport as the default SPI transport implementation.
  • Introduce the Transport trait: defines a consistent interface for reading, writing, and managing SD card blocks, makes the codebase more flexible for additional SD protocols or hardware-accelerated peripherals.
  • SdCard wrapper changes: updates SdCard to hold a mutable reference to a Transport implementor, renames constructors to new_spi and new_spi_with_options (breaking) to clarify SPI usage.

These changes pave the way for supporting more advanced SD interfaces in hardware, improving performance and enabling community-driven protocol expansions.

r? @thejpster

@thejpster
Copy link
Member

This looks really interesting - thank you for the PR!

Did you have a specific SD Host Controller you wanted to implement Transport for?

@thejpster
Copy link
Member

Currently most of the user-facing types inside mod sdcard are re-exported at the top-level to make them easier to find. Should we also re-export sdcard::Transport and sdcard::SpiTransport?

@thejpster
Copy link
Member

thejpster commented Dec 22, 2024

Reading through this, I think a bunch of the implementation detail in the SpiTransport struct could be lifted out into the SdCard struct, and the Transport trait changed to offer:

  • fn card_command(&mut self, command: u8, arg: u32) -> Result<u8, Error>;
  • fn card_acmd(&mut self, command: u8, arg: u32) -> Result<u8, Error>;
  • fn read_data(&mut self, buffer: &mut [u8]) -> Result<(), Error>;
  • fn write_data(&mut self, buffer: &[u8]) -> Result<(), Error>;

For example, I believe this method would be exactly the same regardless of which SD Card transport was used:

/// Read the 'card specific data' block.
fn read_csd(&mut self) -> Result<Csd, Error> {
    match self.card_type {
        Some(CardType::SD1) => {
            let mut csd = CsdV1::new();
            if self.card_command(CMD9, 0)? != 0 {
                return Err(Error::RegisterReadError);
            }
            self.read_data(&mut csd.data)?;
            Ok(Csd::V1(csd))
        }
        Some(CardType::SD2 | CardType::SDHC) => {
            let mut csd = CsdV2::new();
            if self.card_command(CMD9, 0)? != 0 {
                return Err(Error::RegisterReadError);
            }
            self.read_data(&mut csd.data)?;
            Ok(Csd::V2(csd))
        }
        None => Err(Error::CardNotFound),
    }
}

But, I'd probably want to implement the Transport trait for something like the STM32 SD Host Controller before I'd know for sure, and I don't know that there's enough information in the Simplified SD Card Specification to do that, or if you need to see the full version.

@thejpster
Copy link
Member

I pushed a few comment fixes to better track where things now are.

@luojia65
Copy link
Author

luojia65 commented Dec 22, 2024

This looks really interesting - thank you for the PR!

Did you have a specific SD Host Controller you wanted to implement Transport for?

@thejpster: Thanks for your feedback! I'm currently working on the following:

  • SDH/SDIO peripheral for Bouffalo Lab chips (HAL support), which follows the standard SDHC register map.
  • SMHC peripheral for Allwinner chips (HAL crates), which uses a non-standard SDHC register layout.

Several community members are also interested in adding SD protocol support for chips like STM32, ESP32, and others.

Currently most of the user-facing types inside mod sdcard are re-exported at the top-level to make them easier to find. Should we also re-export sdcard::Transport and sdcard::SpiTransport?

Yes, I'd love to! If it's appropriate, I'll push a commit for this change. Edit: it have already been fixed in the latest commits, thanks.

Reading through this, I think a bunch of the implementation detail in the SpiTransport struct could be lifted out into the SdCard struct, and the Transport trait changed to offer:

* `fn card_command(&mut self, command: u8, arg: u32) -> Result<u8, Error>;`
* `fn card_acmd(&mut self, command: u8, arg: u32) -> Result<u8, Error>;`
* `fn read_data(&mut self, buffer: &mut [u8]) -> Result<(), Error>;`
* `fn write_data(&mut self, buffer: &[u8]) -> Result<(), Error>;`

For example, I believe this method would be exactly the same regardless of which SD Card transport was used:

/// Read the 'card specific data' block.
fn read_csd(&mut self) -> Result<Csd, Error> {
    match self.card_type {
        Some(CardType::SD1) => {
            let mut csd = CsdV1::new();
            if self.card_command(CMD9, 0)? != 0 {
                return Err(Error::RegisterReadError);
            }
            self.read_data(&mut csd.data)?;
            Ok(Csd::V1(csd))
        }
        Some(CardType::SD2 | CardType::SDHC) => {
            let mut csd = CsdV2::new();
            if self.card_command(CMD9, 0)? != 0 {
                return Err(Error::RegisterReadError);
            }
            self.read_data(&mut csd.data)?;
            Ok(Csd::V2(csd))
        }
        None => Err(Error::CardNotFound),
    }
}

But, I'd probably want to implement the Transport trait for something like the STM32 SD Host Controller before I'd know for sure, and I don't know that there's enough information in the Simplified SD Card Specification to do that, or if you need to see the full version.

I like the idea of reusing code from the SPI implementation. However, before proceeding, we should carefully review the SD card specification. There are notable differences between the SD and SPI protocols, such as variations in command reply types and initialization processes.

In fact, I attempted to provide a command-level abstraction in this branch. However, I found that while it decouples SPI and SD protocols to some extent, it also makes it harder to create a unified abstraction for both.

Further exploration of the full SD Host Controller Specification might be necessary, especially for implementing the Transport trait for more complex systems like the STM32 SD Host Controller.

I pushed a few comment fixes to better track where things now are.

Thank you!

@luojia65
Copy link
Author

@thejpster: If we're unsure which functions to include in this trait, could we mark the entire trait as experimental or unstable for now? We can stabilize it later once we've finalized the functions to retain in Transport.

@thejpster
Copy link
Member

Are you planning on making Transport implementations outside this crate? I guess we could merge an experimental trait.

I see that embassy-stm32 has a working SD Host Controller driver. I looked at it briefly but I might dig into it a more. The Pico SDK also has an experimental driver that uses a custom PIO program.

@luojia65
Copy link
Author

Are you planning on making Transport implementations outside this crate? I guess we could merge an experimental trait.

I see that embassy-stm32 has a working SD Host Controller driver. I looked at it briefly but I might dig into it a more. The Pico SDK also has an experimental driver that uses a custom PIO program.

@thejpster: Yes, I am planning to implement Transport in external crates. Having the trait available as an experimental feature would make it easier to prototype and test implementations for various platforms, such as those in embassy-stm32 or the Pico SDK. This approach would also allow us to refine the trait based on real-world use cases before finalizing its design.

@thejpster
Copy link
Member

Is a git dependency pointing at this branch sufficient?

@luojia65
Copy link
Author

Is a git dependency pointing at this branch sufficient?

I’d suggest merging it into the upstream instead. While it would still require a Git dependency until a new version is published, merging into upstream ensures the changes are more visible and accessible to the community, encouraging broader testing and feedback. This can help refine the Transport trait more effectively.

@thejpster
Copy link
Member

OK. How do you want to mark the trait as 'unstable' then?

@luojia65
Copy link
Author

luojia65 commented Dec 26, 2024

OK. How do you want to mark the trait as 'unstable' then?

@thejpster: Can we introduce an unstable feature, similar to the unproven feature in embedded-hal 0.2? We could then mark the following items with #[cfg(feature = "unstable")]:

  • pub use crate::sdcard::{Transport, SpiTransport} in lib.rs,
  • pub use spi::SpiTransport in sdcard.rs,
  • The SdCard::new<T>(T) constructor (not SdCard::new_spi)

This approach would allow experimental features to be gated while keeping the stable API intact.

@thejpster
Copy link
Member

Sounds OK to me. I guess the examples will tell us if we broke the API by doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants