Skip to content

sdmmc: add HAL traits for SD/MMC peripherals #662

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented Apr 23, 2025

Adds the initial implementation of the embedded-hal SD/MMC traits.

Includes types and traits useful for handling SD/MMC peripherals.

Adds the initial implementation of the `embedded-hal` SD/MMC traits.

Includes types and traits useful for handling SD/MMC peripherals.
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I started reviewing, but I think this PR needs a bit more work.

Did you get a chance to read https://github.com/rust-embedded/embedded-hal/blob/master/docs/how-to-add-a-new-trait.md?

This PR is missing some background work, and demonstration that it can be implemented on at least two targets.

Finally, is this worth abstracting over in eh? This feels like it could be a really great addition to the ecosystem, but equally how sure are we that we're going to cover every controller operation (and the fact you're trying to cover SPI commands here too) with these abstractions?

Perhaps a more worthwhile abstraction for eh would be a block device trait?

This PR might be better as a stand alone crate, at least initially, to demonstrate its viability.

type Error;

/// Gets whether the device is a MMC card.
fn is_mmc(&self) -> bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo all these bools should be enums, like CardType::Sd and TransportMode::Spi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I also prefer new-types, so I'll make those changes.

@rmsyn
Copy link
Contributor Author

rmsyn commented Apr 30, 2025

Did you get a chance to read https://github.com/rust-embedded/embedded-hal/blob/master/docs/how-to-add-a-new-trait.md?

Not yet, I missed it before opening the PR. Going over it now.

This PR is missing some background work, and demonstration that it can be implemented on at least two targets.

I'm currently implementing it on one platform that uses a DesignWare MMC controller. Should the second platform use a different controller type to meet the requirements?

Finally, is this worth abstracting over in eh?

I think so, but I'm open to differing opinions. SD/MMC controllers are common on SBC boards, and some other microcontrollers. Are there any examples of SD/MMC drivers that implement an e-h block device trait for a SD/MMC peripheral in a non-SPI mode?

the fact you're trying to cover SPI commands here too

That's not exactly the intention. How I imagine it working is that users will implement the SPI trait(s) for the controller/peripheral, and use those methods to implement the Mmc traits. I basically just want the Mmc traits to be generic enough to cover that use-case.

This PR might be better as a stand alone crate, at least initially, to demonstrate its viability.

I am currently working on the traits in the jh71xx-hal crate, and the driver crate that builds on that work in sdmmc-driver.

Adds the `CardType` enum to represent the device type for a SD/MMC
perpipheral.
@rmsyn
Copy link
Contributor Author

rmsyn commented May 1, 2025

Perhaps a more worthwhile abstraction for eh would be a block device trait?

The BlockDevice trait appears to be missing the critical init, set_ios, and setup_bus functions that abstract over the low-level register access necessary to properly perform device initialization. I don't think BlockDevice is going to work without modification to provide HAL traits that drivers can target for SD/MMC peripherals. Happy to be shown otherwise.

Adds the `CardMode` enum to represent the device mode for a SD/MMC
perpipheral.
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