AC: quantize subdevice temperature writes against their own step (Opus review)

async_send_command handed write_fn/validate_fn the raw cache snapshot (real, on-the-wire hrefs), not a subdevice-scoped view. Every other consumer of a full resources dict (exists_fn, rep_fn, is_legacy_board, ...) reads through coordinator.canonical_resources() specifically to avoid this; write_fn/validate_fn didn't, so on a composite AC (issue #177) _temperature_step's resources.get(HREF_TEMP_CONTROL) saw the master's /temperature/control/vs/0 instead of the subdevice's own /temperature/control/vs/1, silently rounding a subdevice's 0.5-degree write to a whole degree. The remote-control gate stays on the raw snapshot -- /remotectrl/* is a shared, MAIN-only resource a subdevice's owned-hrefs-only canonical view would drop entirely. climate.py's target_temperature_step duplicated this same read-in-order logic; pointed it at airconditioner._temperature_step so the read and write paths can't drift again. Also, from the same review: - _quantize_temperature rejects non-finite floats (nan/inf survive float() but raise out of round()/division, escaping write_fn's documented None-on-bad-payload contract). - Deduplicated the quantize-and-check block shared by the temperature_ocf/temperature branches of _climate_write. - Added the /temperatures/vs/0 items[]-fallback test that was previously unreachable (every increment-carrying fixture also has /temperature/control/vs/0, which _temperature_step checks first). - Added a coordinator-level test seeding an indexed subdevice with its own step, distinct from the master's, covering the fix above. - The Towels/Bedding regression test now checks all 7 locale catalogs, not just English -- the bug is a code-mapping error, and test_every_language_mirrors_the_english_catalog only checks key topology, not values.
2026-08-09 16:25:12 +00:00
parent 846aefbcba
commit 676074b2f8
6 changed files with 94 additions and 23 deletions
@@ -58,9 +58,6 @@ from .registry.capabilities.airconditioner import (
 from .registry.capabilities.airconditioner import (
    HREF_POWER_VS as POWER_VS_HREF,
 )
-from .registry.capabilities.airconditioner import (
-    HREF_TEMP_CONTROL as TEMP_CONTROL_HREF,
-)
 from .registry.capabilities.airconditioner import (
    HREF_TEMP_CURRENT as TEMP_CURRENT_HREF,
 )
@@ -80,6 +77,7 @@ from .registry.capabilities.airconditioner import (
    HREF_WIND_STRENGTH as WIND_STRENGTH_HREF,
 )
 from .registry.capabilities.airconditioner import (
+    _temperature_step,
    extend_option_code_bit,
    has_extend_option_code,
    has_option_code,
@@ -558,12 +556,11 @@ class LocalThingsClimate(LocalThingsEntity, ClimateEntity):

    @property
    def target_temperature_step(self) -> float:
-        return (
-            _num(self._rep(TEMP_CONTROL_HREF).get("increment"))
-            or _num(self._rep(TEMP_CONTROL_HREF).get("x.com.samsung.da.increment"))
-            or _num(self._temps_vs().get("x.com.samsung.da.increment"))
-            or 1.0
-        )
+        # Shared with the write path (airconditioner._climate_write) so a
+        # step read here always matches the step a write is quantized to --
+        # self._resources is this entity's own subdevice's canonical view
+        # (issue #177), the same shape _temperature_step expects.
+        return _temperature_step(self._resources) or 1.0

    # -- hvac mode ----------------------------------------------------------

@@ -1186,17 +1186,28 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
            return
        href = bound_entity.href
        rep = self._cache.get(href or "") or {}
-        resources = self._cache.snapshot()
+        # The remote-control gate below keys off the raw on-the-wire href
+        # and a raw snapshot -- /remotectrl/* is a shared, MAIN-only
+        # resource that a subdevice's canonical_resources() view (owned
+        # hrefs only) would drop entirely. write_fn/validate_fn, by
+        # contrast, are written against canonical hrefs (same convention as
+        # exists_fn/rep_fn -- see entity.py's _resources), so they get this
+        # entity's own subdevice view instead of the raw snapshot: without
+        # it, a composite device's write_fn reading resources.get(some
+        # canonical href) (e.g. airconditioner._temperature_step) would
+        # silently see the master's resource instead of its own subdevice's.
+        raw_resources = self._cache.snapshot()
        bypass_remote_control = self._entry.options.get(CONF_BYPASS_REMOTE_CONTROL, False)
        if (
            not bypass_remote_control
-            and remote_control_required_for_write(resources, href or "")
-            and not remote_control_enabled(resources)
+            and remote_control_required_for_write(raw_resources, href or "")
+            and not remote_control_enabled(raw_resources)
        ):
            raise ServiceValidationError(
                translation_domain=DOMAIN,
                translation_key="remote_control_disabled",
            )
+        resources = self.canonical_resources(bound_entity.subdevice)
        validate_fn = getattr(desc, "validate_fn", None)
        if validate_fn is not None:
            error = validate_fn(payload, rep, resources)
@@ -9,6 +9,7 @@ here off the coordinator snapshot. These caps stay out of the global
 capabilities/__init__.py) -- AC-only, by_type registry only.
 """

+import math
 from dataclasses import replace

 from ..capability import Capability
@@ -510,6 +511,13 @@ def _quantize_temperature(value, step):
        numeric = float(value)
    except (TypeError, ValueError):
        return None
+    if not math.isfinite(numeric):
+        # float('nan')/float('inf') pass the try/except above (they're
+        # valid floats) but round()/division on them raises ValueError/
+        # OverflowError instead of returning -- reject here so a bad
+        # payload always comes back as a clean None, not an exception
+        # out of write_fn.
+        return None
    step_value = _num(step) or 1.0
    if step_value <= 0:
        step_value = 1.0
@@ -558,16 +566,13 @@ def _climate_write(payload, rep, href=None, resources=None):
        return (["power", "vs", "0"], {"x.com.samsung.da.power": "On" if value else "Off"})
    if kind == "mode":
        return (["mode", "vs", "0"], {"x.com.samsung.da.modes": [value]})
-    if kind == "temperature_ocf":
+    if kind in ("temperature_ocf", "temperature"):
        quantized = _quantize_temperature(value, _temperature_step(resources))
        if quantized is None:
            return None
-        return (["temperature", "desired", "0"], {"temperature": quantized})
-    if kind == "temperature":
+        if kind == "temperature_ocf":
+            return (["temperature", "desired", "0"], {"temperature": quantized})
        # Vendor items[] array; only one item observed on every AC dump, id '0'.
-        quantized = _quantize_temperature(value, _temperature_step(resources))
-        if quantized is None:
-            return None
        return (
            ["temperatures", "vs", "0"],
            {
@@ -181,6 +181,26 @@ def test_climate_write_preserves_half_degree_temperature_steps():
    )


+def test_temperature_step_falls_back_to_temps_vs_items_when_no_control_resource():
+    """Isolates the /temperatures/vs/0 fallback: every fixture that carries
+    an increment there also carries /temperature/control/vs/0, which
+    _temperature_step checks first -- so without dropping that resource,
+    this fallback branch is never actually exercised, and reinstating the
+    original bug (reading the increment off /temperatures/vs/0's top level
+    instead of unwrapping its items[0]) would still pass every other test."""
+    resources = dict(_load_device("airconditioner_cac"))
+    del resources[airconditioner.HREF_TEMP_CONTROL]
+    assert airconditioner._temperature_step(resources) == 0.5
+    assert airconditioner._climate_write(("temperature", 24.5), {}, None, resources) == (
+        ["temperatures", "vs", "0"],
+        {
+            "x.com.samsung.da.items": [
+                {"x.com.samsung.da.id": "0", "x.com.samsung.da.desired": "24.5"}
+            ]
+        },
+    )
+
+
 def test_climate_write_rounds_to_whole_degree_with_no_advertised_increment():
    """ARTIK051 boards have neither /temperature/control/vs/0 nor an
    increment field on /temperatures/vs/0's item -- target_temperature_step
@@ -201,3 +201,33 @@ async def test_main_climate_write_unaffected_by_subdevice_translation(coordinato

    posted_path, _ = coordinator._session.post_calls[0]
    assert posted_path == ["power", "vs", "0"]
+
+
+async def test_indexed_subdevice_climate_write_quantizes_with_its_own_temperature_step(
+    coordinator,
+) -> None:
+    """A composite AC's write_fn (airconditioner._climate_write) reads the
+    temperature step off write_fn's own `resources` argument -- if
+    async_send_command handed it the raw cache snapshot (real, on-the-wire
+    hrefs) instead of this subdevice's canonical_resources() view, the
+    subdevice's own /temperature/control/vs/1 would be invisible under the
+    canonical HREF_TEMP_CONTROL key, and the write would quantize against
+    the master's step (or none at all) instead of its own."""
+    sub1 = Subdevice(kind="indexed", key="1", seed_path=("device", "1"))
+    coordinator._observe.apply(
+        "/temperature/control/vs/0",
+        {"x.com.samsung.da.increment": "1"},
+        source="poll",
+    )
+    coordinator._observe.apply(
+        "/temperature/control/vs/1",
+        {"x.com.samsung.da.increment": "0.5"},
+        source="poll",
+    )
+    bound = _climate_bound("/mode/vs/1", sub1)
+
+    await coordinator.async_send_command(bound, ("temperature_ocf", 24.5))
+
+    posted_path, posted_bytes = coordinator._session.post_calls[0]
+    assert posted_path == ["temperature", "desired", "1"]
+    assert cbor2.loads(posted_bytes) == {"temperature": 24.5}
@@ -164,15 +164,23 @@ def test_confirmed_washer_table_02_towels_bedding_are_not_swapped():
    """Issue #343: DA_WM_TP1_21_COMMON's Table_02 had 24/33 transposed --
    selecting 'Towels' in HA ran the washer's Bedding cycle and vice versa.
    24 must agree with the 69/6A-79/88 family's own Bedding/Towels pair
-    (6f/70), not with each other."""
-    states = _load("en")["entity"]["select"]["washer_cycle_table_02"]["state"]
-    assert states["24"] == states["6f"] == "Bedding"
-    assert states["33"] == states["70"] == "Towels"
+    (6f/70), not with each other.
+
+    Checked in every locale catalog, not just English: the underlying bug
+    is a device-code mapping error, not a wording issue, and
+    test_every_language_mirrors_the_english_catalog only checks key
+    topology -- a locale-specific 24/33 swap (the exact shape of #343)
+    would still pass that test."""
+    for language in _languages():
+        states = _load(language)["entity"]["select"]["washer_cycle_table_02"]["state"]
+        assert states["24"] == states["6f"], language
+        assert states["33"] == states["70"], language


 def test_confirmed_washer_table_02_missing_course_names():
    """Issue #342: 06/08/a0 had no translation and fell back to the raw
-    device code in the UI."""
+    device code in the UI; 74 was already translated by the time this
+    landed and is pinned here only as a "didn't regress" anchor."""
    states = _load("en")["entity"]["select"]["washer_cycle_table_02"]["state"]
    assert {code: states[code] for code in ("06", "08", "74", "a0")} == {
        "06": "XXL Laundry",