Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/suit/storage/vfs: initial import #17943

Merged
merged 5 commits into from Jun 7, 2022
Merged

sys/suit/storage/vfs: initial import #17943

merged 5 commits into from Jun 7, 2022

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Apr 14, 2022 •
edited

Contribution description

This PR extends suit/storage units with vfs. It also makes suit/storage use xfa to allow for easy declaration of storage units, e.g.: backup filesystem storage can be added:

XFA_USE(char*, suit_storage_files_reg);
XFA(suit_storage_files_reg, 0) char* _slot0_file = "/sda/SLOT2.txt";
XFA(suit_storage_files_reg, 0) char* _slot1_file = "/sda/SLOT3.txt";

Testing procedure

I'll adapt the suit/example_update examples to run on native and use this storage unit once #16771 is in.

Issues/PRs references

Somewhat related to #17962
Depends on #17941
Could use #16771

@github-actions github-actions bot added Area: OTA Area: Over-the-air updates Area: sys Area: System labels Apr 14, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 17, 2022
benpicco
benpicco reviewed Apr 17, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, just one question: How are the different SUIT storage backends identified?

sys/include/suit/storage/vfs.h Outdated Show resolved Hide resolved
sys/suit/storage/vfs.c Show resolved Hide resolved
@fjmolinas
Copy link
Contributor Author

Rebased to get the suit changes in

@github-actions github-actions bot added the Area: examples Area: Example Applications label Apr 22, 2022
@fjmolinas fjmolinas force-pushed the pr_suit_vfs_storage branch 3 times, most recently from 0ed0e04 to 3f0ab9f Compare April 22, 2022 11:53
@fjmolinas
Copy link
Contributor Author

I adapted the examples/suit_update test to also run on native... although I don't think murdock is able to run it.

@benpicco
Copy link
Contributor

benpicco commented Apr 22, 2022 •
edited

I think the Readme needs an update too, it only tells me how to load a file to RAM. (Also: s/suit coap/suit fetch coap/g)

I adapted the examples/suit_update test to also run on native... although I don't think murdock is able to run it.

It won't be able to create the tap device - but with gcoap_fileserver and socket_zep we should be able to run it on CI, just like tests/gnrc_rpl

@chrysn
Copy link
Member

chrysn commented Apr 24, 2022

The PR title and some commit messages are unclear about whether this is SUIT specific (sys/suit/storage) or not (sys/storage). Appears to be the former from a quick glance.

@fjmolinas fjmolinas changed the title sys/storage/vfs: initial import sys/suit/storage/vfs: initial import Apr 25, 2022
@fjmolinas
Copy link
Contributor Author

The PR title and some commit messages are unclear about whether this is SUIT specific (sys/suit/storage) or not (sys/storage). Appears to be the former from a quick glance.

fixed

@fjmolinas
Copy link
Contributor Author

I think the Readme needs an update too, it only tells me how to load a file to RAM.

@benpicco is addressing the missing README enough to get this in? Or will you want to wait for you to add the gcoap_fileserver based test?

@benpicco
Copy link
Contributor

Having a test for this is entirely orthogonal, I just want the readme update so I can test it myself.

@fjmolinas
Copy link
Contributor Author

Having a test for this is entirely orthogonal, I just want the readme update so I can test it myself.

Sorry I missed this comment, will address next week, sorry for the delay.

@fjmolinas fjmolinas requested a review from jia200x as a code owner June 1, 2022 07:10
@github-actions github-actions bot added the Area: doc Area: Documentation label Jun 1, 2022
@fjmolinas
Copy link
Contributor Author

Having a test for this is entirely orthogonal, I just want the readme update so I can test it myself.

@benpicco I just updated the README, there is actually very little to adapt from the regular RAM workflow in the README, only the install-id.

benpicco
benpicco reviewed Jun 1, 2022
View reviewed changes
examples/suit_update/main.c
/* add handled storages */
#if IS_USED(MODULE_SUIT_STORAGE_VFS)
XFA_USE(char*, suit_storage_files_reg);
#ifdef BOARD_NATIVE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why limit this to native?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just adding this storage for native in the test, same question can be asked for RAM storage

@benpicco
Copy link
Contributor

benpicco commented Jun 1, 2022

Looks good to me, feel free to squash.

@fjmolinas
Copy link
Contributor Author

squashed

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2022
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2022
@fjmolinas
Copy link
Contributor Author

Rebased to get the pthon fix in.

@benpicco
Copy link
Contributor

benpicco commented Jun 2, 2022 •
edited

Hm I think you only squashed and didn't rebase - the python test is still failing.

@fjmolinas
Copy link
Contributor Author

All green here @bergzand @benpicco!

@benpicco benpicco merged commit 6019925 into RIOT-OS:master Jun 7, 2022
@fjmolinas fjmolinas deleted the pr_suit_vfs_storage branch June 9, 2022 08:47
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications Area: OTA Area: Over-the-air updates Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fjmolinas @benpicco @chrysn