Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1888 +/- ##
==========================================
- Coverage 99.50% 99.48% -0.02%
==========================================
Files 59 59
Lines 12415 12433 +18
==========================================
+ Hits 12353 12369 +16
- Misses 62 64 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
One additional interoperability point for this PR: ZCL revision 8 §3.18.4 requires the Keep-Alive Read Attributes response to be APS-encrypted. In zigpy 2.2.0, This is separate from the unadvertised-client-cluster dispatch fix in this PR and from the device-specific quirk in zha-device-handlers#5349. Specification: ZCL revision 8, §3.18.4. |
|
@zigpy-review-bot Review, up to 6 review rounds. Consider |
zigpy-review-bot
left a comment
There was a problem hiding this comment.
The approach is sound: the coordinator is the server for 0x0025, so it should answer a read whether or not the client advertises its side, and the fallback is gated tightly (Client_to_Server only, coordinator must host the cluster on dst_ep). Probed on this head and on this head merged onto current dev, it answers the keep-alive read, APS-encrypted once #1889 is in (which also covers the ZCL R8 §3.18.4 encryption point raised earlier in this thread). One unguarded lookup needs a fix, the branch needs a rebase, and a few points are worth deciding before merge.
Must-address
- Frames to coordinator endpoint 0 now raise instead of being ignored. If a ZHA-profile frame arrives with
dst_ep=0(and a non-zerosrc_ep),coordinator.endpoints[0]is the coordinator'sZDOobject, so.in_clustersat device.py:818 raisesAttributeError: No such 'in_clusters' ZDO command._match_packet_endpoint_clusteronly catchesKeyError, andControllerApplication.packet_receiveddoesn't catch it either, so the exception leaves zigpy and lands in the radio library's receive callback (bellows'handle_callbacklogs it asException running handlerwith a traceback). Before this PR the same frame was dropped as "unknown cluster". It's a malformed frame, but an unhandled exception triggered by device traffic is worth a one-line guard, e.g. skip unlessisinstance(coordinator.endpoints[packet.dst_ep], zigpy.endpoint.Endpoint)(or rejectpacket.dst_ep == zdo.ZDO_ENDPOINTup front). - Merge conflict with dev.
tests/test_device.pyconflicts withtest_keep_alive_reply_aps_encryptionfrom #1889 (import block and the adjacent test). Keeping both tests resolves it; the full suite passes on the merge (1352 passed).
Worth deciding
-
Scope is wider than KeepAlive. The fallback applies to every server cluster the coordinator hosts: ep 1 has Basic, OnOff, Time, Ota, KeepAlive and IasAce (
zigpy/application.pyregister_endpoints), ep 2 has Basic. All of these were silently ignored before when the client side wasn't advertised. On the merged tree:- Time and Basic reads are now answered from the
handle_read_attribute_*handlers. That's an improvement. - An OTA Query Next Image gets
NO_IMAGE_AVAILABLE, and thenota.check_cluster_for_ota()runs an image lookup for the transient cluster. ItsOtaImageAvailableEvent(and theOtaQueryCacheUpdatedEvent) are emitted on an object nobody listens to. - A cluster command such as OnOff Toggle or an IasAce command gets a Default Response SUCCESS (unless the request disables the default response) even though no application handler ever sees it. ZHA's cluster handlers are bound to the real
in_clusters/out_clusters, not to the transient object.
If the intent is "answer what the coordinator itself can answer", restricting the fallback to global commands (or to Read Attributes) would keep the Time/Basic/KeepAlive wins without the other side effects. Otherwise, a sentence in the PR description saying this is accepted would help.
- Time and Basic reads are now answered from the
-
This doesn't cover a device that lists 0x0025 as an input cluster. For such a device the flipped-direction lookup at device.py:777-800 wins first, and the reply comes from the device's server
KeepAliveobject.Cluster.replyderives the direction fromis_client, so the Read Attributes Response goes out with the Client_to_Server direction bit, which a strict client will likely discard. This is pre-existing and not caused by this PR, but it matters for the Inovelli reports: VZM35-SN fw 1.07 advertised 0x0025 as an input cluster on ep 1 (zigpy/zha-device-handlers#3003). For a follow-up, the response direction could be taken from the request header (hdr.direction.flip()) instead of the cluster type. -
The reply leaves from the wrong coordinator endpoint when
src_ep != 1.Endpoint.replyuses the device endpoint id as both src and dst endpoint. A keep-alive read from device ep 2 to coordinator ep 1 is therefore answered from coordinator ep 2 (the ZLL endpoint, which doesn't host 0x0025). Every zigpy reply already does this, so it isn't new here. But the new gate checkspacket.dst_epwhile the response doesn't use it. Fine for the ep 1 → ep 1 case seen so far.
Coverage
The two lines codecov flags are the raises at device.py:816 (dst_ep not a coordinator endpoint) and device.py:819 (cluster not hosted). A small parametrized negative test (Server_to_Client, unknown dst_ep, dst_ep=0, unhosted cluster, no coordinator device) would cover them and pin the "still ignored" behaviour.
Field context
- SONOFF ZBMINIL2 is the confirmed case: home-assistant/core#181626, home-assistant/core#181690 (both closed) and home-assistant/core#181063 (open, a duplicate). It was fixed per-device by zigpy/zha-device-handlers#5349 (in zha-quirks 2.3.0, shipped via home-assistant/core#183665). This PR is the generic version of that quirk.
- Inovelli VZM31-SN on firmware 3.04: several users on the Inovelli forum report switches going unavailable under ZHA until air-gapped, one of them several times a day on HA 2026.10 / ZHA 2.3.0, where the response is already APS-encrypted. Inovelli staff pointed to this PR as a likely lead and shared a local patch script. It isn't confirmed yet that 3.0x sends keep-alive reads at all. In the 2.18 descriptor, ep 1's only output cluster is 0x0019, and no 3.04 debug log or diagnostics have been posted. The deciding evidence would be
Ignoring message on unknown cluster: 0x0025from a switch's NWK in a ZHA debug log. If a 3.0x descriptor instead lists 0x0025 as an input cluster, the point above applies and this PR alone won't fix it.
Verified (9 checks)
- PR head 0fd09f3: full suite 1320 passed. Head merged onto dev 2fdc4ab (conflict resolved by keeping both tests): 1352 passed.
- zha dev (c7c46bc) with the merged zigpy installed editable:
tests/test_device.py,test_gateway.py,test_update.py,test_alarm_control_panel.py,test_event.py,test_discover.py— 1067 passed. - Probe on the merged tree: unadvertised KeepAlive read → one Read Attributes Response,
tx_options=ACK|APS_Encryption, Server_to_Client, base 10 / jitter 300. The transient object is the registryKeepAliveclass, so #1889's class-levelaps_encryption = Trueapplies. - Probe: Server_to_Client frame, no coordinator device,
dst_ep=5, and broadcast to ep 0xFF are all still dropped with "Ignoring message on unknown cluster: 0x0025". - Probe: unknown
src_epis rejected earlier by_match_packet_endpoint_cluster("unknown endpoint"), soself.endpoints[packet.src_ep]at device.py:828 can't raise frompacket_received. - Probe: Time read →
time+local_timeanswered. OnOff Toggle (default response enabled) → Default Response SUCCESS. OTA Query Next Image →NO_IMAGE_AVAILABLE+check_cluster_for_ota(<transient Ota>). Image Block →ABORT. - Probe: device with 0x0025 in
in_clusters→ reply directionClient_to_Servervia the flipped-direction path. Device reading from ep 2 → replysrc_ep=2, dst_ep=2. - Coverage on this head: device.py lines 816 and 819 uncovered, matching the codecov report.
- Probe: ZHA-profile Read Attributes on 0x0025 with
src_ep=1, dst_ep=0→AttributeError: No such 'in_clusters' ZDO commandescapesdevice.packet_received.
|
|
||
| assert packet.src_ep is not None | ||
| ep = self.endpoints[packet.src_ep] | ||
| return Cluster.from_id(ep, packet.cluster_id, is_server=False) |
There was a problem hiding this comment.
This returns a transient client cluster for any hosted server cluster, so besides KeepAlive it also handles OTA queries (triggering check_cluster_for_ota on an object with no listeners) and acks cluster commands like OnOff Toggle with a Default Response SUCCESS that no application handler saw. If only coordinator-answerable requests are intended, consider gating on hdr.frame_control.is_general (or on Read Attributes) here.
| if packet.dst_ep not in coordinator.endpoints: | ||
| raise KeyError(packet.cluster_id) | ||
|
|
||
| if packet.cluster_id not in coordinator.endpoints[packet.dst_ep].in_clusters: |
There was a problem hiding this comment.
With packet.dst_ep == 0 this is the coordinator's ZDO object, and .in_clusters raises AttributeError (probed), which nothing in zigpy catches (bellows ends up logging it with a traceback). Please guard on the endpoint type (or reject the ZDO endpoint) so the frame is ignored as before.
| coordinator = app.devices[app.state.node_info.ieee] | ||
|
|
||
| if packet.dst_ep not in coordinator.endpoints: | ||
| raise KeyError(packet.cluster_id) |
There was a problem hiding this comment.
The raises here and at 819 are the two uncovered lines. A parametrized negative test (unhosted cluster, unknown dst_ep, dst_ep=0, Server_to_Client, no coordinator device) would cover them.
| except KeyError: | ||
| pass | ||
| else: | ||
| LOGGER.debug( |
There was a problem hiding this comment.
Related, not introduced here: a device that lists 0x0025 as an input cluster lands in this flipped-direction branch before the new fallback. The reply then comes from its server-type KeepAlive, and Cluster.reply sets the Client_to_Server direction bit on the Read Attributes Response (probed). Inovelli VZM35-SN fw 1.07 advertised it that way (zigpy/zha-device-handlers#3003).
| assert packet.dst.address == dev.nwk | ||
| assert packet.cluster_id == KeepAlive.cluster_id | ||
| assert packet.src_ep == 1 | ||
| assert packet.dst_ep == 1 |
There was a problem hiding this comment.
After rebasing onto #1889, this test could also assert packet.tx_options & t.TransmitOptions.APS_Encryption. The transient-cluster path plus encryption is the combination that matters for keep-alive clients.
Might address home-assistant/core#181626 (comment).
Zigbee devices have endpoints and every endpoint advertises its server and client clusters. For 99.99% of devices, this is accurate and messages do not arrive on endpoints or clusters that are unexpected. Zigpy assumes this is how devices work.
Unfortunately, it looks like the above Sonoff device has hit a bad combination of bugs:
KeepAlivecluster, so it tries to determine connectivity to the coordinator on its own.KeepAlivecluster, so zigpy does not expect this packet to come in and ignores it.KeepAlivecluster.The end result is the device thinks it is not connected to the coordinator and tries to re-connect every few hours.
This PR gives zigpy the ability to respond even if the device is not supposed to have a cluster. Ideally, this should be fixed in the device firmware. We could also work around it in a quirk. This is a more general workaround.