pkg/xipfs: add XIPFS as vfs module#21197
pkg/xipfs: add XIPFS as vfs module#21197GGuche-2XS-Univ-Lille wants to merge 20 commits intoRIOT-OS:2025.01-branchfrom
Conversation
GGuche-2XS-Univ-Lille
requested review from
PeterKietzmann,
jia200x and
miri64
as code owners
github-actions
bot
added
Area: network
|
Interesting! |
|
I think something with your rebase went wrong, you have two commits in the PR that are part of the master branch from benpicco and maribu. |
Added shell commands to create XIPFS executable files and to execute them. Demonstrator has been commented with an execution trace in examples/xipfs.
GGuche-2XS-Univ-Lille
force-pushed
the
2024.10-xipfs-pull-request
branch
from
1787749 to
c588492
Compare
github-actions
bot
removed
Area: network
Yeah I should have checked twice, I tried a a git rebase -i and dropped the two commits you cited. Is it ok with you ? |
|
Yes, it looks good now. Unfortunately I don't have a dwm1001 board for testing this. I'm not a maintainer, so this is not an official review. I took a quick look at the code and I think it would be possible to eliminate the need for dedicated board header files in your xipfs package. And the rest of the constants seem to be pretty much the same for all platforms. There is also a bug that it won't allow you to build for other boards than the Before compiling for the dwm1001After compiling for the dwm1001If the package only supports certain boards, it would be best to add a whitelist for supported boards: https://doc.riot-os.org/creating-an-application.html However I am not sure if that is intended to work for packages as well, I guess a maintainer could answer that. Adding But in general I think it would be better to add general support for all boards (that support flashpage). |
There are some dwm1001 boards deployed in the IoT-LAB testbed (in Lille and Toulouse sites). You can:
(I just tried and it still works!) |
|
For starters, thanks to @crasbe and @aabadie for their feeback; they are much appreciated. We were, and still are, quite newbies into the RIOT community. @crasbe : I am giving a try at the generic code path that you propose, ie relying to FLASH definitions in But now, I am facing an error from the build system where my CPU type is not found anymore : I've searched into RIOT only to find a definition in generated Am I missing something ? Thanks for reading. |
|
I think you don't have to make any significant changes to the original project to be compatible with the RIOT definitions. I can't reproduce the issue you're describing. The error comes from the RIOT/cpu/nrf52/include/cpu_conf.h Line 58 in 7027fd3 What's odd is that the RIOT/boards/dwm1001/Makefile.features Line 1 in 7027fd3 To reproduce the issue I included If you include You could try to run |
|
@crasbe I scratched my head a bit to get how the build system works, while keeping at the same time the riot integration code as a template for other ones. I don't know if changes will be ok for reviewers, but now when building xipfs for RIOT, xipfs relies on I compiled both Please, could you give it a try for your board (nRF52840DK) and let me know if the compilation works for you now ? |
|
I gave this a test with an nRF52DK devboard and the tests run successfully: Then I tested the example and it did work for small executables (10 bytes), but resulted in a stack overflow for larger executables (123 bytes). In this log, the file What's a bit odd is that |
|
@crasbe :
That's good news, and we are quite happy to have XiPFS running on other platforms than DWM1001. About the example.
This is why we provided the So to sum it up, as the example shows and tries to explain (too briefly), to have a viable executable file within XiPFS, please follows these steps :
About the > vfs ls /dev/nvme0p0
2025-02-21 10:35:39,308 # vfs ls /dev/nvme0p0
2025-02-21 10:35:39,308 # test
2025-02-21 10:35:39,309 # test2
2025-02-21 10:35:39,310 # test3//
2025-02-21 10:35:39,311 # total 2 files2 files + 1 directory are shown here. I need to confirm my guess to prevent from unwanted side effects, with other teammates on vacation this week. |
|
Perhaps you can add some of the remarks to the README in the examples folder. Especially the explanation how to create a .bin/.fae file should (also) be part of the README. I was totally expecting the Some remarks about the documentation: Currently this PR introduces two new Doxygen errors, which shouldn't be too hard to address. Furthermore I would propose adding some documentation to the headers, because the documentation that will be accessible from the website is not that verbose. This is your free space for "advertising" your XIPFS.
|
| void xipfs_nvm_write(void *target_addr, const void *data, size_t len) | ||
| { | ||
| flashpage_write(target_addr, data, len); | ||
| } |
There was a problem hiding this comment.
How do you determine the start sector of your filesystem / how do you avoid overwriting the firmware?
We have the AUX slot mechanism to reserve a portion of the internal flash for use by the application, maybe this also works for you?
There was a problem hiding this comment.
Hi @benpicco ,
Thanks for the question.
Historically, XiPFS has been started circa 2022/2023 and mimics what we have observed from other filesystems in RIOT at that time.
We are using the flashpage mechanism/API as depicted in the quoted code snippet.
This XiPFS macro initializes the mount point with respect to FLASH_WRITABLE_INIT.
To the best of our understanding, we were thinking that it would prevent from silent firmware corruption and provide safe flash allocation.
Now, we are not against trying to behave and make XiPFS support AUX slot mechanism.
However, after a quick reading, wrapping our head around it is not that obvious.
Could we discuss about it in RIOT's forum please ?
Either way, we'll try to get back to you asap.
There was a problem hiding this comment.
@benpicco
First of all, sorry for the delay; partly because I was on sick leave for a few days.
-
About the AUX mechanism :
it would have been a lot of changes to make XIPFS follow the way other filesystems in RIOT support AUX mechanism; something we could not afford.
However, we decided to provide a way to construct a vfs_xipfs_mount_t from a mtd_flashpage_t, and thus enabling XIPFS to be compatible with the AUX slot mechanism.
This feature is showcased in 'tests/pkg/xipfs`.
Tests are run once from flash_writable section and a second time from mtd_flash_aux_slot. -
We find out also that there was a memory fault when flushing an internal page buffer on the last page of the flash memory.
Long story short, we fixed the problem and we are now using aligned memory with flashpage memory API.
As a bonus, tests now runs 4 times faster than the previous version.
There was a problem hiding this comment.
@benpicco Do you agree with XIPFS changes about the AUX mechanism ?
|
@GGuche-2XS-Univ-Lille what's the current status? |
crasbe
added
Type: new feature
@crasbe Please feel free to give a try at this PR. The embedded executable should work. We were busy to make the file format evolve. We'll be looking as well to the CI failure, which seems to concern msp430 and 16 bits platforms at first sight. As a side note, is it allowed to kindly ping other reviewers to confirm that changes we made are ok with their remarks ? Thank you for your time ! |
|
Okay, I'll take a look at it again soon. About the CI failure on MSP430: this is caused by this line: On the MSP430, the values for -#define XIPFS_MAGIC (0xf9d3b6cb)
+#define XIPFS_MAGIC (0xf9d3b6cbUL)Of course you can ping maintainers, as long as it is not daily 🤣 |
|
About commit 8adcfe3, two fixes were done :
This commit is synchronized with modifications done on file format, and thus XiPFS Format can be used again to generate your own fae file. @crasbe I have two questions if you don't mind.
|
|
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
There are some compilation errors because the code assumes a 32 bit platform to run on. See inline for the compilation fixes.
But even with that, it will still not link due to:
/opt/riot-toolchain/msp430-elf/10.1.0-18/bin/../lib/gcc/msp430-elf/10.1.0/../../../../msp430-elf/bin/ld: /data/riotbuild/riotbase/tests/pkg/xipfs/bin/z1/xipfs.a: error adding symbols: file in wrong format
| vfs_mount_t vfs_mp; /**< VFS mount point */ | ||
| unsigned magic; /**< xipfs magic number */ | ||
| const char *mount_path; /**< mount point path */ |
There was a problem hiding this comment.
| vfs_mount_t vfs_mp; /**< VFS mount point */ | |
| unsigned magic; /**< xipfs magic number */ | |
| const char *mount_path; /**< mount point path */ | |
| vfs_mount_t vfs_mp; /**< VFS mount point */ | |
| uint32_t magic; /**< xipfs magic number */ | |
| const char *mount_path; /**< mount point path */ |
This will fix this particular issue:
In file included from /data/riotbuild/riotbase/sys/include/fs/xipfs_fs.h:29,
from /data/riotbuild/riotbase/tests/pkg/xipfs/main.c:23:
/data/riotbuild/build/pkg/xipfs/include/xipfs.h:63:21: error: conversion from 'long unsigned int' to 'unsigned int' changes value from '4191401675' to '46795' [-Werror=overflow]
63 | #define XIPFS_MAGIC (0xf9d3b6cbUL)
| ^
/data/riotbuild/riotbase/sys/include/fs/xipfs_fs.h:59:18: note: in expansion of macro 'XIPFS_MAGIC'
59 | .magic = XIPFS_MAGIC, \
| ^~~~~~~~~~~
| XIPFS_ASSERT(ret == 0); | ||
|
|
||
| /* test */ | ||
| ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0, 0xffffffff); |
There was a problem hiding this comment.
| ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0, 0xffffffff); | |
| ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0, -1); |
fixes this error:
/data/riotbuild/riotbase/tests/pkg/xipfs/main.c: In function 'test_xipfs_lseek_einval_whence':
/data/riotbuild/riotbase/tests/pkg/xipfs/main.c:573:48: error: overflow in conversion from 'long unsigned int' to 'int' changes value from '4294967295' to '-1' [-Werror=overflow]
573 | ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0, 0xffffffff);
| ^~~~~~~~~~
| ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0xffffffff, | ||
| SEEK_SET); |
There was a problem hiding this comment.
| ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0xffffffff, | |
| SEEK_SET); | |
| ret = xipfs_open(xipfs_nvme0p0, &desc, "/toto", | |
| -1, 0); |
fixes this error:
/data/riotbuild/riotbase/tests/pkg/xipfs/main.c: In function 'test_xipfs_lseek_einval_whence':
/data/riotbuild/riotbase/tests/pkg/xipfs/main.c:573:48: error: overflow in conversion from 'long unsigned int' to 'int' changes value from '4294967295' to '-1' [-Werror=overflow]
573 | ret = xipfs_lseek(xipfs_nvme0p0, &desc, 0, 0xffffffff);
| ^~~~~~~~~~
For development, we would recommend in general to track the |
|
@kfessel , @maribu , @miri64 : thanks a lot for your comments ! To tell you the truth, we just realized the following obvious fact : there is 0 chance for XiPFS to compile on a non ARM cpu for now, because of these assembly lines in the library. So to sum it up, for now, XiPFS needs two requirements :
The latter is already handled by XiPFS package Makefile.dep. Is that correct ? Thank you for your time and remarks. |
|
I am currently fixing the faulty CI checks in this PR, and most of them are straightforward to solve. There is this commit-msg check that is failing : Please correct me if I'm wrong, but is it wanted that fixup messages length are checked ? If I read correctly Moreover, on autosquash, those messages will disappear in the matching commit, rendering corresponding checks obsolete. Please, do you have any recommendations in such a case ? |
|
no need to care for commit title length error due to "fixup" as you said it will be gone anyway. and you are right we shall probably filter them from message length check (as there is already a warning in place to not accidentally merge fixups. the other tests are more relevant atm Murdock and Static tests |
…egration of xipfs
…IOT integration of xipfs
|
I am puzzled by this compilation failure. Probably something we do or don't do on our side; please let us know. It seems to be a cross-compilation instance with llvm toolchain for the samr21-xpro, guessed from the build title and the Excerpt : Do you have any ideas about what's going on, please ? PS : I'll be out of office from June 9 to June 23 2025, included, but I will reply as soon as I'm back. |
|
Dear RIOT reviewers (@kfessel, @miri64, @benpicco, @maribu, @mguetschow, @crasbe), Let's face it, this PR became quite awful. Would it be ok with you if I close this PR to open a new clean one ? Sorry for the noise, and please let me know if you agree with this proposal. |
|
Of course you can start with a new PR, no worries :) Working with git and GitHub can be quite a steep learning curve and unfortunately sometimes a clean start is the only good solution. |
10 participants
Contribution description
This PR aims at integrating the eXecutable In-Place FileSystem into RIOT operating system.
XIPFS implements regular filesystem storage manipulation capabilities, but also allows
to run code almost directly from the FLASH memory.
Just a little bit of RAM is used to relocate what's needed for the execution (data and bss).
It is provided like any other vfs module, and can be called through the defined vfs API.
Nonetheless, it can also be called directly through its own API (xipfs_* functions).
No matter the actual file size is, XIPFS deals with Flash pages granularity of 4KiB.
That means that with two files with different sizes less than 4 Kib,
vfs dfwill display8 Kib used.
Due to XIPFS design, directories and files are stored in a similar way.
Echoing the remark above, that also means that an empty directory will cost 4 Kib to the
Flash memory.
This implementation has been developed and tested on a Decawave DW1001 board (Quorvo).
However, the Flash memory handling relies on available periph/flashpage.h, and should thus
run on any platform supporting addressable Flash memory.
Testing procedure
Manual and automated tests are provided.
Both are great resources to showcase XIPFS api and abilities.
Manual tests also demonstrate the integration of XIPFS as a VFS submodule.
To run the automated tests :
pyterm, listening to the device.To run the manual tests :
Please open a terminal with a
pyterm, listening to the device.Open another one and move to the examples\xipfs directory.
Now, please enter the following in this shell :
Come back to the
pytermterminal.The first time XIPFS is run onto the board the resulting display should be similar to :
, or to
when XIPFS has already be run and has initialized the two mount points of the filesystem.
Now, please enter
helpexecis a more a demonstration than a regular shell command.However it will :
hello-world.bin, when none is stored onto/dev/nvme0p0;hello-world.binthen.executeis the XIPFS execution shell command to run an executable file.create_executableis the shell command to create an executable_file.Though
create_executabletakes a name and a bytesize, please remember that it will contain no code until filled by a write command.