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

suit: start worker thread on demand, make suit_handle_url() public #18551

Merged
merged 2 commits into from
Nov 30, 2022
Merged

suit: start worker thread on demand, make suit_handle_url() public #18551

merged 2 commits into from
Nov 30, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Sep 4, 2022 •
edited
Loading

Contribution description

Drop the need to call suit_worker_run() before calling suit_worker_trigger() - the thread only has a single purpose anyway.

Also make the suit_handle_url() function available to users who already have a sufficiently large stack, when no worker thread should be started at all.

Testing procedure

Follow the steps in examples/suit_update:

> suit: received URL: "coap://[2001:db8::1]/fw/suit_update/samr21-xpro/riot.suit.latest.bin"
suit_worker: started.
suit_worker: downloading "coap://[2001:db8::1]/fw/suit_update/samr21-xpro/riot.suit.latest.bin"
suit_worker: got manifest with size 485
suit: verifying manifest signature
suit: validated manifest version
)Manifest seq_no: 1669802730, highest available: 1669802619
suit: validated sequence number
)Formatted component name: 
Comparing manifest offset 1000 with other slot offset
Comparing manifest offset 20800 with other slot offset
validating vendor ID
Comparing 547d0d74-6d3a-5a92-9662-4881afd9407b to 547d0d74-6d3a-5a92-9662-4881afd9407b from manifest
validating vendor ID: OK
validating class id
Comparing 8818989e-a257-5994-ac9a-554b77898083 to 8818989e-a257-5994-ac9a-554b77898083 from manifest
validating class id: OK
Comparing manifest offset 1000 with other slot offset
Comparing manifest offset 20800 with other slot offset
SUIT policy check OK.
Formatted component name: 
riotboot_flashwrite: initializing update to target slot 1
Fetching firmware |█████████████████████████| 100%
Finalizing payload store
Verifying image digest
Starting digest verification against image
Install correct payload
Verified installed payload
Verifying image digest
Starting digest verification against image
Verified installed payload
Image magic_number: 0x544f4952
Image Version: 0x63872aea
Image start address: 0x00020900
Header chksum: 0x45bb3515

suit_worker: update successful
suit_worker: rebooting...
----> ethos: hello received
Failed to send flush request: Operation not permitted
gnrc_uhcpc: Using 4 as border interface and 0 as wireless interface.
gnrc_uhcpc: only one interface found, skipping setup.
main(): This is RIOT! (Version: 2022.10-devel-570-g15a43-suit_worker_cleanup)
RIOT SUIT update example application
Running from slot 1
Image magic_number: 0x544f4952
Image Version: 0x63872aea
Image start address: 0x00020900
Header chksum: 0x45bb3515

Starting the shell
>

Issues/PRs references

@benpicco benpicco requested a review from maribu September 4, 2022 14:02
@github-actions github-actions bot added Area: examples Area: Example Applications Area: OTA Area: Over-the-air updates Area: sys Area: System labels Sep 4, 2022
@benpicco benpicco added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 4, 2022
@benpicco benpicco force-pushed the suit_worker_cleanup branch from 10f2fef to 15a43f2 Compare September 5, 2022 05:47
@maribu
Copy link
Member

maribu commented Sep 5, 2022

How about using an event thread instead, which could also be reused for other stuff?

@benpicco
Copy link
Contributor Author

benpicco commented Sep 19, 2022 •
edited
Loading

My only concern there is that events are usually short tasks - if we'd use the event thread for installing the update, that would block other events for a long time.

I'm not sure if anything on the system level relies on the event thread though, so this would then only block user applications which is probably fine.

The main idea behind this is that we now export suit_handle_url(), so if the user already has a thread with a sufficient stack size, they can just use that and skip the dedicated worker thread.

@kaspar030
Copy link
Contributor

I'm not sure if anything on the system level relies on the event thread though, so this would then only block user applications which is probably fine.

Hm, that's one thing I like about the extra thread, that updating doesn't stop the application.

@benpicco
Copy link
Contributor Author

Well with this the user can just call suit_handle_url() from the event thread if they so desire - with #18656 just a single event_callback_oneshot() is needed.

So I think we don't have to mandate the event thread for this.

@benpicco
Copy link
Contributor Author

@maribu thank you!

@benpicco benpicco merged commit e18bc19 into RIOT-OS:master Nov 30, 2022
@benpicco benpicco deleted the suit_worker_cleanup branch November 30, 2022 10:42
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
@kfessel kfessel mentioned this pull request Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

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

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

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

Assignees
No one assigned
Labels
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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2023.01
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @maribu @kaspar030