Skip to content

Add SdCardSpiDevice trait #185

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 10 commits into
base: develop
Choose a base branch
from

Conversation

avsaase
Copy link

@avsaase avsaase commented Apr 10, 2025

Closes #184.

This only adds a blocking trait. Once #176 is merged I can rebase and add the async version. Or this can be merged first and the other PR can add the async version. Either is fine with me.

@thejpster
Copy link
Member

thejpster commented Apr 11, 2025

Seems reasonable enough, but I'd like to see some current users of embedded-sdmmc-rs port over to this style to check it's usable in practice.

@thejpster
Copy link
Member

There's also some overlap with #170, which is about SD Cards in general (i.e. using SD Card Host Controllers with the 4-bit bus) rather than this which is about SPI bus based SD Cards specifically.

avsaase and others added 2 commits April 11, 2025 19:44
Co-authored-by: Jonathan 'theJPster' Pallant <[email protected]>
@avsaase
Copy link
Author

avsaase commented Apr 11, 2025

Seems reasonable enough, but I'd like to see some current users of embedded-sdmmc-rs port over to this style to check it's usable in practice.

I'll also do some dogfooding by using it in my project. I'm mostly interested in the async API but I think I can do some tests with the block API.

There's also some overlap with #170, which is about SD Cards in general (i.e. using SD Card Host Controllers with the 4-bit bus) rather than this which is about SPI bus based SD Cards specifically.

How do you want to move forward with this? There hasn't been any recent activity on that PR. I think it makes sense to first fix the dummy byte issue.

@thejpster
Copy link
Member

If we're going to abstract the SD card transport it makes sense to do it in a way that both fixes the SPI issue and allows the use of SD Host Controllers.

I also have a mild preference for custom types over implementing traits on tuples. I think it makes it clearer what's going on.

Maybe something like:

  • struct VolumeManager uses a BlockDevice
  • SdCard implements BlockDevice using an SdCardTransport
  • RefCellSpiSdCardTransport implements SdCardTransport using a &RefCell
  • Stm32SdHostTransport also implements SdCardTransport
  • etc

@avsaase
Copy link
Author

avsaase commented Apr 12, 2025

Personally I don't have the need for a generic Transport trait. I understand why it would be useful to have but I think that's a separate thing from what this PR tries to a do. If @luojia65 is still interested in their PR I'm happy to discuss how we can combine efforts but I would like to limit the scope of this PR to just fixing the dummy byte issue.


- `embedded-hal-bus-03`: adds support for `embedded-hal-bus::spi::ExclusiveDevice`. This is probably the easiest way to use this crate when the SD card is the only device on the bus.
- `embassy-sync-06`: adds support for using a blocking mutex from the `embassy-sync` crate. This allows sharing the bus between multiple tasks.

```rust
use embedded_sdmmc::{SdCard, VolumeManager, Mode, VolumeIdx};
// Build an SD Card interface out of an SPI device, a chip-select pin and the delay object
Copy link
Member

Choose a reason for hiding this comment

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

This example needs to be kept in sync with the copy in readme-test.

@thejpster
Copy link
Member

Ok, fair enough.

@ValouBambou
Copy link
Contributor

ValouBambou commented Apr 30, 2025

Is this still planned? I wrote a patch to fix the build test checks and open a PR in @avsaase fork. I'm interested in trying to have the async API merged, so if you need help I will be happy to contribute!

Valentin Trophime and others added 2 commits April 30, 2025 17:26
@avsaase
Copy link
Author

avsaase commented Apr 30, 2025

Some (positive) life events happened so I couldn't work on this for a few weeks but I'm still planning on getting this merged. The last thing I did was to quickly whip up an async version of this PR to see how that would work. Unfortunately it did not work at all and consistently gave CRC errors. I haven't had the chance yet to see if this was a problem with this PR or my quick async version of it (it doesn't look like I pushed that code to github). In any case it's probably best to test the code from this PR first to see if the basic approach is correct. If you want to help with that that would be great.

@ValouBambou
Copy link
Contributor

I could test your 2 versions async and blocking next week if you want I would have access to real hardware (rp2040 + some SD cards). I will just need the async code. I'm not sure what is the best way to include the async code since the other PR is still waiting.

@avsaase avsaase changed the title Add SdCardDevice trait Add SdCardSpiDevice trait Apr 30, 2025
@avsaase
Copy link
Author

avsaase commented Apr 30, 2025

I pushed my async compatible version of this PR here: https://github.com/avsaase/embedded-sdmmc-rs/tree/bisync-sd-card-device.

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.

Sending 0xFF byte with CS deasserted when used on shared bus
3 participants