Initialize all fields of esp_vfs_fat_mount_config_t

[tomvanbraeckel]

Leaving max_files uninitialized triggers ESP_ERR_NO_MEM at runtime because it's trying to allocate memory for an unspecified (but large) number of files.

In esp-idf_v4.3/components/fatfs/vfs/vfs_fat.c around line 160:

size_t ctx_size = sizeof(vfs_fat_ctx_t) + max_files * sizeof(FIL);
vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ff_memalloc(ctx_size);
if (fat_ctx == NULL) {
    return ESP_ERR_NO_MEM;
}

So this needs to be initialized properly.

[tomvanbraeckel]

Tested properly; I'm able to read/write/list files on the internal flash (fat) using the network file manager etc.

[tomvanbraeckel]

BTW, to test this, I patched this into rg_tool.py:

diff --git a/rg_tool.py b/rg_tool.py
index f4771ae9..9bee6db8 100755
--- a/rg_tool.py
+++ b/rg_tool.py
@@ -157,6 +157,11 @@ def build_image(apps, device_type, img_format="esp32"):
         table_ota += 1
         image_data += data + b"\xFF" * (part_size - len(data))

+    # add storage for RG_STORAGE_DRIVER == 4 // Internal flash
+    table_csv.append("storage, data, fat, , 4M,");

I might implement it as a proper --addpartition argument, so you can do:

./rg_tool.py --addpartition "storage, data, fat, , 4M," --target odroid-go build-img

Or would you prefer something like:

./rg_tool.py --fatstoragepartition 4M --target odroid-go build-img

Guidance is welcome!

[ducalex]

.max_files = 16, // must be initialized, otherwise it will use an uninitialized ("random") value, which can trigger ESP_ERR_NO_MEM if it's a big one

Do you know why max_files ends up being filled with garbage when not defined? I've had this issue with other structs in esp-idf but I was always under the impression that the C standard required unspecified struct members be default-initialized (aka zeroed)?

I might implement it as a proper --addpartition argument, so you can do:

Adding support to rg_tool is a good idea! I think your second example would be less error-prone.

PS: Can you base your future PRs on the dev branch please? It's not the end of the world but I prefer to keep master close to the latest stable release.

[tomvanbraeckel]

Good question!

So I went digging and you're right; when you don't specify the value, it is initialized to 0.

The failure actually comes from the code (ESP-IDF) doing a ff_memalloc(max_files * sizeof(bool)) which becomes an ff_malloc(0) which becomes a malloc(0), which returns NULL (failure) because allocating 0 bytes doesn't make sense.

So my comment about why it fails is wrong there, it's not because of uninitialized memory, it's due to allocating 0 bytes of memory. Also, no need to initialize .allocation_unit_size to zero as this is done by default.

Third, I see now that this max_files is the max open files, not the max total files. And as the other storage drivers use just "4", I assume that's enough and will use the same value.

So no urgency, but I should submit a new pull request (against the "dev" branch) to improve this a bit further. Apologies for the many pull requests... I need to learn to curb my enthusiasm ;-)

[ducalex]

Thanks for the investigation! I think your patch is still fine, being explicit is always better.

The maximum files open at once by any of retro-go app is 3 I think, so 4 might be close and maybe we should up to 5-8. As you've probably seen by now, keeping the number low just saves a bit of memory, it's not that critical.

By the way maybe we should have the flash driver always available? This capability doesn't really need to be decided at build-time.

To illustrate my point, this is what I mean: https://github.com/ducalex/retro-go/commit/4ed74d04edac7a09d5e8814d3b8b3e3800e6b4c6

If you think it's a good idea then you could include that change in your next PR (the linked commit is just an example, untested and maybe not the best structure).

[tomvanbraeckel]

My pleasure! Will do :-)

BTW, I've been working on retro-go for DAYS now, adding support for piezoelectric buzzer audio, as that's all we have in the Fri3D Camp badges. It's working! But I'm still tweaking it for quality etc!