Repository navigation
Conversation
|
Thanks for the PR! Could you elaborate a bit more about this proposed workflow for ZHA? |
|
For example. When a user creates an automation for lights and select a few or area. The option is in the interface to create a group directly from the selection. For this we need to check if the group is created correctly or if the group already exists and re-use the group. |
liudger
left a comment
There was a problem hiding this comment.
Self-review of the current draft at f760e17 (not an independent maintainer approval). The small read-only API is a reasonable fit, but the error classification should be tightened before marking this ready. See the two inline findings.
Validation: endpoint + group suites passed (96 tests), scoped Ruff check/format and diff check passed. Additional external probes produced 4 failures covering the two findings and 3 passes confirming fresh reads including empty membership, unchanged local membership, transport-error propagation, and cancellation. Those read-only/freshness assertions should also be added to the committed regression tests. No hardware validation was performed, and GitHub currently reports no checks for this PR.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Preserve scan failure handling and strengthen the read-only behavior test.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds a public API to read Zigbee group membership directly from devices without updating the local cache.
Changes:
- Adds
UnsupportedCluster. - Implements
Endpoint.get_group_membership(). - Reuses the API in scans and adds regression tests.
Review findings:
- Moderate: Preserve
AttributeErrorhandling in background scans. - Nit: Verify successful readback does not update the group cache.
| File | Description |
|---|---|
zigpy/exceptions.py |
Adds the unsupported-cluster exception. |
zigpy/endpoint.py |
Implements membership readback and scan integration. |
tests/test_endpoint.py |
Tests membership readback and error handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1890 +/- ##
=======================================
Coverage 99.50% 99.50%
=======================================
Files 59 59
Lines 12415 12425 +10
=======================================
+ Hits 12353 12363 +10
Misses 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Looks correct against current dev. One thing needs settling before merge: how this interacts with the open #1778, which changes the error path this API classifies.
Coordination with #1778. #1778 (open) makes Device._maybe_match_response raise InvalidDefaultResponse for any non-SUCCESS ZCL default response. With it merged, the isinstance(res, DefaultResponse) branch in get_group_membership() never sees UNSUP_CLUSTER_COMMAND & co. The exception escapes from get_membership() as an InvalidDefaultResponse and never becomes UnsupportedCluster. So a device that genuinely lacks the command would stop reporting as unsupported, which is the exact distinction this API exists to make. I checked this by merging #1778 into this branch locally (conflicts only in endpoint.py/exceptions.py, resolved by keeping both sides) and driving a real Default Response frame through Device.packet_received:
| status | PR head alone | PR head + #1778 |
|---|---|---|
UNSUP_CLUSTER_COMMAND |
UnsupportedCluster |
InvalidDefaultResponse |
FAILURE |
InvalidResponse |
InvalidDefaultResponse |
group_membership_scan() stays safe either way, because both exceptions are ZigbeeExceptions. Only the public contract of the new method breaks.
The new tests don't catch this: they set ep.device.request.return_value to a DefaultResponse, which skips response matching entirely, so both variants pass. Please:
- make the status mapping work in both worlds. For example, also catch
InvalidResponsesubclasses that carry a.status(theInvalidDefaultResponseshape in #1778) and map them through the same unsupported-status set. Or agree with the #1778 author which PR lands first and adapt the second one. - add at least one test that sends a real Default Response packet through
dev.packet_received(the pattern fromtest_request_error_default_responsein #1778'stests/test_device.py). Then whichever PR merges second fails loudly instead of silently changing the exception type.
Minor / optional
UnsupportedClusteris also raised for command-level rejections (UNSUP_CLUSTER_COMMAND,UNSUP_GENERAL_COMMAND, …), not only a missing cluster. Keeping the rejectedstatusas an attribute (as #1778 does onInvalidDefaultResponse) would let callers tell "no Groups cluster" apart from "device rejected Get Group Membership" without parsing the message.- There's no ZHA-side consumer PR yet. The error contract (what counts as unsupported vs. failed) is easiest to settle against the actual caller, so linking it once it exists would help.
Verified (5 checks)
- Full suite at ef68c71: 1332 passed.
- Real-packet probe (Groups
get_membershipanswered by aDefaultResponseviaDevice.packet_received) on the PR head:UNSUP_CLUSTER_COMMAND→UnsupportedCluster,FAILURE→InvalidResponse;group_membership_scan()swallows both and leaves the group cache untouched. - Same probe with #1778 merged in: both statuses surface as
InvalidDefaultResponse; the scan still swallows them. - The scan keeps the same outcome for every error path on
dev: a missing Groups cluster or request-sideAttributeErrorstill returns quietly, and timeouts/ZigbeeExceptions are still logged and swallowed. Only the debug message changes for non-"unsupported" default responses. - The three existing threads (default-response classification, narrowing the
AttributeErrortry to theself.groupslookup, keepingAttributeErrorin the scan) are addressed at the current head.
Why
A proposed Home Assistant/ZHA workflow creates or reuses a Zigbee group from selected lights. Before offering that group as an automation target, it needs to confirm membership directly with each device. Cached membership alone cannot confirm that the devices actually joined.
The existing
group_membership_scan()updates the local cache and handles failures internally. Callers need a read-only result that distinguishes a successful read from a timeout or unsupported operation.Changes
Endpoint.get_group_membership()to request membership from the device without reading or updating the local group cache.frozenset; raiseUnsupportedClusterfor missing/rejected Groups support and propagate transport errors.group_membership_scan()and add regression tests.Kept as a draft for feedback on the API and error handling.
Testing
pytest tests/test_endpoint.py -q --tb=short: 75 passed.