sdio: initial SDIO HAL implementation #3503

rmsyn · 2025-05-20T01:12:04Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs developer guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Provides the initial skeleton for the embedded_hal::mmc::MmcOps implementation for the SDIO peripherals on the ESP32C6 microcontroller.

Testing

Doc tests for now.

Integration tests via examples program is WIP.

MabezDev · 2025-05-20T13:21:49Z

esp-hal/src/sdio/esp32c6.rs

+    }
+
+    /// Releases the inner SDIO peripherals back to the caller.
+    pub const fn release(self) -> (SLC, SLCHOST) {


FYI this pattern goes against our guidelines in esp-hal: https://github.com/esp-rs/esp-hal/blob/main/documentation/DEVELOPER-GUIDELINES.md.

We use peripheral singletons with lifetime parameters, you can "borrow" (our method is called reborrow, but now I'm wondering if borrow makes more sense cc: @bugadani) a peripheral to allow drivers to automatically "release" the peripheral singleton again once the driver has been dropped.

I almost completely refactored using your advice, and the developer guidelines. Also took a look at some other peripheral implementations, and hopefully got closer to the intended style of esp-hal.

Still more to implement for the Info structs, and will likely add State + Configuration structs. Though, I'm not sure if those would find a better home inside the Sdio struct, or the peripheral Slc* structs.

MabezDev · 2025-05-20T13:22:33Z

esp-hal/src/sdio/result.rs

+//! Error and result types for SDIO peripherals.
+
+/// Convenience alias for the SDIO peripheral [Result](core::result::Result) type.
+pub type Result<T> = core::result::Result<T, Error>;


I don't think this is something that we want, if you need it for the eh impl then please inline it close to where the eh impl will live

I removed the Result alias, but did keep the Error type. Error currently only holds one variant, but the plan is to implement more for specific failure conditions for the hal::mmc implementation.

esp-hal/src/sdio/esp32c6.rs

esp-hal/src/sdio.rs

Dominaezzz · 2025-05-22T23:10:13Z

esp-hal/src/sdio.rs

+
+impl core::error::Error for Error {}
+
+impl<'d> MmcOps for Sdio<'d> {


Something is not quite right here. MmcOps is meant for the master side of this protocol, but you've implemented this on the slave peripheral.

I think you want the SD/MMC peripheral (#1928) and not the SDIO peripheral, right?

Something is not quite right here. MmcOps is meant for the master side of this protocol, but you've implemented this on the slave peripheral.

I think you want the SD/MMC peripheral (#1928) and not the SDIO peripheral, right?

Correct, the MmcOps trait is currently for the host, and I need to split the trait into potentially three traits:

MmcCommon

MmcHost

MmcDevice

I'll keep working on the e-h trait along with the work in this pull request. Thanks for the review.

rmsyn · 2025-05-30T04:12:48Z

Running the CI / * checks are failing, partially due to some weird dependency resolution.

As I get closer to a full implementation, I may need to bring in the embedded-hal::mmc::* stuff locally to avoid the git dependency.

What's strange is that running cargo clippy ... directly passes, but the xtask lint-packages can't resolve the embedded_hal::mmc::MmcCommon trait, and fails to build the embassy-hal-async examples.

I'll keep debugging.

bugadani · 2025-05-30T07:33:04Z

It would be best to not block this PR on embedded-hal, as it'll likely take months to stabilize a new set of traits. You should instead demonstrate new functionality in a new crate, like embedded-sdmmc in the embedded-hal repository, so that it can be released unstably and implemented by esp-hal and one other required implementor.

That way you wouldn't have to patch around the dependency issues with multiple embedded-hal versions in the same project.

MabezDev · 2025-05-30T11:32:55Z

To expand on @bugadani's comment, we can't merge this with git dependencies, so I would take the suggestion to split this into a separate crate and then work on upstreaming to eh proper. Feel free to iterate here in the mean time, but if you want this merged it won't be with a fork of embedded-hal.

rmsyn · 2025-05-31T00:04:51Z

It would be best to not block this PR on embedded-hal, as it'll likely take months to stabilize a new set of traits. You should instead demonstrate new functionality in a new crate, like embedded-sdmmc in the embedded-hal repository, so that it can be released unstably and implemented by esp-hal and one other required implementor.

This sounds like a good plan. I'll bring it up in next week's meeting, and see what the rest of the HAL team thinks.

To expand on @bugadani's comment, we can't merge this with git dependencies, so I would take the suggestion to split this into a separate crate and then work on upstreaming to eh proper. Feel free to iterate here in the mean time, but if you want this merged it won't be with a fork of embedded-hal.

Right, I had no intention of trying to get this PR merged with the git dependency. My original thought process was to prove the implementation here and in jh71xx-hal, iterate on the trait in embedded-hal, stabilize in embedded-hal, and take this PR out of Draft status after an embedded-hal release that includes the mmc module + traits.

However, the solution discussed above sounds like a better plan. We could do a embedded-hal-sdmmc v0.1.0-alpha.whatever release, and base the work off that crate until mmc stuff gets merged upstream (if ever).

Without trying to bikeshed too much, we would probably need to call it embedded-hal-sdmmc, since @thejpster already has embedded-sdmmc-rs. embedded-hal-sdmmc also keeps with the naming convention of embedded-hal-[async, io, nb, ...].

Uses the experimental `embedded-hal-sdmmc` crate for SD/MMC traits. See discussion for reasoning: - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)>

Provides the initial skeleton for the `embedded_hal::mmc::MmcOps` implementation for the SDIO peripherals on the ESP32C6 microcontroller.

Adds `Pins` + `Mode` type for SDIO GPIO pin configuration.

Adds the `State` type to represent the SDIO states + transitions.

Uses the experimental `embedded-hal-sdmmc` crate for SD/MMC traits. See discussion for reasoning: - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)>

rmsyn marked this pull request as draft May 20, 2025 01:12

MabezDev reviewed May 20, 2025

View reviewed changes

rmsyn force-pushed the esp32c6/sdmmc-impl branch 2 times, most recently from b7eeb4c to 2142d4d Compare May 22, 2025 03:31

Dominaezzz reviewed May 22, 2025

View reviewed changes

rmsyn force-pushed the esp32c6/sdmmc-impl branch 3 times, most recently from dddf388 to c3a7b0d Compare May 30, 2025 03:17

rmsyn force-pushed the esp32c6/sdmmc-impl branch from c3a7b0d to f017518 Compare May 30, 2025 04:14

rmsyn mentioned this pull request Jun 4, 2025

crate: transfer embedded-hal-sdmmc ownership rust-embedded-community/meta#35

Open

rmsyn added 3 commits June 4, 2025 02:06

sdio: initial SDIO HAL implementation

5fcb28e

Provides the initial skeleton for the `embedded_hal::mmc::MmcOps` implementation for the SDIO peripherals on the ESP32C6 microcontroller.

sdio: add GPIO pins configuration

d1796b6

Adds `Pins` + `Mode` type for SDIO GPIO pin configuration.

sdio: add peripheral state

c38418c

Adds the `State` type to represent the SDIO states + transitions.

rmsyn force-pushed the esp32c6/sdmmc-impl branch from fb552b8 to b0058c0 Compare June 4, 2025 02:06

rmsyn force-pushed the esp32c6/sdmmc-impl branch from b0058c0 to a53f104 Compare June 4, 2025 02:07

rmsyn force-pushed the esp32c6/sdmmc-impl branch from a53f104 to 5ec5792 Compare June 4, 2025 02:19

sdio: use experimental embedded-hal-sdio crate

ec42929

Uses the experimental `embedded-hal-sdmmc` crate for SD/MMC traits. See discussion for reasoning: - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)> - <esp-rs#3503 (comment)>

rmsyn force-pushed the esp32c6/sdmmc-impl branch from 5ec5792 to ec42929 Compare June 4, 2025 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

sdio: initial SDIO HAL implementation #3503

sdio: initial SDIO HAL implementation #3503

Uh oh!

rmsyn commented May 20, 2025

Uh oh!

MabezDev May 20, 2025

Uh oh!

rmsyn May 21, 2025 •

edited

Loading

Uh oh!

MabezDev May 20, 2025

Uh oh!

rmsyn May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Dominaezzz May 22, 2025

Uh oh!

rmsyn May 23, 2025

Uh oh!

rmsyn commented May 30, 2025

Uh oh!

bugadani commented May 30, 2025

Uh oh!

MabezDev commented May 30, 2025

Uh oh!

rmsyn commented May 31, 2025

Uh oh!

Uh oh!


		impl core::error::Error for Error {}

		impl<'d> MmcOps for Sdio<'d> {

sdio: initial SDIO HAL implementation #3503

Are you sure you want to change the base?

sdio: initial SDIO HAL implementation #3503

Uh oh!

Conversation

rmsyn commented May 20, 2025

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

Uh oh!

MabezDev May 20, 2025

Choose a reason for hiding this comment

Uh oh!

rmsyn May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MabezDev May 20, 2025

Choose a reason for hiding this comment

Uh oh!

rmsyn May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dominaezzz May 22, 2025

Choose a reason for hiding this comment

Uh oh!

rmsyn May 23, 2025

Choose a reason for hiding this comment

Uh oh!

rmsyn commented May 30, 2025

Uh oh!

bugadani commented May 30, 2025

Uh oh!

MabezDev commented May 30, 2025

Uh oh!

rmsyn commented May 31, 2025

Uh oh!

Uh oh!

rmsyn May 21, 2025 •

edited

Loading