Skip to content

Fix configuring attribute reporting #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
puddly merged 12 commits into from
Feb 13, 2026
Merged

Fix configuring attribute reporting #648

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b954341
Remove handling for `manufacturer_code`, now done by zigpy
TheJulianJES Feb 7, 2026
3da1161
Pass `ZCLAttributeDef` and `ReportingConfig`
TheJulianJES Feb 7, 2026
1e96ec6
Update tests to expect new `configure_reporting_multiple` calls
TheJulianJES Feb 7, 2026
b2c551e
Swap conditions to be useful
TheJulianJES Feb 7, 2026
def5a7a
Test `_configure_reporting_status`
TheJulianJES Feb 10, 2026
6d3269c
Import `Color` cluster
TheJulianJES Feb 10, 2026
c697339
Swap to custom `TestClusterHandler` for `Color` cluster
TheJulianJES Feb 10, 2026
295872b
Add `ids` with nicer names
TheJulianJES Feb 10, 2026
ac89d9b
Assert entire calls
TheJulianJES Feb 10, 2026
5e04a5b
Guard against zigpy (or mocks) returning an empty list as response
TheJulianJES Feb 11, 2026
fa797c3
Fix `res` typing
TheJulianJES Feb 11, 2026
182a8bc
Remove check for zigpy returning an `Exception` or a single `Configur…
TheJulianJES Feb 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 143 additions & 12 deletions tests/test_cluster_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from zhaquirks.xiaomi.aqara.sensor_switch_aq3 import BUTTON_DEVICE_TYPE, SwitchAQ3
from zigpy.device import Device as ZigpyDevice
from zigpy.endpoint import Endpoint as ZigpyEndpoint
import zigpy.exceptions
import zigpy.profiles.zha
from zigpy.quirks import DEVICE_REGISTRY
import zigpy.types as t
Expand All @@ -32,7 +33,9 @@
PowerConfiguration,
)
from zigpy.zcl.clusters.homeautomation import Diagnostic
from zigpy.zcl.clusters.lighting import Color
from zigpy.zcl.clusters.measurement import TemperatureMeasurement
from zigpy.zcl.helpers import ReportingConfig
import zigpy.zdo.types as zdo_t

from tests.common import (
Expand All @@ -47,13 +50,19 @@
zigpy_device_from_json,
)
from zha.application import Platform
from zha.application.const import ATTR_QUIRK_ID
from zha.application.const import (
ATTR_QUIRK_ID,
ZHA_CLUSTER_HANDLER_MSG_BIND,
ZHA_CLUSTER_HANDLER_MSG_CFG_RPT,
)
from zha.application.gateway import Gateway
from zha.application.platforms.button import IdentifyButton
from zha.exceptions import ZHAException
from zha.zigbee.cluster_handlers import (
AttrReportConfig,
ClientClusterHandler,
ClusterBindEvent,
ClusterConfigureReportingEvent,
ClusterHandler,
ClusterHandlerStatus,
parse_and_log_command,
Expand Down Expand Up @@ -175,7 +184,7 @@ def endpoint_mock(zigpy_coordinator_device: ZigpyDevice) -> Endpoint:
),
(zigpy.zcl.clusters.hvac.Fan.cluster_id, 1, {"fan_mode"}),
(
zigpy.zcl.clusters.lighting.Color.cluster_id,
Color.cluster_id,
1,
{
"current_x",
Expand Down Expand Up @@ -303,7 +312,7 @@ async def test_in_cluster_handler_config(
reported_attrs = set()

for mock_call in cluster.configure_reporting_multiple.mock_calls:
reported_attrs.update(mock_call.args[0].keys())
reported_attrs.update(attr_def.name for attr_def in mock_call.args[0])

assert attrs == reported_attrs
assert cluster.configure_reporting.call_count == 0
Expand Down Expand Up @@ -924,7 +933,7 @@ class TestZigbeeClusterHandler(ClusterHandler):
mock_ep.profile_id = zigpy.profiles.zha.PROFILE_ID
mock_ep.device.zdo = AsyncMock()

cluster = zigpy.zcl.clusters.lighting.Color(mock_ep)
cluster = Color(mock_ep)
cluster.bind = AsyncMock(
spec_set=cluster.bind,
return_value=[zdo_t.Status.SUCCESS], # ZDOCmd.Bind_rsp
Expand All @@ -941,23 +950,145 @@ class TestZigbeeClusterHandler(ClusterHandler):
cluster_handler = TestZigbeeClusterHandler(cluster, endpoint)
await cluster_handler.async_configure()

# Since we request reporting for five attributes, we need to make two calls (3 + 1)
# Since we request reporting for four attributes, we need to make two calls (3 + 1)

assert cluster.configure_reporting_multiple.mock_calls == [
mock.call(
{
"current_x": (1, 60, 1),
"current_hue": (1, 60, 2),
"color_temperature": (1, 60, 3),
Color.AttributeDefs.current_x: ReportingConfig(1, 60, 1),
Color.AttributeDefs.current_hue: ReportingConfig(1, 60, 2),
Color.AttributeDefs.color_temperature: ReportingConfig(1, 60, 3),
}
),
mock.call(
{
"current_y": (1, 60, 4),
Color.AttributeDefs.current_y: ReportingConfig(1, 60, 4),
}
),
]


@pytest.mark.parametrize(
("response", "expected_statuses"),
[
# Exception result: all attributes marked as FAILURE
(
zigpy.exceptions.ZigbeeException("timeout"),
{"current_x": "FAILURE", "current_y": "FAILURE"},
),
# Single ConfigureReportingResponseRecord: all attributes marked as FAILURE
(
foundation.ConfigureReportingResponseRecord(
status=foundation.Status.SUCCESS
),
{"current_x": "FAILURE", "current_y": "FAILURE"},
),
# Single SUCCESS in a list: all attributes marked as SUCCESS (ZCL 2.5.8.1.3)
(
[
foundation.ConfigureReportingResponseRecord(
status=foundation.Status.SUCCESS
)
],
{"current_x": "SUCCESS", "current_y": "SUCCESS"},
),
# Empty list: unexpected response, all attributes marked as FAILURE
(
[],
{"current_x": "FAILURE", "current_y": "FAILURE"},
),
# Per-attribute results: mixed success/failure
(
[
foundation.ConfigureReportingResponseRecord(
status=foundation.Status.SUCCESS,
attrid=Color.AttributeDefs.current_x.id,
),
foundation.ConfigureReportingResponseRecord(
status=foundation.Status.UNSUPPORTED_ATTRIBUTE,
attrid=Color.AttributeDefs.current_y.id,
),
],
{"current_x": "SUCCESS", "current_y": "UNSUPPORTED_ATTRIBUTE"},
),
],
ids=[
"exception",
"single_record",
"single_success_list",
"empty_list",
"mixed_per_attribute",
],
)
async def test_configure_reporting_status(
zha_gateway: Gateway, response, expected_statuses
) -> None:
"""Test configure reporting status parsing via async_configure."""
zigpy_coordinator_device: ZigpyDevice = zigpy_coordinator_device_mock(zha_gateway)
endpoint: Endpoint = endpoint_mock(zigpy_coordinator_device)

class TestClusterHandler(ClusterHandler):
BIND = True
REPORT_CONFIG = (
AttrReportConfig(attr="current_x", config=(1, 60, 1)),
AttrReportConfig(attr="current_y", config=(1, 60, 2)),
)

mock_ep = mock.AsyncMock()
mock_ep.profile_id = zigpy.profiles.zha.PROFILE_ID
mock_ep.device.zdo = AsyncMock()

cluster = Color(mock_ep)
cluster.bind = AsyncMock(
spec_set=cluster.bind,
return_value=[zdo_t.Status.SUCCESS],
)
cluster.configure_reporting_multiple = AsyncMock(
spec_set=cluster.configure_reporting_multiple,
return_value=response,
)

cluster_handler = TestClusterHandler(cluster, endpoint)

mock_emit = MagicMock()
cluster_handler._endpoint.device.emit = mock_emit

await cluster_handler.async_configure()

assert mock_emit.call_args_list == [
mock.call(
ZHA_CLUSTER_HANDLER_MSG_BIND,
ClusterBindEvent(
cluster_name=cluster.name,
cluster_id=cluster.cluster_id,
cluster_handler_unique_id=cluster_handler.unique_id,
success=True,
),
),
mock.call(
ZHA_CLUSTER_HANDLER_MSG_CFG_RPT,
ClusterConfigureReportingEvent(
cluster_name=cluster.name,
cluster_id=cluster.cluster_id,
cluster_handler_unique_id=cluster_handler.unique_id,
attributes={
attr_name: {
"min": 1,
"max": 60,
"id": attr_name,
"name": attr_name,
"change": idx + 1,
"status": expected_status,
}
for idx, (attr_name, expected_status) in enumerate(
expected_statuses.items()
)
},
),
),
]


async def test_invalid_cluster_handler(zha_gateway: Gateway, caplog) -> None: # pylint: disable=unused-argument
"""Test setting up a cluster handler that fails to match properly."""

Expand All @@ -970,7 +1101,7 @@ class TestZigbeeClusterHandler(ClusterHandler):
zigpy_ep = zigpy.endpoint.Endpoint(mock_device, endpoint_id=1)
zigpy_ep.profile_id = zigpy.profiles.zha.PROFILE_ID

cluster = zigpy_ep.add_input_cluster(zigpy.zcl.clusters.lighting.Color.cluster_id)
cluster = zigpy_ep.add_input_cluster(Color.cluster_id)
cluster.configure_reporting_multiple = AsyncMock(
spec_set=cluster.configure_reporting_multiple,
return_value=[
Expand Down Expand Up @@ -1015,7 +1146,7 @@ class TestZigbeeClusterHandler(ColorClusterHandler):
zigpy_ep = zigpy.endpoint.Endpoint(mock_device, endpoint_id=1)
zigpy_ep.profile_id = zigpy.profiles.zha.PROFILE_ID

cluster = zigpy_ep.add_input_cluster(zigpy.zcl.clusters.lighting.Color.cluster_id)
cluster = zigpy_ep.add_input_cluster(Color.cluster_id)
cluster.configure_reporting_multiple = AsyncMock(
spec_set=cluster.configure_reporting_multiple,
return_value=[
Expand Down Expand Up @@ -1055,7 +1186,7 @@ class TestZigbeeClusterHandler(ColorClusterHandler):
zigpy_ep = zigpy.endpoint.Endpoint(mock_device, endpoint_id=1)
zigpy_ep.profile_id = zigpy.profiles.zha.PROFILE_ID

cluster = zigpy_ep.add_input_cluster(zigpy.zcl.clusters.lighting.Color.cluster_id)
cluster = zigpy_ep.add_input_cluster(Color.cluster_id)
cluster.configure_reporting_multiple = AsyncMock(
spec_set=cluster.configure_reporting_multiple,
return_value=[
Expand Down
10 changes: 9 additions & 1 deletion tests/test_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
from zigpy.zcl import ClusterType
from zigpy.zcl.clusters import general
from zigpy.zcl.clusters.general import Ota, PowerConfiguration
from zigpy.zcl.clusters.measurement import CarbonDioxideConcentration
from zigpy.zcl.foundation import Status, WriteAttributesResponse
from zigpy.zcl.helpers import ReportingConfig
import zigpy.zdo.types as zdo_t

from tests.common import (
Expand Down Expand Up @@ -1176,7 +1178,13 @@ async def test_join_binding_reporting(zha_gateway: Gateway) -> None:

assert mock_bind.mock_calls == [call()]
assert mock_reporting_config.mock_calls == [
call({"measured_value": (30, 900, 1e-6)})
call(
{
CarbonDioxideConcentration.AttributeDefs.measured_value: ReportingConfig(
30, 900, 1e-6
)
}
)
]


Expand Down
47 changes: 24 additions & 23 deletions zha/zigbee/cluster_handlers/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
Status,
ZCLAttributeDef,
)
from zigpy.zcl.helpers import ReportingConfig

from zha.application.const import (
ZHA_CLUSTER_HANDLER_MSG,
Expand Down Expand Up @@ -369,12 +370,6 @@ async def configure_reporting(self) -> None:
devices are unreachable.
"""
event_data = {}
kwargs = {}
if (
self.cluster.cluster_id >= 0xFC00
and self._endpoint.device.manufacturer_code
):
kwargs["manufacturer"] = self._endpoint.device.manufacturer_code

for attr_report in self.REPORT_CONFIG:
attr, config = attr_report["attr"], attr_report["config"]
Expand All @@ -399,12 +394,17 @@ async def configure_reporting(self) -> None:
to_configure[REPORT_CONFIG_ATTR_PER_REQ:],
)
while chunk:
reports = {rec["attr"]: rec["config"] for rec in chunk}
reports = {
self.cluster.find_attribute(rec["attr"]): ReportingConfig(
*rec["config"]
)
for rec in chunk
}
try:
res = await RETRYABLE_REQUEST_DECORATOR(
self.cluster.configure_reporting_multiple
)(reports, **kwargs)
self._configure_reporting_status(reports, res[0], event_data)
)(reports)
self._configure_reporting_status(reports, res, event_data)
except (zigpy.exceptions.ZigbeeException, TimeoutError) as ex:
self.debug(
"failed to set reporting on '%s' cluster for: %s",
Expand All @@ -429,26 +429,27 @@ async def configure_reporting(self) -> None:

def _configure_reporting_status(
self,
attrs: dict[str, tuple[int, int, float | int]],
res: list | tuple,
attrs: dict[ZCLAttributeDef, ReportingConfig],
res: list[ConfigureReportingResponseRecord],
event_data: dict[str, dict[str, Any]],
) -> None:
"""Parse configure reporting result."""
if isinstance(res, (Exception, ConfigureReportingResponseRecord)):
attr_names = {attr_def.name for attr_def in attrs}
if not res or isinstance(res, (Exception, ConfigureReportingResponseRecord)):
Copy link
Copy Markdown
Contributor Author

@TheJulianJES TheJulianJES Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be able to hit the isinstance(res, (Exception, ConfigureReportingResponseRecord)) check ever now? Like, returning an exception or just a single ConfigureReportingResponseRecord? Should I just remove this?

If so, should I at least keep the guard against zigpy returning an empty list? Even though that should not happen after zigpy/zigpy#1764 anymore. We'd just mark the attributes as failed to configure if zigpy (or some test mocks elsewhere) somehow return an empty list. Or should we just ignore that and fail however that would fail? (causing SUCCESS)

Copy link
Copy Markdown
Contributor Author

@TheJulianJES TheJulianJES Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it with 182a8bc (this PR). Having zigpy return an exception or a single status report can't happen at all from what I see. It's always a list at least.

Copy link
Copy Markdown
Contributor

@puddly puddly Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, returning exception objects (instead of raising them) is a legacy artifact and something that we've slowly been trying to get rid of. There are more instances within the ZHA library that still account for this possibility. I believe we do still have a few asyncio.gather(..., return_exceptions=True) invocations that were the cause of this pattern.

# assume default response
self.debug(
"attr reporting for '%s' on '%s': %s",
attrs,
attr_names,
self.name,
res,
)
for attr in attrs:
event_data[attr]["status"] = Status.FAILURE.name
for attr_name in attr_names:
event_data[attr_name]["status"] = Status.FAILURE.name
return
Comment on lines +438 to 447
Copy link
Copy Markdown
Contributor Author

@TheJulianJES TheJulianJES Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, zigpy should also never return an empty list. Do we want to keep this failure case in case zigpy breaks again, so we report all attributes as failed to configure then?

If we remove this, they'd report as being successfully configured otherwise, which also isn't great. But this should never happen (again), soo..

Copy link
Copy Markdown
Contributor

@puddly puddly Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep it in. In the near future I'd like to persist the attribute configuration state in the database so this code likely will be reworked anyways.

if res[0].status == Status.SUCCESS and len(res) == 1:
if len(res) == 1 and res[0].status == Status.SUCCESS:
self.debug(
"Successfully configured reporting for '%s' on '%s' cluster: %s",
attrs,
attr_names,
self.name,
res,
)
Expand All @@ -458,8 +459,8 @@ def _configure_reporting_status(
# attributes, in order to save bandwidth. In the case of successful configuration of all attributes,
# only a single attribute status record SHALL be included in the command, with the status field set to
# SUCCESS and the direction and attribute identifier fields omitted.
for attr in attrs:
event_data[attr]["status"] = Status.SUCCESS.name
for attr_name in attr_names:
event_data[attr_name]["status"] = Status.SUCCESS.name
return

for record in res:
Expand All @@ -477,14 +478,14 @@ def _configure_reporting_status(
self.name,
res,
)
success = set(attrs) - set(failed)
success = attr_names - set(failed)
self.debug(
"Successfully configured reporting for '%s' on '%s' cluster",
set(attrs) - set(failed),
success,
self.name,
)
for attr in success:
event_data[attr]["status"] = Status.SUCCESS.name
for attr_name in success:
event_data[attr_name]["status"] = Status.SUCCESS.name

async def async_configure(self) -> None:
"""Set cluster binding and attribute reporting."""
Expand Down
Loading