diff --git a/tests/test_quirks.py b/tests/test_quirks.py index 36f68f1ee3..06a5e22cef 100644 --- a/tests/test_quirks.py +++ b/tests/test_quirks.py @@ -5,6 +5,7 @@ import collections import importlib import json +import math from pathlib import Path from unittest import mock @@ -996,6 +997,53 @@ def test_suspicious_cluster_moves(quirk: CustomDevice) -> None: ) +async def test_local_data_cluster_read_does_not_reconvert(device_mock) -> None: + """Reading a LocalDataCluster must not re-apply a converting `_update_attribute`. + + `read_attributes_raw` serves values straight from the attribute cache and zigpy + feeds every successful read result back through `_update_attribute`. A cluster + that converts values there would otherwise convert its own output again on + every read, walking the stored value towards the conversion's fixed point. + """ + registry = DeviceRegistry() + + class ConvertingLocalCluster(zhaquirks.LocalDataCluster): + """Local cluster that converts values in `_update_attribute`.""" + + cluster_id = 0x1235 + + class AttributeDefs(foundation.BaseAttributeDefs): + """Attribute definitions.""" + + measured_value = foundation.ZCLAttributeDef(id=0, type=t.uint16_t) + + def _update_attribute(self, attrid, value): + if attrid == 0 and value > 0: + value = int(10000 * math.log10(value) + 1) + super()._update_attribute(attrid, value) + + ( + QuirkBuilder(device_mock.manufacturer, device_mock.model) + .adds(ConvertingLocalCluster) + .add_to_registry(registry) + ) + device = registry.resolve(device_mock) + cluster = device.endpoints[1].in_clusters[0x1235] + + # a value from the device is converted once + cluster._update_attribute(0, 100) + assert cluster.get(0) == 20001 + + # repeated reads must keep returning that value, not convert it again + for _ in range(4): + assert await cluster.read_attributes([0]) == ({0: 20001}, {}) + assert cluster.get(0) == 20001 + + # a genuine new value from the device is still converted + cluster._update_attribute(0, 10) + assert cluster.get(0) == 10001 + + async def test_local_data_cluster(device_mock) -> None: """Ensure reading attributes from a LocalDataCluster works as expected.""" registry = DeviceRegistry() diff --git a/tests/test_xiaomi.py b/tests/test_xiaomi.py index 318dd393c7..e46f91ece0 100644 --- a/tests/test_xiaomi.py +++ b/tests/test_xiaomi.py @@ -1595,6 +1595,32 @@ async def test_xiaomi_e1_thermostat_schedule_settings_deserialization( assert str(s) == expected_string +async def test_xiaomi_local_illuminance_survives_repeated_reads( + zigpy_device_from_quirk, +): + """Reading the local illuminance cluster must not re-apply the lux conversion. + + The cluster converts lux to the ZCL representation in `_update_attribute`, and + zigpy echoes read results back through it. Without a guard each read converts + the previous result again, walking the value towards the conversion's fixed + point: 100 lx would read back as 20001, 43011, 46336, 46660. + """ + device = zigpy_device_from_quirk(zhaquirks.xiaomi.aqara.motion_ac02.LumiMotionAC02) + illuminance_cluster = device.endpoints[1].illuminance + measured_value = IlluminanceMeasurement.AttributeDefs.measured_value.id + + # 100 lx from the device converts once + illuminance_cluster.update_attribute(measured_value, 100) + converted = int(10000 * math.log10(100) + 1) + assert illuminance_cluster.get(measured_value) == converted + + for _ in range(4): + success, failure = await illuminance_cluster.read_attributes([measured_value]) + assert not failure + assert success[measured_value] == converted + assert illuminance_cluster.get(measured_value) == converted + + @pytest.mark.parametrize( "quirk, invalid_iilluminance_report", ( diff --git a/zhaquirks/__init__.py b/zhaquirks/__init__.py index 9c20f1bc88..b2d36ab65a 100644 --- a/zhaquirks/__init__.py +++ b/zhaquirks/__init__.py @@ -96,6 +96,32 @@ class LocalDataCluster(CustomCluster): _DEFAULT_VALUES: dict[int, typing.Any] = {} _VALID_ATTRIBUTES: set[int] = set() + def __init__(self, *args, **kwargs) -> None: + """Initialize the cluster.""" + super().__init__(*args, **kwargs) + # Attribute ids handed back by the most recent `read_attributes_raw`, waiting + # to be echoed into `_update_attribute` by zigpy. See `_update_attribute`. + self._read_echo: set[int] = set() + + def _update_attribute(self, attrid: int, value: typing.Any) -> None: + """Update an attribute, ignoring zigpy echoing our own read results back. + + `read_attributes_raw` serves values straight from the attribute cache, and + zigpy feeds every successful read result back through `_update_attribute`. + On a cluster that converts values there, that converts an already converted + value a second time and stores the result, so every read corrupts the value + further. The cache is the source of truth for a local cluster, so an echo of + a value we just returned carries no new information and is dropped. + + The value is not compared, only the attribute id: a subclass overriding + `_update_attribute` sits ahead of this one in the MRO and has already + transformed the value by the time it gets here. + """ + if attrid in self._read_echo: + self._read_echo.discard(attrid) + return + super()._update_attribute(attrid, value) + def get(self, key: int | str, default: typing.Any | None = None) -> typing.Any: """Get cached attribute, falling back to _DEFAULT_VALUES then default.""" try: @@ -144,6 +170,13 @@ async def read_attributes_raw(self, attributes, manufacturer=None, **kwargs): or record.attrid in self._VALID_ATTRIBUTES ): record.status = foundation.Status.SUCCESS + # zigpy echoes every successful read result back through + # `_update_attribute`; record them so that echo can be recognised there. + self._read_echo = { + record.attrid + for record in records + if record.status == foundation.Status.SUCCESS + } return (records,) def _write_attr_records(self, attributes: dict) -> list[foundation.Attribute]: