Fix replying to default response with default response - #1882
TheJulianJES wants to merge 1 commit into
Conversation
`handle_cluster_general_request()` emitted a Default Response for any general command whose Disable Default Response bit was clear. An incoming Default Response (0x0B) is a general command, so a device that leaves the bit clear — which real devices do — got answered with a Default Response carrying `command_id=11`. ZCL R8 2.5.12.2 forbids this: criterion 1 is "A device receives a unicast command that is not a Default Response command", and the section closes with "the Default Response command SHALL not be generated in response to reception of another Default Response command". The exclusion is narrow — the same paragraph still requires a Default Response for other response commands such as Write Attributes Response — so only Default Response itself is excluded here. Beyond spec compliance, each spurious unicast takes the per-device concurrency slot. On a sleepy end device mid-OTA this inserted an extra frame between every image block response, and the APS ack for it could take seconds. Guard the generation site rather than `send_default_rsp()`, which is public, documented as sending unconditionally, and called directly by quirks.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1882 +/- ##
=======================================
Coverage 99.50% 99.50%
=======================================
Files 59 59
Lines 12413 12413
=======================================
Hits 12351 12351
Misses 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewed at 5c8bf54. Correct, correctly scoped, and well evidenced — the spec citation checks out verbatim, every load-bearing claim in the description reproduced, and all three suites are green. No blockers and nothing to address.
The spec text is unambiguous and the guard matches it exactly. ZCL R8 §2.5.12.2 closes with "The Default Response command SHALL be generated in response to reception of all commands, including response commands (such as the Write Attributes Response command), under the conditions specified above. However, the Default Response command SHALL not be generated in response to reception of another Default Response command." — so keying only on Default_Response is the whole exclusion, and widening it to other response commands would be a spec violation in the other direction. Guarding the generation site rather than send_default_rsp() is the right call for the reason given: that method's docstring promises unconditional behavior and quirks call it directly.
One mechanism detail worth recording — it confirms the description rather than contradicting it, and explains the frame counts.
Only unmatched Default Responses ever reach this code. Device.packet_received calls _maybe_match_response() first, and on a hit it returns before zcl_cluster.handle_message(); the ResponseKey is (endpoint, cluster, direction, TSN), so a Default Response that still has a live pending future for its TSN is consumed there and never reaches the tail. What falls through is the leftovers — a request already completed or timed out, one sent with expect_reply=False, or a duplicate. That is why the logs show 89 and 407 spurious frames rather than one per command sent, and it means the fix cannot regress request/response matching: it only touches frames that had already fallen out of that path.
Verified (9 checks)
- Spec quote — pulled §2.5.12.2 from ZCL R8 directly. Criterion 1 ("A device receives a unicast command that is not a Default Response command") and the closing sentence are both quoted accurately, including the part that requires a Default Response for other response commands.
- New test genuinely pins the fix — restored
dev'szigpy/zcl/__init__.pyunder the PR's test file: failsassert 1 == 0. With the fix: passes. - The wire frame decodes as described —
00 20 0b 05 01→Basic:DefaultResponse(command_id=5, status=Status.FAILURE), TSN 32,frame_type=GLOBAL_COMMAND,disable_default_response=0. Matches the log evidence exactly. - The exclusion did not widen —
test_handle_cluster_general_request_disable_default_rspandtest_handle_cluster_general_request_not_attr_reportboth still pass untouched, soReport_AttributesandWrite_Attributesstill get their Default Response. - zigpy — 1316 passed.
- zha — 1374 passed, this branch editable-installed over the pinned zigpy.
- zha-device-handlers — 4841 passed, 2 xfailed, same setup. (Count differs from the 4820 in the description only because quirks
devhas gained tests since; green either way.) - All six quirks overriding
handle_cluster_general_request— re-derived independently and the description's classification holds.develco/emi_han,sinope/lightandbosch/rbsh_trv0_zb_eudelegate tosuper()and inherit the fix;konke(returns unlessReport_Attributes),mli/tint(returns unlessWrite_Attributes) andxiaomi(a no-op body) never reached the tail to begin with. None depended on the old behavior. - #1870 does not conflict and does not cover this — checked at head e90e4c5, still the current head.
maybe_send_default_rsp()has exactly one caller, insidehandle_cluster_request, and its suppression keys off_command_owners_for(command), which is empty for a general command. Itshandle_cluster_general_requesttail is the unmodifiedif not hdr.frame_control.disable_default_response:— identical todev.
Two notes, neither an ask:
The forward note about folding this into maybe_send_default_rsp() if #1870 later extends it to the general-command path is the right instinct — two divergent conditions in the two paths is exactly the shape that rots. Worth carrying into whichever PR merges second.
I also checked whether zigpy's own outgoing Default Response could invite a reply and start a ping-pong: it cannot. general_command() defaults disable_default_response to True, and a probe on a server cluster confirms the emitted frame carries DDR=1. So there was never a loop risk here, only the wasted unicast the description describes.
A second opinion from GitHub Copilot (GPT-5.6 Sol, read-only) returned no findings.
| # ZCL R8 2.5.12.2: a Default Response is never sent in response to another | ||
| # Default Response | ||
| if ( | ||
| hdr.command_id != foundation.GeneralCommand.Default_Response |
There was a problem hiding this comment.
Worth knowing for anyone reading this guard later: only unmatched Default Responses ever get here.
Device.packet_received runs _maybe_match_response() before zcl_cluster.handle_message() and returns early on a hit, and the ResponseKey is (endpoint, cluster, direction, TSN) — so a Default Response that still has a live pending future for its TSN is consumed upstream and never reaches this line. What reaches it is the residue: a request already completed or timed out, one sent with expect_reply=False, or a duplicate.
That is a useful property for this guard — it means the change cannot affect request/response matching, only frames that had already fallen out of that path.
There was a problem hiding this comment.
@zigpy-review-bot If it's a bug, create an issue: It seems like the OTA process doesn't react on "failure" default responses, so it eventually just dies with the stale timer. Should this be addressed (to fail earlier)?
There was a problem hiding this comment.
Yes, it's a real bug — verified against dev at 083d14f. But I'd argue against failing on the first failure Default Response; details below.
Why zigpy never reacts. OTAManager.__enter__ registers callback_for_response listeners for exactly four Ota server (cluster) commands — query_next_image, image_block, image_page, upgrade_end (zigpy/ota/manager.py:79-120). A Default Response is a general command, so it matches none of those schema filters. And there's no pending future for it either: image_block_response is an Ota client command, so it goes out via client_command() → reply(), which defaults expect_reply=False and disable_default_response=True (zigpy/zcl/__init__.py:877, :1760). So it falls through _maybe_match_response into handle_cluster_general_request and is dropped — the same path this PR is fixing the reply side of. The transfer then ends only via _stall_callback → _finish(Status.TIMEOUT) (zigpy/ota/manager.py:125-127), and the status the device actually reported is discarded. Same gap for query_next_image_response and upgrade_end_response.
Reproduced — fake device answers our Image Block Response with DefaultResponse(0x05, FAILURE), then goes silent
-> we sent image_block_response tsn=3, disable_default_response=1
raw default response frame: 00 03 0b 05 01
RESULT: status=<Status.TIMEOUT: 148> after 0.32s (stall timer was patched to 0.3s), blocks sent=1
Note disable_default_response=1 on our side: zigpy asks the device not to respond, and it responds anyway — which §11.13.8.5 explicitly permits for error cases. The probe test is parked alongside the issue draft.
On failing earlier. ZCL R8 §11.13.8.5.1 makes a client-side Default Response a per-block rejection, not an abort: on a badly formatted Image Block Response "the client SHOULD ignore the command and SHALL send default response command with MALFORMED_COMMAND status to the server", and the client is then free to re-request the same offset. Nothing in §11.13.8.5 ends the transfer. Your own log agrees — 89 of these frames inside a single transfer that kept progressing in between (the 30 s stall timer didn't fire until the end), which is exactly reject-and-re-request. Aborting on the first one would kill transfers that would otherwise recover.
So what's actually worth doing is narrower: (1) stop discarding the status — log the rejection with the file offset, since today a failed OTA is a bare TIMEOUT with no hint the device was actively rejecting blocks; (2) fail on a stuck signature rather than a single rejection — N consecutive rejections with no forward progress, or a rejection never followed by another Image Block Request — which gives you the "fail earlier" behaviour with the device's own status; (3) treat MALFORMED_COMMAND separately, since §11.13.8.5.1's causes all accuse our own Image Block Response (data size vs. the client's maximum_data_size, offset past the image size, missing fields) rather than being something to retry. The frient device sends generic FAILURE, so that last rule wouldn't have caught it.
One implementation caveat: there's no clean seam for it today. callback_for_response listeners are device-scoped, and Device.packet_received hands listeners only (hdr, command) with no cluster context — so a matcher can't distinguish an Ota Default Response from a PollControl one, and command ids collide across clusters (your second log has PollControl:DefaultResponse(command_id=0, status=TIMEOUT) on the same device). Either the Ota cluster's general-command path feeds OTAManager directly, or the listener API gains cluster context.
Filed as #1883 with the trace, the probe test and the spec argument above.
2 participants
Proposed change
handle_cluster_general_request()emitted a Default Response for any general command whose Disable Default Response bit was clear. An incoming Default Response (0x0B) is a general command, so a device that leaves the bit clear — which real devices do — got answered with a Default Response carryingcommand_id=11.ZCL R8 §2.5.12.2 forbids this: criterion 1 is "A device receives a unicast command that is not a Default Response command", and the section closes with "the Default Response command SHALL not be generated in response to reception of another Default Response command". The exclusion is narrow — the same paragraph still requires a Default Response for other response commands such as Write Attributes Response — so only Default Response itself is excluded here.
Guard the generation site rather than
send_default_rsp(), which is public, documented"""Send default response unconditionally.""", and called directly by quirks. Changing that method's contract would be a surprise to downstream callers.Why it matters beyond spec compliance
Each spurious unicast takes the per-device concurrency slot (
Device concurrency (1) reached, delaying requestfollows immediately), and the APS ack for it can take seconds on a sleepy end device. During an OTA this inserts an extra frame between every image block response.To be clear about scope: this is a spec-compliance and overhead fix, not a fix for the OTA failure in the logs below. That had a separate root cause (fast polling never engaging because the device never sent a
PollControlcheck-in — territory of #1817 and #1849/#1850/#1851/#1852). This removes wasted traffic; it does not on its own make that upgrade succeed.Evidence
Observed on frient/Develco devices with
PollControl/Otaon endpoint 35, in two HA debug logs:0x2BF3, endpoint 35,Ota). The device sends raw00 20 0b 05 01=DefaultResponse(command_id=5, status=FAILURE)withdisable_default_response=0, and zigpy answersDefaultResponse(command_id=11, status=SUCCESS).0xAE74) — and there it also lands onPollControl, not justOta: an incomingPollControl:DefaultResponse(command_id=0, status=TIMEOUT)is answered withDefaultResponse(command_id=11, SUCCESS). So this is not OTA-specific.Why only the general-command path is guarded
handle_cluster_request()deliberately gets no equivalent guard.FrameControl.is_clusteris purelyframe_type == CLUSTER_COMMAND, and a frame only decodes as a Default Response underGLOBAL_COMMAND, so a Default Response never reaches the cluster path. Adding acommand_id != 0x0Bcheck there would instead suppress the legitimate Default Response owed to real cluster commands that happen to use ID0x0b.Relationship to #1870
#1870 (draft) addresses the same family of bug, but does not cover this case: its
maybe_send_default_rsp()is called only fromhandle_cluster_request, thehandle_cluster_general_requesttail there is byte-identical todev, and its suppression keys off registered command owners, which do not exist for a Default Response. Checked against heade90e4c5. The two changes do not conflict as they stand.Forward note for whoever rebases #1870: if
maybe_send_default_rsp()is later extended to cover the general-command path too, this guard should move into that method rather than leaving two divergent conditions in the two paths.Verified
test_handle_cluster_general_request_disable_default_rspstill assertsgeneral_command.call_count == 1for aReport_Attributeswith the bit cleared, andtest_handle_cluster_general_request_not_attr_reportasserts exactmock_callsequality includingDefault_Responsefor aWrite_Attributes. Both are untouched by this PR and both pass — a broader guard would have failed them.assert 1 == 0) and passes after. It uses the exact wire frame from the log (00 20 0b 05 01), mirroring the existingtest_handle_cluster_general_request_disable_default_rspshape.ruff check/ruff formatclean.handle_cluster_general_request— all six checked.develco/emi_han,sinope/lightandbosch/rbsh_trv0_zb_eudelegate tosuper()for non-Report_Attributescommands and so inherit the fix;mli/tint,xiaomiandkonkereturn early and never reach the tail. None relies on the previous behavior.Out of scope
§2.5.12.2's other rule — no Default Response for a command received in error via broadcast or multicast — is not addressed here. Worth a follow-up.