Repository navigation
Do not abort OTA when the image notify cannot be delivered - #1909
Open
zigpy-review-bot wants to merge 2 commits into
Open
zigpy-review-bot wants to merge 2 commits into
zigpy-review-bot wants to merge 2 commits into
Conversation
Image notify is optional and sleepy end devices often cannot receive it, they are expected to query for new images on their own. Previously, a failed image notify finished the OTA with `FAILURE`, which tore down the session and made the device's own requests get `NO_IMAGE_AVAILABLE` or `ABORT`, even when the device had already started downloading the image during the notify retries. - A failed image notify is now logged and the OTA waits for the device to request the image, finishing with `TIMEOUT` if it never does. - The device's first OTA request cancels a still pending image notify, so the notify does not delay our responses to the device. - The device's first OTA request also restarts the stall timer, so the OTA can never wait forever once the device has started talking to us.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1909 +/- ##
==========================================
- Coverage 99.51% 99.49% -0.02%
==========================================
Files 61 61
Lines 12733 13158 +425
==========================================
+ Hits 12671 13092 +421
- Misses 62 66 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
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.
Fixes #1891.
A sleepy end device often can't receive the OTA Image Notify. It only polls its parent occasionally, and the coordinator may report a missing APS ack while the parent still holds the frame (analysis in #1891 (comment)). Image Notify is optional in the ZCL OTA spec; devices are expected to query for new images on their own.
Until now, a failed Image Notify finished the OTA with
FAILURE. That ended the session, so the device's own Query Next Image gotNO_IMAGE_AVAILABLEand its Image Block requests gotABORT, even when the device had already started downloading during the notify retries.Changes
MAX_TIME_WITHOUT_PROGRESS, 30 s). If it never does, the OTA finishes withTIMEOUT.Behavior change
A device that really is unreachable now ends with
TIMEOUTabout 30 s after the notify gives up, instead ofFAILUREright away. ZHA's update entity reports "Update was not successful" either way. During those 30 s,ota_in_progressstays set, so a second update attempt or a re-interview of that device is refused until then.Tests
New tests in
tests/ota/test_ota_manager.pysimulate a sleepy device whose Image Notify can't be delivered. They cover:TIMEOUT);TIMEOUT, no hang);All new tests except the last one fail on dev; the last one guards the new notify task handling. A test fails if any of these is removed: the device-request hook in the query or block handler, the notify cancellation, the first-request stall timer restart, or the cancellation of the notify task when the update itself is cancelled.
The two existing notify-failure cases in
tests/test_device.pynow assertTIMEOUTinstead of just "notSUCCESS".The full suite passes, also when merged with current dev.
Not in this PR