SD card (sdmmc driver) support #170

huming2207 · 2024-01-09T05:55:59Z

Hi @BrianPugh

As per discussed in issue #34 , here's my attempt for adding SD card support for this library.

Meanwhile, please be aware that:

I haven't tested my work yet, I will test it later this week. But you may have a read for the code if you would like to.
The read_size, prog_size and block_size in LittleFS configuration are forced to set to the same size of SD card sector. AFAIK this is normally 512 to 1024 bytes for some small industrial solderable SD NAND chip like XTSD01GLGEAG, or 4096-8192 bytes for other modern SDHC/SDXC cards.

Please feel free provide suggestions if you have any. Thanks.

Regards,
Jackson

huming2207 · 2024-01-09T06:30:56Z

I just realised thte sdmmc library in ESP-IDF v4.4 doesn't support erase! 😅

This is a hard one - I think I have to either backport a erase function or just disable SD card support for v4.4 target. I'd prefer the second option as our company and myself won't use IDF v4.4 in 2024 at all and I prefer not to provide support for this ESP-IDF version.

What do you think? @BrianPugh

BrianPugh · 2024-01-09T06:59:19Z

I've converted this PR to a draft until you mark it as ready.

The read_size, prog_size and block_size in LittleFS configuration are forced to set to the same size of SD card sector.

I don't really see an issue with this, should I 😅 ?

This is a hard one - I think I have to either backport a erase function or just disable SD card support for v4.4 target. I'd prefer the second option as our company and myself won't use IDF v4.4 in 2024 at all and I prefer not to provide support for this ESP-IDF version.

I agree that disabling SD support in v4.4 is the way to go.

BrianPugh

High level feedback from skimming the code:

Should we hide SD card support behind a KConfig macro? I'm not sure if the compiler will optimize out sd-stuff otherwise.
If so, items like the new sdcard field in esp_vfs_littlefs_conf_t should be conditionally added. Similarly, the addition of sdmmc in CMakeLists.txt should be conditional.
Be sure to add unit tests! I've got an esp32 board that has a microsd slot, but I've honestly never used it. Would be good to check your work!

Looking pretty nice & clean so far!

huming2207 · 2024-01-09T23:13:27Z

Hi @BrianPugh , thanks for your feedback!

In regards to disabling the SD card support in v4.4, I've tried adding this in to Kconfig:

    config LITTLEFS_SDMMC_SUPPORT
        bool "SDMMC support"
        default y
        depends on IDF_VERSION_MAJOR >= 5
        help
            Toggle SD card support
            This requires IDF v5+ as older ESP-IDF do not support SD card erase.

and soon I realise depends on IDF_VERSION_MAJOR >= 5 will not work, as in the context of Kconfig there are no IDF_VER or IDF_VERSON_* variables. 😅

I guess I have to do that in CMake...

Jackson

BrianPugh · 2024-01-09T23:19:18Z

A slightly inelegant solution is to not have it depends on anything, and then raise a compilation warning in the code:

#if CONFIG_LITTLEFS_SDMMC_SUPPORT && IDF_VERSION_MAJOR  < 5
#error "SDMMC is not supported on esp-idf <5.0.0"
#endif

It doesn't have to be perfect since esp-idf 4.4 EOL is realtively soon (as you mentioned).

huming2207 · 2024-01-09T23:20:28Z

A slightly inelegant solution is to not have it depends on anything, and then raise a compilation warning in the code:

Yea sure, I'm doing this now.

Meanwhile I will let Espressif know about lacking the IDF_VER stuff.

huming2207 · 2024-01-09T23:32:53Z

A slightly inelegant solution is to not have it depends on anything, and then raise a compilation warning in the code:

Hmm, in this approach we still need to find a way to fix the CI, otherwise it will keep failing. So far I left default n to LITTLEFS_SDMMC_SUPPORT

huming2207 · 2024-01-10T01:17:53Z

@BrianPugh I've copied the demo code and implemented something like this:

    auto *sdmmc = sd.get_sdmmc_handle(); // This is our SD card manager returning its internal sdmmc_card_t handle

    esp_vfs_littlefs_conf_t conf = {
            .base_path = "/littlefs",
            .partition_label = nullptr,
            .partition = nullptr,
            .sdcard = sdmmc,
            .format_if_mount_failed = true,
            .read_only = false,
            .dont_mount = false,
            .grow_on_mount = true,
    };

    ESP_ERROR_CHECK(esp_vfs_littlefs_register(&conf));

    ESP_LOGI(TAG, "LittleFS Mounted");


    ESP_LOGI(TAG, "Opening file");
    FILE *f = fopen("/littlefs/hello.txt", "w");
    if (f == NULL)
    {
        ESP_LOGE(TAG, "Failed to open file for writing");
        return;
    }
    fprintf(f, "LittleFS Rocks!\n");
    fclose(f);
    ESP_LOGI(TAG, "File written");

    // Check if destination file exists before renaming
    struct stat st;
    if (stat("/littlefs/foo.txt", &st) == 0)
    {
        // Delete it if it exists
        unlink("/littlefs/foo.txt");
    }

    // Rename original file
    ESP_LOGI(TAG, "Renaming file");
    if (rename("/littlefs/hello.txt", "/littlefs/foo.txt") != 0)
    {
        ESP_LOGE(TAG, "Rename failed");
        return;
    }

    // Open renamed file for reading
    ESP_LOGI(TAG, "Reading file");
    f = fopen("/littlefs/foo.txt", "r");
    if (f == NULL)
    {
        ESP_LOGE(TAG, "Failed to open file for reading");
        return;
    }

    char line[128];
    char *pos;

    fgets(line, sizeof(line), f);
    fclose(f);
    // strip newline
    pos = strchr(line, '\n');
    if (pos)
    {
        *pos = '\0';
    }
    ESP_LOGI(TAG, "Read from file: '%s'", line);

    // All done, unmount partition and disable LittleFS
    esp_vfs_littlefs_unregister_sdmmc(sdmmc);
    ESP_LOGI(TAG, "LittleFS unmounted");

...and it works:

huming2207 · 2024-01-10T01:34:11Z

Hi @BrianPugh

In regards to this:

3. Be sure to add unit tests! I've got an esp32 board that has a microsd slot, but I've honestly never used it. Would be good to check your work!

I don't think I can implement this easily, as our company's board is with an uncommon SD NAND chip, similar to the XTSD01GLGEAG chip I mentioned above, and the procedure of creating the sdmmc_card_t requires a separate process which is confidential. I cannot submit that in this PR as it may breach the NDA. Meanwhile currently I also don't have a ESP32 dev board that comes with a SD card slot.

Since this implementation is done and proven working, if you would like to, you can review and approve this PR first. In the meantime I will see if I can buy a dev board and implement the unit test later.

Regards,
Jackson

BrianPugh

I did a first pass looking over it, it looks very good! My only request is to rearrange the macros a little bit to reduce duplicate code between the if/else macro statements.

Ill try and test this with some hardware this weekend (if I can figure out how to set up an sd card).

BrianPugh · 2024-01-10T02:28:04Z

CMakeLists.txt

+    if (CONFIG_LITTLEFS_SDMMC_SUPPORT)
+        list(APPEND pub_requires esp_partition sdmmc)
+    else()
+        list(APPEND pub_requires esp_partition sdmmc)


i don't think sdmmc should be in this else. It'd probably also be good to pull out the common stuff:

list(APPEND pub_requires esp_partition) if(IDF_VERSION_MAJOR GREATER_EQUAL 5) if (CONFIG_LITTLEFS_SDMMC_SUPPORT) list(APPEND pub_requires sdmmc) endif() else() list(APPEND pub_requires spi_flash) endif()

Oh yes, my bad, fixing now

Odd, this doesn't work:

It will result in fatal error: sdmmc_cmd.h: No such file or directory since somehow the sdmmc library isn't included. I've tried rebuilding after a full clean but problem still persists.

src/esp_littlefs.c

huming2207 · 2024-01-10T04:45:18Z

@BrianPugh somehow I couldn't get the CMake problem fixed as per I said in the comment. But I've put the sdmmc into PRIV_REQUIRES as a workaround instead.

huming2207 · 2024-01-15T00:12:27Z

Hi @BrianPugh , may I ask is there any updates on this?

BrianPugh · 2024-01-15T04:17:01Z

I'm spread a bit thin, but I was able to confirm that no original functionality has been negatively impacted, so I'm happy about that. The code you submitted is also clean and well written. Unfortunately, I dont have the time to personally confirm SD functionality. I don't see any obvious issues with the code, and it appears to be working for you.

So how about this: I merge this PR, and release v1.13.0. Would you be comfortable coming aboard with triage role to give supervision/code-reviews for SD-related issues?

huming2207 · 2024-01-15T04:21:11Z

So how about this: I merge this PR, and release v1.13.0. Would you be comfortable coming aboard with triage role to give supervision/code-reviews for SD-related issues?

Yea sure, I'm definitely happy to join!

For our company we are using this functionality for our products as well. So far we can confirm the basic read/write works just fine. If there's any issue, we can fix it (and we have to, as our products also depend on it 😅).

BrianPugh · 2024-01-15T04:30:12Z

excellent! Tagged v1.13.0 and added you to the project! Thank you very much for this PR and for joining!

huming2207 added 3 commits January 9, 2024 16:37

Initial implmentation for SD card support

15435bc

Make sure cache size is sufficient for SD card

430b175

Try fixing build for mainline ESP-IDF

bc1f1ed

huming2207 force-pushed the master branch from 5de6567 to bc1f1ed Compare January 9, 2024 06:22

BrianPugh marked this pull request as draft January 9, 2024 06:53

BrianPugh reviewed Jan 9, 2024

View reviewed changes

huming2207 force-pushed the master branch from 4d6e517 to 4a6d775 Compare January 9, 2024 23:40

sdmmc: allow toggling the SDMMC support

aa45b70

huming2207 force-pushed the master branch from 4a6d775 to aa45b70 Compare January 9, 2024 23:48

huming2207 added 2 commits January 10, 2024 11:32

Do a full erase when formatting SD card

1190584

sdmmc: implement format, VFS register & deregister

f2dbd2d

huming2207 added 2 commits January 10, 2024 12:22

Try fix linting error

86f15aa

Try fix compile error

f007e87

huming2207 marked this pull request as ready for review January 10, 2024 01:34

huming2207 added 2 commits January 10, 2024 14:39

sdmmc: implement get info API

dc62c84

sdmmc: implement mount check API for SDMMC

3e5830b

BrianPugh reviewed Jan 10, 2024

View reviewed changes

huming2207 added 3 commits January 10, 2024 15:20

Code cleanup as per review comment

536651b

Limit SD card size

74cb038

Temporary fix on CMake REQUIRES

ca0e139

BrianPugh merged commit 1083230 into joltwallet:master Jan 15, 2024
7 checks passed

BrianPugh mentioned this pull request May 4, 2024

Add support for mutliple backends #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SD card (sdmmc driver) support #170

SD card (sdmmc driver) support #170

huming2207 commented Jan 9, 2024

huming2207 commented Jan 9, 2024

BrianPugh commented Jan 9, 2024 •

edited

Loading

BrianPugh left a comment

huming2207 commented Jan 9, 2024

BrianPugh commented Jan 9, 2024

huming2207 commented Jan 9, 2024

huming2207 commented Jan 9, 2024

huming2207 commented Jan 10, 2024

huming2207 commented Jan 10, 2024

BrianPugh left a comment

BrianPugh Jan 10, 2024

huming2207 Jan 10, 2024

huming2207 Jan 10, 2024 •

edited

Loading

huming2207 commented Jan 10, 2024 •

edited

Loading

huming2207 commented Jan 15, 2024

BrianPugh commented Jan 15, 2024

huming2207 commented Jan 15, 2024

BrianPugh commented Jan 15, 2024

SD card (sdmmc driver) support #170

SD card (sdmmc driver) support #170

Conversation

huming2207 commented Jan 9, 2024

huming2207 commented Jan 9, 2024

BrianPugh commented Jan 9, 2024 • edited Loading

BrianPugh left a comment

Choose a reason for hiding this comment

huming2207 commented Jan 9, 2024

BrianPugh commented Jan 9, 2024

huming2207 commented Jan 9, 2024

huming2207 commented Jan 9, 2024

huming2207 commented Jan 10, 2024

huming2207 commented Jan 10, 2024

BrianPugh left a comment

Choose a reason for hiding this comment

BrianPugh Jan 10, 2024

Choose a reason for hiding this comment

huming2207 Jan 10, 2024

Choose a reason for hiding this comment

huming2207 Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

huming2207 commented Jan 10, 2024 • edited Loading

huming2207 commented Jan 15, 2024

BrianPugh commented Jan 15, 2024

huming2207 commented Jan 15, 2024

BrianPugh commented Jan 15, 2024

BrianPugh commented Jan 9, 2024 •

edited

Loading

huming2207 Jan 10, 2024 •

edited

Loading

huming2207 commented Jan 10, 2024 •

edited

Loading