-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
suit/transport/vfs: add VFS as source for firmware updates #18045
Conversation
a4e1cd6
to
bcfddf8
Compare
bcfddf8
to
59cfbca
Compare
Nice! I would only suggest splitting the transport/coap part frojm the transport/vfs side. |
03634a2
to
17ad4ee
Compare
I've moved the transport worker thread to it's own file, now the separation worker thread <-> CoAP transport is also cleaner. I would then also rename Should I do this here or in a separate PR? |
c610265
to
ba49ac5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a second look I like the new direction, there are some comments that are against code that was only moved around, but still... Main concern right now is that I'm not sure that the storage_helper
code or print_download
code should be in handlers_command_seq
, wouldn't it fit better in worker
or common
?
ba49ac5
to
8fe072d
Compare
6e5ea8e
to
1d2a9b9
Compare
974fb3c
to
3e65754
Compare
I think this is still unaddressed no? |
Do you agree with moving the printbar and helper code to the worker file? |
But it's not related to the worker thread, is it? |
Hmm maybe not, but it's not related to the common handlers either I think, I think its best to keep only SUIT manifest parsing in those files, maybe @bergzand has a better suggestion of where to place that? |
Personally, I would rather have an extra |
|
Hmm true I guess it's already a bit mixed up with |
@benpicco can you re-test after all the changes for good measure? I don't have hw with an sdcard right now.... |
Also I realize there is no changes to the README.md, would be good to have something there. But the README is already huge, so maybe documenting in the headers would be better? |
3e65754
to
d9114f8
Compare
Jup still works:
|
@fjmolinas is the documentation in the header sufficient? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK and GO
Contribution description
This allows to load firmware updates from files on local storage.
Testing procedure
Place the two slot files as well as the latest manifest on an SD card in the
fw/
folder.Issues/PRs references
includes #18038