SD card (sdmmc driver) support

[huming2207]

Hi @BrianPugh

As per discussed in issue https://github.com/joltwallet/esp_littlefs/issues/34 , here's my attempt for adding SD card support for this library.

Meanwhile, please be aware that:

1. 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.
2. 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]

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]

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.

[huming2207]

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]

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]

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]

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]

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

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

[huming2207]

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

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

[BrianPugh]

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]

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]

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