Add wrapper for SD/SDIO/SDMMC driver and vFS

[AlixANNERAUD]

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

[Vollbrecht]

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]

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]

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

[Vollbrecht]

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

wired unrelated install error / i restarted the workflow

[Vollbrecht]

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.

[ivmarkov]

@Vollbrecht @AlixANNERAUD Once you feel this PR is mostly complete, would you let me know? I would also like to do a review w.r.t. naming, code conventions followed in the crate etc.

Please don't expect any large redesign requests, but stuff like:

• Renaming structs
• Putting const new here and there
• Putting Debug, Send etc. here and there

[ivmarkov]

One thing which is a tad more obtrusive and we might have to change is the whole Builder pattern. We just don't use it. What we have instead (primarily in esp-idf-hal) is the notion of Config (which IS actually a builder, but does not contain peripherals) + all peripherals directly passed to a new constructor at driver construction time.

But maybe let's solve the other issues first and leave the renames / consistency with the other code for the end. Up to you actually how you would like to tackle this.

[AlixANNERAUD]

@Vollbrecht @AlixANNERAUD Once you feel this PR is mostly complete, would you let me know? I would also like to do a review w.r.t. naming, code conventions followed in the crate etc.

Please don't expect any large redesign requests, but stuff like:

* Renaming structs

* Putting `const new` here and there

* Putting `Debug`, `Send` etc. here and there

Ok, I'm currently fixing CI errrors.

[AlixANNERAUD]

What we have instead (primarily in esp-idf-hal) is the notion of Config (which IS actually a builder, but does not contain peripherals) + all peripherals directly passed to a new constructor at driver construction time.

But maybe let's solve the other issues first and leave the renames / consistency with the other code for the end. Up to you actually how you would like to tackle this.

Okay, that makes sense for reducing the memory footprint. Do you have an example of that? It seems feasible to me.

[AlixANNERAUD]

@Vollbrecht @AlixANNERAUD Once you feel this PR is mostly complete, would you let me know? I would also like to do a review w.r.t. naming, code conventions followed in the crate etc.

Please don't expect any large redesign requests, but stuff like:

* Renaming structs

* Putting `const new` here and there

* Putting `Debug`, `Send` etc. here and there

I think you can start the review.

[ivmarkov]

What we have instead (primarily in esp-idf-hal) is the notion of Config (which IS actually a builder, but does not contain peripherals) + all peripherals directly passed to a new constructor at driver construction time. But maybe let's solve the other issues first and leave the renames / consistency with the other code for the end. Up to you actually how you would like to tackle this.

Okay, that makes sense for reducing the memory footprint. Do you have an example of that? It seems feasible to me.

It is not so much for memory footprint, as it is for consistency.

As for an example - every single driver in esp-idf-hal. Take CanDriver or any other.

[AlixANNERAUD]

What we have instead (primarily in esp-idf-hal) is the notion of Config (which IS actually a builder, but does not contain peripherals) + all peripherals directly passed to a new constructor at driver construction time. But maybe let's solve the other issues first and leave the renames / consistency with the other code for the end. Up to you actually how you would like to tackle this.

Okay, that makes sense for reducing the memory footprint. Do you have an example of that? It seems feasible to me.

It is not so much for memory footprint, as it is for consistency.

As for an example - every single driver in esp-idf-hal. Take CanDriver or any other.

Okay, but I will need to have a chunky new function because there are a lot of peripherals to be passed (up to 10 with slot configuration using GPIO matrix). Is that problematic?

[ivmarkov]

What we have instead (primarily in esp-idf-hal) is the notion of Config (which IS actually a builder, but does not contain peripherals) + all peripherals directly passed to a new constructor at driver construction time. But maybe let's solve the other issues first and leave the renames / consistency with the other code for the end. Up to you actually how you would like to tackle this.

Okay, that makes sense for reducing the memory footprint. Do you have an example of that? It seems feasible to me.

It is not so much for memory footprint, as it is for consistency. As for an example - every single driver in esp-idf-hal. Take CanDriver or any other.

Okay, but I will need to have a chunky new function because there are a lot of peripherals to be passed (up to 10 with slot configuration using GPIO matrix). Is that problematic?

My understanding is that the user - one way or another - does have to pass ALL of them when slot config is enabled? If yes, isn't this making the builder pattern anyway not so useful, as the builder makes sense only when it has initial state where the user can provide only a small fraction of the config or none at all (defaults). And only add to the builder what is different from the default. My understanding is this does not work for peripherals, in that all of them have to be passed to the driver anyway.

[ivmarkov]

@AlixANNERAUD I've left just one comment (for now) on the SpiDeviceBuilder, so that you can understand why I say that the "builder" pattern is not so useful w.r.t. peripherals. You really have to pass ALL of them. Otherwise, you cannot make sure - at compile time - that these very same peripherals are not used somewhere else.

[ivmarkov]

There are other issues that need addressing, like the fact that SpiDeviceBuilder needs to be lifetimed, as in SpiDeviceBuilder<'d> but we'll get there when we get there. :)

[AlixANNERAUD]

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

[ivmarkov]

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

[ivmarkov]

@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.

[ivmarkov]

@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]

@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.

[Vollbrecht]

@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!

[niuhuan]

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

https://github.com/esp-rs/esp-idf-svc/issues/439