fw_update: implement retry with backoff #703
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Visit the preview URL for this PR (updated for commit f18f360): https://golioth-firmware-sdk-doxygen-dev--pr703-szczys-fw-upda-r3e0ovzc.web.app (expires Fri, 20 Dec 2024 21:36:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a9993e61697a3983f3479e468bcb0b616f9a0578 |
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
0e88b3b to
051546f
Compare
szczys
force-pushed
the
szczys/fw-report-stability
branch
from
1d412b1 to
16fbd8c
Compare
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
051546f to
8507b7b
Compare
szczys
force-pushed
the
szczys/fw-report-stability
branch
from
16fbd8c to
5144bca
Compare
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
8507b7b to
31d5b66
Compare
Code Coverage
|
Codecov ReportAttention: Patch coverage is
|
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
31d5b66 to
505a7d0
Compare
szczys
force-pushed
the
szczys/fw-report-stability
branch
2 times, most recently
from
be31140 to
c60c269
Compare
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
2 times, most recently
from
ec86659 to
288f72d
Compare
szczys
force-pushed
the
szczys/fw-report-stability
branch
2 times, most recently
from
c82b93e to
867e299
Compare
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
288f72d to
83faa32
Compare
sam-golioth
approved these changes
Dec 12, 2024
There was a problem hiding this comment.
Nice work @szczys! I left one comment about rollover. We should think about it holistically across all the places where we're making comparisons or performing math using the result of golioth_sys_now_ms() so I don't think that should block merging this PR.
src/fw_update.c
Outdated
|
|
||
| static int32_t backoff_ms_before_expiration(struct fw_update_component_context *ctx) | ||
| { | ||
| uint64_t actual_duration = golioth_sys_now_ms() - ctx->last_fail_ts; |
There was a problem hiding this comment.
We may need to consider overflow on FreeRTOS. The API for golioth_sys_now_ms() returns a uint64_t, but it's calculated from the tick count which is a uint32_t (usually, technically it can be configured). Depending on the tick period this could rollover in as little as 49 days.
There was a problem hiding this comment.
I wrote this with rollover in mind. Though I did not realize we don't have access to the full bit-width of the type being return. Since I'm returning an int32 for the duration, why don't I just mask the uint64 tick count to uint32 before calculation? I think it will perform the same way.
szczys
force-pushed
the
szczys/fw-report-stability
branch
from
867e299 to
7ec533d
Compare
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
83faa32 to
5888e01
Compare
sam-golioth
approved these changes
Dec 13, 2024
Record the timestamp at which a download fails and the desired backoff duration in the component context. Add functions to set these variables after each failure, check them to see if the backoff duration has expired, and to reset them at the beginning and end of a component download operation. Stored and calculated values for backoff timing use uint32_t. While golioth_sys_now_ms() returns uint64_t, not all platforms (ESP-IDF) support that full bit-width. Signed-off-by: Mike Szczys <[email protected]>
Move the mutex lock/unlock functions inside of received_new_target_component() as it should never be called without guarding the manifest pointer which is also updated on another thread by the on_ota_manifest callback. Check stored target component against what was received with manifest and use along with the backoff_duration_ms time value in the context to detect when a component is already being downloaded. Reset the backoff values in the context when a new component is received from the manifest. Signed-off-by: Mike Szczys <[email protected]>
When awaiting a retry, use the timeout value of the manifest semaphore to wait until the appropriate backoff duration has passed before retrying a download. This allows a new manifest to be received and acted upon while awaiting a retry. Set up the component for a retry as soon as the download step has finished (whether it is a success or failure). The retry continues to be in effect until all validation and other steps of the fw_update process have been confirmed. Finally, reset the backoff values to indicate a retry is not necessary. Signed-off-by: Mike Szczys <[email protected]>
Use the new GOLIOTH_OTA_REASON_AWAIT_RETRY reason when reporting IDLE state during retry backoff period. Signed-off-by: Mike Szczys <[email protected]>
szczys
force-pushed
the
szczys/fw-update-retry-with-backoff
branch
from
5888e01 to
f18f360
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves https://github.com/golioth/firmware-issue-tracker/issues/699