SD/MMC/FAT partition mouting with `esp:mount/4` and `umount` #1289

bettio · 2024-09-27T20:50:08Z

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

bettio · 2024-09-28T13:34:07Z

src/platforms/esp32/partitions.csv

-main.avm, data, phy,     0x210000,   0x100000
+boot.avm,  data, phy,     0x1D0000,    0x80000,
+main.avm, data, phy,     0x250000,   0x80000
+fatpart, data, phy, 0x2D0000, 0x100000


@petermm: honestly it is still totally WIP, I just pushed stuff for having a few CI runs ;)

petermm · 2024-09-28T19:03:26Z

confirmed working taking image and saving to sd card on esp32cam:

  def start() do
    :ok = :esp32cam.init()
    IO.inspect("Camera initialized.~n")
    :ok = :esp.mount("sdmmc", "/test", :fat, [])
    loop(1)
  end

  def loop(count) do
    {:ok, image} = :esp32cam.capture()
    path = "/test/img_" <> Integer.to_string(count) <> ".jpg"

    case :atomvm.posix_open(path, [:o_wronly, :o_creat, :o_excl], 0o644) do
      {:ok, fd} ->
        {:ok, l} = :atomvm.posix_write(fd, image)
        :atomvm.posix_close(fd)

      _ ->
        IO.inspect("some error")
    end

    IO.inspect("Captured image.  size=#{:erlang.byte_size(image)}")
    loop(count + 1)
  end

bettio · 2024-09-29T10:27:45Z

src/platforms/esp32/components/avm_sys/include/esp32_sys.h

@@ -114,6 +124,8 @@ struct ESP32PlatformData
 #endif
    mbedtls_ctr_drbg_context random_ctx;
    bool random_is_initialized;
+
+    struct SyncList misc_entries;


This not the most elegant solution maybe, but for sure avoids changing multiple times ESP32PlatformData.
I suggest keeping this for attaching structs that are not so frequently used, such as mount points, for which a linear search is good enough.

@pguyot on a second thought misc_entries might be not the best name, what about attachments_list?

bettio · 2024-09-29T10:29:20Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+            target, &host_config, &slot_config, &mount_config, &mount->handle.card);
+#endif
+
+    } else if (!strcmp(source, "sdspi")) {


Help for manual testing this might be appreciated.

I have tried with several devices but it seems none of the ones I own use SDMMC_HOST_DEFAULT() or SDSPI_HOST_DEFAULT() pins, so for wide spread compatibility these will need to be configurable.

bettio · 2024-09-29T10:29:50Z

confirmed working taking image and saving to sd card on esp32cam:

  def start() do
    :ok = :esp32cam.init()
    IO.inspect("Camera initialized.~n")
    :ok = :esp.mount("sdmmc", "/test", :fat, [])
    loop(1)
  end

  def loop(count) do
    {:ok, image} = :esp32cam.capture()
    path = "/test/img_" <> Integer.to_string(count) <> ".jpg"

    case :atomvm.posix_open(path, [:o_wronly, :o_creat, :o_excl], 0o644) do
      {:ok, fd} ->
        {:ok, l} = :atomvm.posix_write(fd, image)
        :atomvm.posix_close(fd)

      _ ->
        IO.inspect("some error")
    end

    IO.inspect("Captured image.  size=#{:erlang.byte_size(image)}")
    loop(count + 1)
  end

Can you re-run this test after my recent changes?

libs/eavmlib/src/esp.erl

src/platforms/esp32/components/avm_builtins/CMakeLists.txt

pguyot · 2024-09-29T11:31:02Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+        mount->misc_entry.entry_type = (uintptr_t) nif_esp_mount;
+        mount->base_path = strdup(target);
+        mount->mount_type = FATSPIFlash;
+        synclist_append(&platform->misc_entries, &mount->misc_entry.list_head);


Instead of altering platform data, why don't you use a resource? It would unmount if garbage collected, and would make iterating on the list of mount points unnecessary.

That was one of my thoughts, but I wanted to mimic the same semantic that exists on regular unix where disks are unmounted using the path as handle, so in case we generalize our API we are already support the same semantics.

But, after your feedback, I was thinking that one day we can store paths inside of refs on systems that uses paths as handles for unmounting.

Does it makes sense to you?

Definitely. If the system requires paths to unmount, it is unlikely very short on memory so we can maintain a copy around of the path in the resource. While on ESP32, a resource will consume a little less than the linked list with the copy of the path.

@pguyot on a 3rd thought there are other problems with this approach:

Basically open files are resources, so we need to make sure that they are closed and garbage collected before running umount.

I dind't check if umounting can cause errors when files are open, but I don't think there is an option to postpone destructor and running it again when all files have been closed, in order to try umount a second time

I'm not sure if performing I/O (that might also fail) from a resource destructor is a good idea, but here I can change my mind

In short explicit umount I think gives a better control over operation order and error handling

I never thought umount should only be implicit. It should be explicit and resource garbage collection should serve as a fallback. The same thing is true with files. Close should be explicitely called.

If necessary, we can increase the ref count of resources that need to be destroyed later. But in this case, it shouldn't be the case. It's a programming error to not explicitely call umount or close, but an error that doesn't leak with a resource (and does without a resource).

I think I fixed this comment.

src/platforms/esp32/components/avm_builtins/storage_nif.c

pguyot · 2024-09-29T12:10:58Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+
+        struct MountedFS *mount = malloc(sizeof(struct MountedFS));
+        mount->misc_entry.entry_type = (uintptr_t) nif_esp_mount;
+        mount->base_path = strdup(target);


I'm not sure this is freed, but if this is moved to a resource we probably no longer need it.

That is a memory leak for sure, indeed.

Also, we could avoid the allocation of strdup here by simply moving ownership of the pointer (e.g. base_path = target; target = NULL;).

Good point, fixed.

src/platforms/esp32/components/avm_builtins/storage_nif.c

src/platforms/esp32/components/avm_builtins/CMakeLists.txt

bettio · 2024-10-01T23:57:54Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+        return make_esp_error_tuple(ret, ctx);
+    }
+
+    mounted_fs->mount_type = Unmounted;


@pguyot I'm looking for a suggestion here, should I protect the MountedFS resource with some kind of lock?

After analysis, I believe we need some kind of protection because esp idf function esp_vfs_fat_spiflash_unmount_rw_wl doesn't look thread safe to me.

https://github.com/espressif/esp-idf/blob/46acfdce969f03c02b001fe4d24fa9e98f6adc5e/components/fatfs/vfs/vfs_fat_spiflash.c#L214

If we call it twice with the same value close enough, first core would free the pointer and second would try to free it before it's nullified.

The lock could be implemented around mount_type with a special umounting type, or just setting umounted before it's actually umounted, since we don't have the umounted => mounted transition.

At the end of the day I realized that using a SpinLock was guaranteed to be safe and headache free.

pguyot · 2024-10-02T20:20:37Z

libs/eavmlib/src/esp.erl

+    Source :: unicode:chardata(),
+    Target :: unicode:chardata(),
+    FS :: fat,
+    Opts :: list(proplists:property()) | #{atom() => term()}


proplists:proplist() is directly a list.

pguyot · 2024-10-02T20:22:07Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+
+    char *target = interop_term_to_string(target_term, &str_ok);
+    if (!str_ok || strlen(target) > 8) {
+        RAISE_ERROR(BADARG_ATOM);


Shouldn't we free source here before returning an invalid term?

Yes, and sorry for the pedantic repeated mistake, I think I fixed all of them.

src/platforms/esp32/components/avm_builtins/storage_nif.c

pguyot · 2024-10-02T20:25:49Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+
+        struct MountedFS *mount = malloc(sizeof(struct MountedFS));
+        mount->misc_entry.entry_type = (uintptr_t) nif_esp_mount;
+        mount->base_path = strdup(target);


Also, we could avoid the allocation of strdup here by simply moving ownership of the pointer (e.g. base_path = target; target = NULL;).

src/platforms/esp32/components/avm_builtins/storage_nif.c

pguyot · 2024-10-02T20:42:57Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+        return make_esp_error_tuple(ret, ctx);
+    }
+
+    mounted_fs->mount_type = Unmounted;


After analysis, I believe we need some kind of protection because esp idf function esp_vfs_fat_spiflash_unmount_rw_wl doesn't look thread safe to me.

https://github.com/espressif/esp-idf/blob/46acfdce969f03c02b001fe4d24fa9e98f6adc5e/components/fatfs/vfs/vfs_fat_spiflash.c#L214

If we call it twice with the same value close enough, first core would free the pointer and second would try to free it before it's nullified.

The lock could be implemented around mount_type with a special umounting type, or just setting umounted before it's actually umounted, since we don't have the umounted => mounted transition.

petermm · 2024-10-03T08:51:18Z

hmm :esp.mount("sdspi", "/test", :fat, spi_host: spi, cs: 5) returns {:ok, ""} now and then just stalls.

UncleGrumpy · 2024-10-03T22:42:50Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+    union
+    {
+        sdmmc_card_t *card;
+        wl_handle_t wl;


I'm curious why not use wear leveling for all mounts? It is supported on SD and flash, for both SPIFFS and FatFS.

It's not in the scope of this PR obviously, but I think it would be very beneficial to explore using a filesystem and wear leveling for the application partition, so we do not prematurely wear out the first few sectors of the app partition from heavy rewrites of the same few blocks at the beginning of the partition.

it depends on the kind of usage you are planning.
However I would rather iterate.

bettio · 2024-10-05T10:10:01Z

Tested with spiflash on both esp-idf v5 and v4.
It mounts correctly, opendir and open/write/close work correctly too.

petermm · 2024-10-05T10:20:14Z

Not working here sdspi:

  def start do
    spi_settings = [
      bus_config: [
        miso: 19,
        mosi: 23,
        sclk: 18,
        peripheral: "spi3"
      ]
    ]
    spi = :spi.open(spi_settings)
    IO.inspect("spi")
    ab = :esp.mount("sdspi", "/test", :fat, spi_host: spi, cs: 5)
    IO.inspect(ab)
    path = "/test/test.txt"
    IO.inspect("start")

    case :atomvm.posix_open(path, [:o_wronly, :o_creat, :o_excl], 0o644) do
      {:ok, fd} ->
        {:ok, l} = :atomvm.posix_write(fd, "My String")
        IO.inspect(:atomvm.posix_close(fd))
        IO.inspect("done")

      err ->
        IO.inspect(err)
        IO.inspect("some error")
    end

    :ok = :esp.umount("/test")

gives:


AtomVM init.
I (2616) sys: Loaded BEAM partition main.avm at address 0x250000 (size=1048576 bytes)
Starting application...
I (2746) spi_driver: SPI Bus initialized.
"spi"
I (2766) gpio: GPIO[5]| InputEn: 0| OutputEn: 1| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0 
{:ok, ""}
I (2846) gpio: GPIO[5]| InputEn: 1| OutputEn: 0| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0 
"start"
{:error, :enoent}
"some error"
CRASH

Don't understand the return value of :esp.mount being {:ok, ""}, the two gpio: GPIO[5] logs suggests to me the spi is being torn down somehow.. - worked before, but not currently (in my testing)

pguyot · 2024-10-05T11:36:52Z

libs/eavmlib/src/esp.erl

+%% @doc     Unmounts filesystem located at given path
+%% @end
+%%-----------------------------------------------------------------------------
+-spec umount(Target :: unicode:chardata()) -> ok.


Spec needs to be updated, as it's a resource now.

pguyot · 2024-10-05T11:37:15Z

libs/eavmlib/src/esp.erl

+    erlang:nif_error(undefined).
+
+%%-----------------------------------------------------------------------------
+%% @param   Target the path of the mounted filesystem that should be unmounted


Documentation needs to be updated as it's a resource now.

pguyot · 2024-10-05T11:38:15Z

libs/eavmlib/src/esp.erl

+    Target :: unicode:chardata(),
+    FS :: fat,
+    Opts :: proplists:proplist() | #{atom() => term()}
+) -> ok.


Spec of return type needs to be updated

pguyot · 2024-10-05T11:38:35Z

libs/eavmlib/src/esp.erl

+%% @param   Target the path where the filesystem will be mounted
+%% @param   FS the filesystem, only fat is supported now
+%% @param   Opts
+%% @returns ok in case of success, otherwise an error


Documentation of return type needs to be updated

Add missing `export_type([...])` and missing `proplists` type Signed-off-by: Davide Bettio <[email protected]>

Increase factory app partition size. Signed-off-by: Davide Bettio <[email protected]>

UncleGrumpy · 2024-10-07T14:42:36Z

libs/eavmlib/src/esp.erl

+    Target :: unicode:chardata(),
+    FS :: fat,
+    Opts :: proplists:proplist() | #{atom() => term()}
+) -> {ok, mounted_fs()} | {error, Reason}.


Not sure if this is related to the CI build error, but you might want to specify Reason :: term() here, and again below.

that's the quick typical lunch break change mistake !

UncleGrumpy · 2024-10-07T14:43:13Z

libs/eavmlib/src/esp.erl

+%% @doc     Unmounts filesystem located at given path
+%% @end
+%%-----------------------------------------------------------------------------
+-spec umount(Target :: mounted_fs()) -> ok | {error, Reason}.


See comment above.

petermm · 2024-10-07T18:04:54Z

src/platforms/esp32/components/avm_builtins/storage_nif.c

+        term_put_tuple_element(return_term, 0, OK_ATOM);
+        term_put_tuple_element(return_term, 1, mount_term);
+    }
+    enif_release_resource(mount);


not working sdmmc nor sdspi.

moving enif_release_resource(mount); down into the umount function makes it all work.

@petermm this surprises me, and probably @pguyot can give an hint here.

enif_alloc_resource gives a resources that has already the ref counter set to 1.

enif_make_resource increments it to 2

we need enif_make_resource at line 273 to get it back to 1, so it can GC later.

oh my, it appears to have been build gremlins somehow, despite everything being cleaned out:/

(changed down to otp26 and it started working, going back to otp27 kept it working :/)

Allow to mount and umount external storage such as SD/MMC or internal flash using `esp:mount/4` and `esp:umount/1`. Right now only `fat` filesystem is supported. Their semantic and parameters resembles unix mount and umount syscalls. Signed-off-by: Davide Bettio <[email protected]>

petermm

Tested successfully sdmmc and sdspi.

Adds a Storage section to the Programmers guide, with the recently added esp:mount/umount atomvm#1289 Signed-off-by: Peter M <[email protected]>

Add esp:mount/umount to programmers-guide.md Adds a Storage section to the Programmers guide Esp section https://www.atomvm.net/doc/main/programmers-guide.html#esp32-specific-apis , with the recently added esp:mount/umount #1289 Bit unsure of the Internal flash part, but should be good. These changes are made under both the "Apache 2.0" and the "GNU Lesser General Public License 2.1 or later" license terms (dual license). SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

bettio changed the title ~~Esp mount~~ WIP: Esp mount Sep 28, 2024

bettio commented Sep 28, 2024

View reviewed changes

bettio force-pushed the esp-mount branch 2 times, most recently from d52cf89 to 6784189 Compare September 28, 2024 17:01

bettio force-pushed the esp-mount branch from a20d82e to 6ef340c Compare September 29, 2024 10:23

bettio changed the title ~~WIP: Esp mount~~ SD/FAT partition mouting with esp:mount/4 and umount Sep 29, 2024

bettio changed the title ~~SD/FAT partition mouting with esp:mount/4 and umount~~ SD/MMC/FAT partition mouting with esp:mount/4 and umount Sep 29, 2024

bettio requested a review from petermm September 29, 2024 10:24

bettio commented Sep 29, 2024

View reviewed changes

pguyot reviewed Sep 29, 2024

View reviewed changes

bettio force-pushed the esp-mount branch from 2319174 to 9c2db19 Compare September 29, 2024 11:57

pguyot reviewed Sep 29, 2024

View reviewed changes

bettio force-pushed the esp-mount branch 2 times, most recently from 81410bb to 333b769 Compare September 29, 2024 18:37

petermm reviewed Sep 29, 2024

View reviewed changes

src/platforms/esp32/components/avm_builtins/storage_nif.c Show resolved Hide resolved

petermm reviewed Oct 1, 2024

View reviewed changes

src/platforms/esp32/components/avm_builtins/CMakeLists.txt Show resolved Hide resolved

bettio force-pushed the esp-mount branch from 333b769 to 38e426c Compare October 1, 2024 23:04

bettio commented Oct 1, 2024

View reviewed changes

bettio force-pushed the esp-mount branch from 38e426c to bcafabd Compare October 2, 2024 00:07

bettio requested review from pguyot and petermm October 2, 2024 00:07

pguyot reviewed Oct 2, 2024

View reviewed changes

bettio force-pushed the esp-mount branch 3 times, most recently from 378bfd9 to 2990608 Compare October 2, 2024 22:45

bettio requested a review from pguyot October 2, 2024 22:45

bettio force-pushed the esp-mount branch from 2990608 to 70686fe Compare October 3, 2024 18:59

bettio force-pushed the esp-mount branch 3 times, most recently from 100b5d4 to 20ee7d6 Compare October 3, 2024 22:01

UncleGrumpy reviewed Oct 3, 2024

View reviewed changes

bettio force-pushed the esp-mount branch from 20ee7d6 to 8f29d22 Compare October 3, 2024 23:15

bettio force-pushed the esp-mount branch from 8f29d22 to 24b7f24 Compare October 5, 2024 10:31

pguyot reviewed Oct 5, 2024

View reviewed changes

bettio added 2 commits October 7, 2024 14:16

proplists: make proplists:proplist/0 and proplists:property/0 avail

006d97f

Add missing `export_type([...])` and missing `proplists` type Signed-off-by: Davide Bettio <[email protected]>

ESP32 tests: make room for additional tests

5f3a78f

Increase factory app partition size. Signed-off-by: Davide Bettio <[email protected]>

bettio force-pushed the esp-mount branch from 24b7f24 to 60af272 Compare October 7, 2024 12:16

bettio requested review from pguyot and UncleGrumpy October 7, 2024 12:17

UncleGrumpy reviewed Oct 7, 2024

View reviewed changes

petermm reviewed Oct 7, 2024

View reviewed changes

bettio force-pushed the esp-mount branch from 60af272 to e9c0802 Compare October 7, 2024 21:46

bettio requested review from petermm and UncleGrumpy October 7, 2024 21:58

petermm approved these changes Oct 8, 2024

View reviewed changes

bettio merged commit 9be487f into atomvm:release-0.6 Oct 8, 2024
92 of 93 checks passed

petermm added a commit to petermm/AtomVM that referenced this pull request Oct 11, 2024

Add esp:mount/umount to programmers-guide.md

652408a

Adds a Storage section to the Programmers guide, with the recently added esp:mount/umount atomvm#1289 Signed-off-by: Peter M <[email protected]>

petermm added a commit to petermm/AtomVM that referenced this pull request Oct 11, 2024

Add esp:mount/umount to programmers-guide.md

b2c378d

Adds a Storage section to the Programmers guide, with the recently added esp:mount/umount atomvm#1289 Signed-off-by: Peter M <[email protected]>

petermm added a commit to petermm/AtomVM that referenced this pull request Oct 11, 2024

Add esp:mount/umount to programmers-guide.md

1375a99

Adds a Storage section to the Programmers guide, with the recently added esp:mount/umount atomvm#1289 Signed-off-by: Peter M <[email protected]>

petermm mentioned this pull request Oct 11, 2024

Add esp:mount/umount to programmers-guide.md #1311

Merged

petermm added a commit to petermm/AtomVM that referenced this pull request Oct 12, 2024

Add esp:mount/umount to programmers-guide.md

b44e4fa

Adds a Storage section to the Programmers guide, with the recently added esp:mount/umount atomvm#1289 Signed-off-by: Peter M <[email protected]>

SD/MMC/FAT partition mouting with esp:mount/4 and umount #1289

SD/MMC/FAT partition mouting with esp:mount/4 and umount #1289

Conversation

bettio commented Sep 27, 2024

Choose a reason for hiding this comment

petermm commented Sep 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UncleGrumpy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

bettio commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermm commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio commented Oct 5, 2024

petermm commented Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermm left a comment • edited Loading

Choose a reason for hiding this comment

SD/MMC/FAT partition mouting with `esp:mount/4` and `umount` #1289

SD/MMC/FAT partition mouting with `esp:mount/4` and `umount` #1289

UncleGrumpy Oct 8, 2024 •

edited

Loading

bettio Sep 29, 2024 •

edited

Loading

petermm commented Oct 5, 2024 •

edited

Loading

petermm left a comment •

edited

Loading