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

Add wrapper for SD/SDIO/SDMMC driver and vFS #422

Merged
merged 38 commits into from
May 17, 2024

Conversation

AlixANNERAUD
Copy link
Contributor

Hi,
According to issue #420 .
Am I heading in the right direction ?

@AlixANNERAUD AlixANNERAUD changed the title Feature/sd fat Add wrapper for SD/SDIO/SDMMC driver and vFS Apr 23, 2024
Copy link
Collaborator

@Vollbrecht Vollbrecht left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Am I heading in the right direction ?

You did a great start we surly will get there!

For the SDMMC support we need to introduce some gated compilation. To my understanding, currently - SDMMC host hardware is only present in esp32 / esp32s3. Other devices such as a esp32c6 uses the SDIO api. The esp-idf docu is a bit confusing on this point. For a first hind you can look at the failed CI runs, that complain that for the specific risc targets the API is not there.

We could feature gate by esp variants directly or use some of the down handed information from esp-idf like SOC_SDMMC_HOST_SUPPORTED flag.

If you first only want to work on the sdmmc part is fine, but it would be extra nice if you could use the sdspi_host api as a fallback for targets that does not support sdmmc. Edit: you already provide both apis ❤️ so its just a matter of gating the sdmmc variant correctly. In concrete i talk about this sdmmc api vs this sdspi api

In either case we would gate the sdmmc api in sd module part you created, so we dont brake targets that dosn't have it.

src/sd/spi.rs Outdated Show resolved Hide resolved
@Vollbrecht
Copy link
Collaborator

can you have a look at the clippy output from the CI runs. If you have a particular question around something feel free to ask !

@AlixANNERAUD
Copy link
Contributor Author

AlixANNERAUD commented Apr 30, 2024

can you have a look at the clippy output from the CI runs. If you have a particular question around something feel free to ask !

Well, I'm having trouble finding documentation on how the cfg flags of target capabilities are defined. It seems they are exported from ESP IDF #define macros, but I can't figure out which one corresponds to the Rust equivalent of SOC_SDMMC_HOST_SUPPORTED (like esp_idf_bt_classic_enabled is the equivalent of SOC_BT_CLASSIC_SUPPORTED). I'm currently using esp_idf_sdmmc_host_enabled, but I'm not convinced it's the correct one.

@AlixANNERAUD
Copy link
Contributor Author

What is going on with the CI ? There's an issue with ldproxy installation.

@Vollbrecht
Copy link
Collaborator

What is going on with the CI ? There's an issue with ldproxy installation.

wired unrelated install error / i restarted the workflow

@Vollbrecht
Copy link
Collaborator

can you have a look at the clippy output from the CI runs. If you have a particular question around something feel free to ask !

Well, I'm having trouble finding documentation on how the cfg flags of target capabilities are defined. It seems they are exported from ESP IDF #define macros, but I can't figure out which one corresponds to the Rust equivalent of SOC_SDMMC_HOST_SUPPORTED (like esp_idf_bt_classic_enabled is the equivalent of SOC_BT_CLASSIC_SUPPORTED). I'm currently using esp_idf_sdmmc_host_enabled, but I'm not convinced it's the correct one.

The correct one should be something like esp_idf_soc_sdmmc_host_supported . You can always double check with running cargo rustc -- --print cfg. This will print out all flags that are available on your current used target.

Regarding the other CI error with respect to "use alloc::ffi::CString;" : We also use alloc flags for stuff that uses heap allocation - this is done so that the hal can technically build on no_std. Its mostly used as a dev helper to mark things explicitly for us where heap allocs occure on our site. You would also need gates here.

src/sd/spi.rs Outdated Show resolved Hide resolved
@AlixANNERAUD AlixANNERAUD requested a review from ivmarkov May 1, 2024 19:56
@AlixANNERAUD
Copy link
Contributor Author

AlixANNERAUD commented May 5, 2024

Has the binding between rust's std::fs and esp-idf vfs been done? @ivmarkov @Vollbrecht

@ivmarkov
Copy link
Collaborator

ivmarkov commented May 6, 2024

@AlixANNERAUD Not yet. Sorry for being slow with my feedback. Will return some more today.

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Holidays over here. :)

src/sd/host.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated
all(esp_idf_version_major = "5", esp_idf_version_minor = "0"),
all(esp_idf_version_major = "5", esp_idf_version_minor = "1"),
)))] // For ESP-IDF v5.2 and later
pub enum DelayPhase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

1 - Create a submodule pub mod config and move DelayPhase in there
2 - #[derive(Debug, Default, Copy, Clone, Eq, PartialEq)] and pick which item in the enum should be the default, if there is a meaningful default. Otherwise do not derive Debug
3 - as isize? Why?
4 - If you look at other drivers, we often implement e.g. From<sdmmc_delay_phase_t> for DelayPhase.

Copy link
Contributor Author

@AlixANNERAUD AlixANNERAUD May 9, 2024

Choose a reason for hiding this comment

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

  1. Ok but why deriving Default ? It doesn't makes sense here ?
  2. Since Rust enums appear to be encoded as isize while sdmmc_delay_phase_t is a c_uint, casting becomes necessary.

src/sd/mmc.rs Outdated Show resolved Hide resolved
src/sd/mmc.rs Outdated Show resolved Hide resolved
src/sd/spi.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated Show resolved Hide resolved
src/sd/host.rs Outdated Show resolved Hide resolved
examples/sd_spi.rs Outdated Show resolved Hide resolved
examples/sd_mmc.rs Outdated Show resolved Hide resolved
src/fs/config.rs Outdated Show resolved Hide resolved
src/sd/config.rs Outdated Show resolved Hide resolved
@AlixANNERAUD AlixANNERAUD requested a review from ivmarkov May 12, 2024 20:40
@ivmarkov
Copy link
Collaborator

@AlixANNERAUD I'll merge this shortly, but might do a round of final renames/changes post commit - hope you don't mind. For one, this code is not #[cfg()]-ed yet on the VFS and FAT components being enabled, so it will fail to compile when ESP IDF is compiled without support for VFS.

1 similar comment
@ivmarkov
Copy link
Collaborator

@AlixANNERAUD I'll merge this shortly, but might do a round of final renames/changes post commit - hope you don't mind. For one, this code is not #[cfg()]-ed yet on the VFS and FAT components being enabled, so it will fail to compile when ESP IDF is compiled without support for VFS.

@AlixANNERAUD
Copy link
Contributor Author

AlixANNERAUD commented May 17, 2024

@AlixANNERAUD I'll merge this shortly, but might do a round of final renames/changes post commit - hope you don't mind. For one, this code is not #[cfg()]-ed yet on the VFS and FAT components being enabled, so it will fail to compile when ESP IDF is compiled without support for VFS.

Thank you very much for your help, it's really kind of you to have taken the time to assist me. I've added the missing cfg for vfs/fat.

@ivmarkov ivmarkov merged commit 15253a4 into esp-rs:master May 17, 2024
12 checks passed
@Vollbrecht
Copy link
Collaborator

@AlixANNERAUD I'll merge this shortly, but might do a round of final renames/changes post commit - hope you don't mind. For one, this code is not #[cfg()]-ed yet on the VFS and FAT components being enabled, so it will fail to compile when ESP IDF is compiled without support for VFS.

Thank you very much for your help, it's really kind of you to have taken the time to assist me. I've added the missing cfg for vfs/fat.

Thanks for pushing the PR through!

@AlixANNERAUD AlixANNERAUD deleted the feature/sd_fat branch May 17, 2024 18:14
@niuhuan
Copy link

niuhuan commented Jun 24, 2024

my tf card init faild on esp32-s3 use this example .

#439

@AlixANNERAUD AlixANNERAUD mentioned this pull request Jul 12, 2024
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.

4 participants