Stop LocalDataCluster reads from re-applying quirk value conversions

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()

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",
     (

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]: