extmod: add ROMFS filesystem and VfsRom driver #16446

dpgeorge · 2024-12-19T00:29:41Z

Summary

This is split out from #8381. This PR includes just the minimal number of commits from #8381 to get ROMFS support added:

the core ROMFS filesystem definition
the corresponding VfsRom driver
ability to memory map files
ability to import .mpy files in-place from the ROMFS
enabled on unix and qemu ports
a unittest-based test for the functionality

What's missing is integration into each port. That's left for #8381.

Testing

This PR includes a comprehensive unittest for VfsRom that runs under unix and qemu ports, and gets full coverage of the VfsRom driver. It's also been tested as part of #8381.

github-actions · 2024-12-19T00:38:34Z

Code size report:

   bare-arm:    +4 +0.007% 
minimal x86:    +6 +0.003% 
   unix x64: +5240 +0.623% standard[incl +736(data)]
      stm32:    +8 +0.002% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +8 +0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32: +2210 +0.554% VIRT_RV32

codecov · 2024-12-19T00:52:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (4729a89) to head (c732041).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16446      +/-   ##
==========================================
+ Coverage   98.57%   98.59%   +0.01%     
==========================================
  Files         164      167       +3     
  Lines       21349    21617     +268     
==========================================
+ Hits        21045    21313     +268     
  Misses        304      304

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpgeorge · 2024-12-19T01:05:21Z

Ah, there's still two lines of code that are uncovered by the test. Will fix.

robert-hh · 2024-12-19T09:27:04Z

I tried to just add board specific files to this PR. But that did not work to get ROMFS. vfs.VfsRom is there, but at least the vfs.rom_ioctl() method is missing. And of course the changed mpremote.

robert-hh · 2024-12-19T09:50:52Z

This commit is missing:
5349f9207 2024-11-15 extmod/modvfs: Add rom_ioctl function.
Adding it reveals vfs_rom_ioctl(), and adding suitable board files then provide a ROMFS.
Compiling for boards w/o romfs support still works.

dpgeorge · 2024-12-19T23:23:20Z

Thanks @robert-hh for looking at this. As mentioned in the top comment, I left out the integration with the bare-metal ports. The aim with this PR is to add the VfsRom driver and integration with the import machinery so that .mpy files can be imported in-place. That's all tested with the new test added here, and enabled on unix and qemu for testing.

So this PR is a self-contained piece of work, to finalise the ROMFS filesystem itself and how it works with the memory mapping feature.

Once this is merged, #8381 will be rebased and adds the final pieces of the puzzle to get it working on the ports, and with mpremote.

projectgus

This looks really good to me! It's nice and clean, and I like how it's possible to construct a RomFs in RAM inside MicroPython and do tests from that. The unit tests are great example of what's possible with the new unittest support, as well.

While I was reading the code I was trying to think of pathological cases that might perform poorly, but couldn't think of any really. Obviously directories with a huge number of entries will be pretty slow, but there's unlikely to be any of them. Plus I like how you've made the format extensible so additional cache data or something else can be added in, later.

projectgus · 2024-12-20T03:14:57Z

extmod/vfs_rom.c

+    const uint8_t *filesystem_top;
+};
+
+static mp_uint_t extract_record(const uint8_t **fs, const uint8_t **fs_next) {


It doesn't mean much because of C's type system, but it is nice for the reader of code to see "this variable is a record kind" or "this functions returns a record_kind". So I'd suggest typedef-ing the enum as a record_kind_t and flagging the places an mp_uint_t is expected to be a record_kind_t, if possible.

I'm not sure it's possible to use the enum, because the FS should support arbitrary record kinds, eg they might be 32-bit numbers. If the C compiler sees that it's an enum with only a handful of possibilities, it may narrow the enum type to 8 bits, and hence a large and unknown record type might be truncated to 8 bits and alias a known one.

Instead, maybe it's better to change these constants to simple #define's, and then just have a typedef for record_kind_t to be mp_uint_t.

I added a typedef for record_kind_t and changed the enum to #define's (because it's part of the filesystem spec, IMO better to explicitly list the values for the records).

For once our old friend the C standard has good news! C99 6.4.4.3 Enumeration constants: "An identifier declared as an enumeration constant has type int." The storage size should be the same as mp_uint_t.

There is a gcc option, -fshort-enums that allows the non-standard behaviour you're describing. I just noticed that it is currently turned on for the nrf port (only), but I suggest we disable it there rather than accommodate it.

Our replies crossed, but that seems fine to me too. 😄

The storage size should be the same as mp_uint_t.

With nanboxing, mp_uint_t is uint64_t, which is bigger than int.

There is a gcc option, -fshort-enums that allows the non-standard behaviour you're describing. I just noticed that it is currently turned on for the nrf port (only), but I suggest we disable it there rather than accommodate it.

IMO this is a useful compiler option to reduce code size and RAM usage (more likely a benefit for RAM usage) and it would be best not to have to disallow the use of this option just for this bit of code. Eg third party ports may want to use this option, and I don't know how easy it is for us to warn against using it.

Our replies crossed, but that seems fine to me too. 😄

Haha, OK!

If you think it's fine, let's leave it as a typedef mp_uint_t record_kind_t.

I don't know how easy it is for us to warn against using it.

MP_STATIC_ASSERT(sizeof(some_enum) == sizeof(int)) would catch it, but I understand your reasoning. No worries.

extmod/vfs_rom.c

projectgus · 2024-12-20T03:40:27Z

extmod/vfs_rom_file.c

+                break;
+            case 't':
+                type = &mp_type_vfs_rom_textio;
+                break;


I hadn't noticed MicroPython treats invalid mode strings as 'r' before, but I see it's the case for all filesystems. I don't see it documented as a CPython difference, should we document it?

Ah, that's interesting.

Yes we should document this, or maybe fix it (raise an exception for unknown codes).

I've made a note to look into this in the future

OK, great.

Maybe it's worth making a helper function for all filesystems to use, which implements this switch/logic. Then at least the issue at hand is confined to a single location.

extmod/vfs_rom_file.c

py/persistentcode.c

tests/extmod/vfs_rom.py

projectgus · 2024-12-20T04:16:16Z

tests/extmod/vfs_rom.py

+)
+
+
+class VfsRomWriter:


Not a comment for this PR, but did you have any thoughts about how to unify this class and the one that will eventually be added in tools/mpremote/mpremote/romfs.py? Maybe it's a good excuse to have some metadata in the test file that tells run-tests to mount a directory from the host when running it?

I was trying to think of a good way to do this... but in the end the code in the test will only be the minimal code needed to create the test ROMFS. The code in mpremote's ROMFS implementation will eventually be more sophisticated, eg to support alignment. And probably we should have separate tests for that mpremote code, which tests the ROMFS writer.

Here, the aim is to test the VfsRom driver.

dpgeorge · 2024-12-20T04:27:42Z

Obviously directories with a huge number of entries will be pretty slow, but there's unlikely to be any of them.

The file lookup code will skip entire directories if the file/directory that's being searched for is not in that directory. So there is some amount of lookup-optimisation here already. But, yes, if a directory contains a lot of files and you want a file in that directory, it will be linear (number of files in the directory) in the search for the file.

This commit defines a new ROMFS filesystem for storing read-only files that can be memory mapped, and a new VfsRom driver. Files opened from this filesystem support the buffer protocol. This allows naturally getting the memory-mapped address of the file using: - memoryview(file) - uctypes.addressof(file) Furthermore, if these files are .mpy files then their content can be referenced in-place when importing. Such imports take up a lot less RAM than importing from a normal filesystem. This is essentially dynamically frozen .mpy files, building on the revamped v6 .mpy file format. Signed-off-by: Damien George <damien@micropython.org>

Allows an interned string to reference static/ROM data, instead of allocating it on the GC heap. Signed-off-by: Damien George <damien@micropython.org>

This allows accessing data directly in ROM if the reader supports it. Signed-off-by: Damien George <damien@micropython.org>

If the file can be memory mapped (because it responds to the buffer protocol) then return a memory-reader that directly references the ROM data of the file. Signed-off-by: Damien George <damien@micropython.org>

This adds an optimisation for loading .mpy files from a reader that points to ROM. In such a case qstr, str and bytes data, along with bytecode, are all referenced in-place in ROM. Signed-off-by: Damien George <damien@micropython.org>

Signed-off-by: Damien George <damien@micropython.org>

Provides full coverage of the VfsRom driver. Signed-off-by: Damien George <damien@micropython.org>

dpgeorge added the extmod Relates to extmod/ directory in source label Dec 19, 2024

dpgeorge requested a review from projectgus December 19, 2024 00:29

dpgeorge force-pushed the extmod-add-vfs-rom branch from 817a342 to 86d37c9 Compare December 19, 2024 03:50

dpgeorge added this to the release-1.25.0 milestone Dec 19, 2024

dpgeorge force-pushed the extmod-add-vfs-rom branch from 86d37c9 to 6e86086 Compare December 19, 2024 23:15

projectgus reviewed Dec 20, 2024

View reviewed changes

projectgus approved these changes Dec 20, 2024

View reviewed changes

dpgeorge added 8 commits December 23, 2024 13:04

py/qstr: Add qstr_from_strn_static() helper function.

a1c3210

Allows an interned string to reference static/ROM data, instead of allocating it on the GC heap. Signed-off-by: Damien George <damien@micropython.org>

py/reader: Provide mp_reader_try_read_rom() function.

f870e8d

This allows accessing data directly in ROM if the reader supports it. Signed-off-by: Damien George <damien@micropython.org>

extmod/vfs_reader: Add support for opening a memory-mappable file.

5e9dd4b

If the file can be memory mapped (because it responds to the buffer protocol) then return a memory-reader that directly references the ROM data of the file. Signed-off-by: Damien George <damien@micropython.org>

unix: Enable VfsRom on standard and coverage variants.

d9378c9

Signed-off-by: Damien George <damien@micropython.org>

qemu/mpconfigport: Enable VfsRom.

def9a37

Signed-off-by: Damien George <damien@micropython.org>

tests/extmod: Add VfsRom test.

c732041

Provides full coverage of the VfsRom driver. Signed-off-by: Damien George <damien@micropython.org>

dpgeorge force-pushed the extmod-add-vfs-rom branch from b5b1ca0 to c732041 Compare December 23, 2024 02:05

dpgeorge merged commit c732041 into micropython:master Dec 23, 2024
65 checks passed

dpgeorge deleted the extmod-add-vfs-rom branch December 23, 2024 02:35

dpgeorge mentioned this pull request Mar 4, 2025

Define vfs.rom_ioctl(), add romfs commands to mpremote, and implement ROM partition support on stm32, rp2, esp32, esp8266 #16857

Merged

Uh oh!

extmod: add ROMFS filesystem and VfsRom driver #16446

extmod: add ROMFS filesystem and VfsRom driver #16446

Uh oh!

Conversation

dpgeorge commented Dec 19, 2024

Summary

Testing

Uh oh!

github-actions bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dpgeorge commented Dec 19, 2024

Uh oh!

robert-hh commented Dec 19, 2024

Uh oh!

robert-hh commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpgeorge commented Dec 19, 2024

Uh oh!

projectgus left a comment

Choose a reason for hiding this comment

Uh oh!

projectgus Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgeorge commented Dec 20, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

robert-hh commented Dec 19, 2024 •

edited

Loading

projectgus Dec 20, 2024 •

edited

Loading