Correctly parse the ZCL default response in write_attributes#1767
Correctly parse the ZCL default response in write_attributes#1767
write_attributes#1767Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1767 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 63 63
Lines 12680 12680
=======================================
Hits 12619 12619
Misses 61 61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Oh, this is interesting! When do we even get a default response here? I only vaguely remember something about this.. Is it device specific? |
|
I just saw the inconsistency, I don't think it's a codepath that I've ever seen (it may even be impossible?) but we have it in zigpy so maybe it was seen at some point. |
There was a problem hiding this comment.
Pull request overview
Aligns write_attributes() with the ZCL Default Response schema by reading the status from the correct field, matching the behavior fixed previously for configure_reporting_multiple.
Changes:
- Fix
write_attributes()default-response handling to use the second field (status) instead of the first. - Update/add tests to construct realistic ZCL Default Response frames for
write_attributes()andconfigure_reporting_multiple(). - Adjust a quirk test mock response tuple to keep
write_attributes()behavior consistent with the updated parsing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
zigpy/zcl/__init__.py |
Fixes default response parsing in write_attributes() by taking status from the correct position. |
tests/test_zcl.py |
Adds/updates tests to validate correct status extraction from Default Responses and cache behavior. |
tests/test_quirks.py |
Updates a mocked Endpoint.request() return tuple to remain compatible with default-response parsing expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # to include the manufacturer ID in the request | ||
| with patch.object(ep, "request", AsyncMock()) as request_mock: | ||
| request_mock.return_value = (zcl.foundation.Status.SUCCESS, "done") | ||
| request_mock.return_value = (0x12, zcl.foundation.Status.SUCCESS) |
There was a problem hiding this comment.
The mocked ep.request() response uses a hardcoded command id 0x12, which doesn’t correspond to any of the three commands invoked in this test (cluster command, read_attributes, write_attributes). This makes the test brittle if the code later validates the Default Response’s command_id. Prefer returning a Default Response whose command_id is derived from the command_id kwarg of the request (e.g., via an AsyncMock side_effect).
| request_mock.return_value = (0x12, zcl.foundation.Status.SUCCESS) | |
| async def _request_side_effect(*args, **kwargs): | |
| return kwargs["command_id"], zcl.foundation.Status.SUCCESS | |
| request_mock.side_effect = _request_side_effect |
| # to include the manufacturer ID in the request | ||
| with patch.object(ep, "request", AsyncMock()) as request_mock: | ||
| request_mock.return_value = (zcl.foundation.Status.SUCCESS, "done") | ||
| request_mock.return_value = (0x12, zcl.foundation.Status.SUCCESS) |
There was a problem hiding this comment.
I think we'll need to update quite a few quirks tests as well then 😅
There was a problem hiding this comment.
Looks like only three...
FAILED tests/test_develco.py::test_frient_emi - ValueError: Failed to convert status='done' from type <class 'str'> to <enum 'Status'>
FAILED tests/test_tuya.py::test_fan_switch_writes_attributes[TS0501FanSwitch] - ValueError: Failed to convert status='done' from type <class 'str'> to <enum 'Status'>
FAILED tests/test_xiaomi.py::test_xiaomi_eu_plug_binding[PlugMAEU01] - ValueError: Failed to convert status='done' from type <class 'str'> to <enum 'Status'>But I'm sure custom quirks might exist that rely on this behavior.
There was a problem hiding this comment.
Looks like only three...
Interesting, so I guess we don't verify the output a lot..? https://github.com/search?q=repo%3Azigpy%2Fzha-device-handlers+Status.SUCCESS%2C+%22done%22&type=code
But I'm sure custom quirks might exist that rely on this behavior.
Really? For these, it's just the tests which do something stupid. I wouldn't expect any actual quirks to somehow return a default response with the fields in a different order.
| results = await cluster.write_attributes( | ||
| {Basic.AttributeDefs.manufacturer: "manufacturer"} | ||
| ) | ||
|
|
||
| # It should correctly return FAILURE (0x01), not Write_Attributes (0x02) | ||
| assert results[0][0].status == foundation.Status.FAILURE |
There was a problem hiding this comment.
nit: While we're at it, should we also assert this didn't make it into the attribute cache?
On the other hand, the logic for the default response currently only adds the status to records_group, so this is basically already covered by the test above.
#1764 used
status = rsp[1]butwrite_attributesusesstatus = response[0]. I think both need to be aligned, since the ZCL default response stores the status as the second field.