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

Fix #507 - add missing sdmmc_host_get_dma_info for sdcard driver #508

Merged

Conversation

fangpenlin
Copy link
Contributor

@fangpenlin fangpenlin commented Jan 8, 2025

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.

Pull Request Details 📖

Fix #507 by adding the missing sdmmc_host_get_dma_info function pointer for the SD card host driver

Description

The get_dma_info function pointer here:

esp-idf-hal/src/sd.rs

Lines 384 to 390 in 8a11ef4

#[cfg(not(any(
esp_idf_version_major = "4",
all(esp_idf_version_major = "5", esp_idf_version_minor = "0"),
all(esp_idf_version_major = "5", esp_idf_version_minor = "1"),
all(esp_idf_version_major = "5", esp_idf_version_minor = "2"),
)))] // For ESP-IDF v5.3 and later
get_dma_info: None,

supposes to be pointing to sdmmc_host_get_dma_info based on what I saw from the SDMMC_HOST_DEFAULT macro. Without this func pointer, the first time calling it will crash due to calling a null pointer from here:

https://github.com/espressif/esp-idf/blob/d7d3eb3f0a99f069a7814b3c0824626383e4a511/components/sdmmc/sdmmc_sd.c#L95

Testing

Adding patch pointing hal repo to my PR this problem is gone:

[patch.crates-io]
esp-idf-hal = { git = "https://github.com/LaunchPlatform/esp-idf-hal", rev = "ec432eae61fdbeecaaf05a7bd33b8ffa738911ab" }

output:

I (416) sleep: Configure to isolate all GPIO pins in sleep state
I (423) sleep: Enable automatic switching of GPIO sleep configuration
I (431) main_task: Started on CPU0
I (441) main_task: Calling app_main()
I (441) gpio: GPIO[36]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 1| Pulldown: 0| Intr:0
I (451) gpio: GPIO[35]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 1| Pulldown: 0| Intr:0 
I (461) gpio: GPIO[37]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 1| Pulldown: 0| Intr:0
I (531) esp_idf_svc_ws_client_cross_thread_bug: File File { fd: 9 } created
I (531) esp_idf_svc_ws_client_cross_thread_bug: File File { fd: 9 } written with [72, 101, 108, 108, 111, 44, 32, 119, 111, 114, 108, 100, 33]
I (541) esp_idf_svc_ws_client_cross_thread_bug: File File { fd: 9 } seeked
I (551) esp_idf_svc_ws_client_cross_thread_bug: File File { fd: 9 } opened
I (561) esp_idf_svc_ws_client_cross_thread_bug: File File { fd: 9 } read: Hello, world!
I (561) esp_idf_svc_ws_client_cross_thread_bug: Entry: "DCIM"
I (571) esp_idf_svc_ws_client_cross_thread_bug: Entry: "FOO.TXT"
I (571) esp_idf_svc_ws_client_cross_thread_bug: Entry: "SYSTEM~1"
I (581) esp_idf_svc_ws_client_cross_thread_bug: Entry: "TEST.TXT"

you can also see the branch I created in the reproducing repo:

https://github.com/LaunchPlatform/esp-idf-hal-sd-get-info-dma-bug/tree/with-fix

@fangpenlin fangpenlin force-pushed the bugfix-507-missing-sd-get-dma-info-ptr branch from a18fce5 to ff88340 Compare January 8, 2025 07:02
@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 8, 2025

Is it not working without this callback provided, with V5.3?

@fangpenlin fangpenlin changed the title WIP: Fix #507 - add missing sdmmc_host_get_dma_info for sdcard driver Fix #507 - add missing sdmmc_host_get_dma_info for sdcard driver Jan 8, 2025
@fangpenlin fangpenlin marked this pull request as ready for review January 8, 2025 07:21
@fangpenlin
Copy link
Contributor Author

Probably won't work with v5.3. The get_dma_info function is called from multiple places in

https://github.com/espressif/esp-idf/blob/d7d3eb3f0a99f069a7814b3c0824626383e4a511/components/sdmmc/sdmmc_sd.c#L95

Any function relies on it will crash. I am not sure if there any cases this function won't be called (like DMA not available?)

@fangpenlin
Copy link
Contributor Author

The PR is ready for review, by the way I tried my best to run clippy, but there are many build errors. I have no time to resolve them all just for running clippy. This is just one line change, there shouldn't be anything new code smell added.

@fangpenlin
Copy link
Contributor Author

The platform I am using for testing this is esp32-s3-usb-otg

https://espressif-docs.readthedocs-hosted.com/projects/esp-dev-kits/en/latest/esp32s3/esp32-s3-usb-otg/user_guide.html

by the way

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 8, 2025

Thanks - merging.

BTW, if you are successfully running cargo build, there is no reason whatsoever cargo clippy to fail, as it is a subset of cargo build so to say.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 8, 2025

... just reporting more stuff.

@ivmarkov ivmarkov merged commit 3972bf3 into esp-rs:master Jan 8, 2025
15 checks passed
@fangpenlin fangpenlin deleted the bugfix-507-missing-sd-get-dma-info-ptr branch January 8, 2025 07:48
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.

SD card driver crash due to lack of get_dma_info function pointer after idf v5.3
2 participants