diff --git a/custom_components/localthings/registry/capabilities/operational.py b/custom_components/localthings/registry/capabilities/operational.py index 8494e6c..ec64a33 100644 --- a/custom_components/localthings/registry/capabilities/operational.py +++ b/custom_components/localthings/registry/capabilities/operational.py @@ -35,12 +35,44 @@ def _int(v): return None +def _state_is_active(rep): + return _SAMSUNG_STATE_TO_OCF.get(rep.get("x.com.samsung.da.state")) == "active" + + def _is_active(rep): """Check if appliance is actively running and cycle is not finished.""" - return ( - _SAMSUNG_STATE_TO_OCF.get(rep.get("x.com.samsung.da.state")) == "active" - and rep.get("x.com.samsung.da.progress") != "Finish" - ) + return _state_is_active(rep) and rep.get("x.com.samsung.da.progress") != "Finish" + + +def _just_finished(rep): + """progress/progress_percentage's sticky_fn (issue #345, see sensor.py's + _apply_sticky): arms their grace window the moment `progress` reads + 'Finish'. + + Deliberately not also requiring `state == 'active'` in the same rep, + unlike _is_active above: #345 reports a washer whose `state` can + already read idle by the time `progress` is observed at 'Finish' -- + the same-family dryer's firmware apparently keeps `state` at 'active' + longer, per the report -- so requiring both together risked the arm + condition never actually firing on the one device this fixes. + _apply_sticky's edge-triggering (only a fresh False->True transition + (re)arms) is what keeps a `progress` stuck at 'Finish' indefinitely + (the same class of quirk `_completion_minutes` below already works + around) from holding this open forever instead.""" + return rep.get("x.com.samsung.da.progress") == "Finish" + + +def _live_progress_code(rep): + """progress/progress_percentage's sticky_bypass_fn: a concrete, + non-Finish progress code being reported right now -- e.g. a new + cycle's own real 'Wash'/'Spin' -- must win over a still-open hold from + the previous cycle immediately. Not keyed on `state` (unlike + _is_active): _just_finished's whole premise is that `state` can't be + trusted to still say 'active' while a fresh, real progress value is + already there, and the same applies to recognizing when it's moved on + to a new one -- including while paused, e.g. adding a sock mid-hold.""" + v = rep.get("x.com.samsung.da.progress") + return v is not None and v not in ("None", "Finish") def _remaining_seconds(raw): @@ -153,19 +185,31 @@ OPERATIONAL_STATE = Capability( BinarySensorDesc( key="cycle_active", device_class="running", - rep_fn=lambda rep: ( - _SAMSUNG_STATE_TO_OCF.get(rep.get("x.com.samsung.da.state")) == "active" - and rep.get("x.com.samsung.da.progress") != "Finish" - ), + rep_fn=_is_active, ), + # sticky_* (issue #345): once progress reads 'Finish', keep + # showing Finish/100 for a grace window even after machine_state + # reverts, rather than falling to Idle/0 the instant it does -- + # see sensor.py's _apply_sticky. rep_fn below is otherwise + # unchanged; the hold is entirely a read-side, per-entity concern, + # deliberately not gated on machine_state (_just_finished's + # docstring explains why). sticky_live_fn reads the raw field the + # same ungated way, for sticky_bypass_fn's benefit: a real + # progress value reported while paused (e.g. adding a sock + # mid-cycle) must win over a stale hold even though rep_fn itself + # would show "Idle"/0 there. SensorDesc( key="progress", icon="mdi:progress-wrench", rep_fn=lambda rep: ( "Idle" - if _SAMSUNG_STATE_TO_OCF.get(rep.get("x.com.samsung.da.state")) != "active" + if not _state_is_active(rep) else _progress(rep.get("x.com.samsung.da.progress")) ), + sticky_fn=_just_finished, + sticky_value_fn=lambda rep: "Finish", + sticky_live_fn=lambda rep: _progress(rep.get("x.com.samsung.da.progress")), + sticky_bypass_fn=_live_progress_code, ), SensorDesc( key="progress_percentage", @@ -173,9 +217,13 @@ OPERATIONAL_STATE = Capability( state_class="measurement", rep_fn=lambda rep: ( 0 - if _SAMSUNG_STATE_TO_OCF.get(rep.get("x.com.samsung.da.state")) != "active" + if not _state_is_active(rep) else _int(rep.get("x.com.samsung.da.progressPercentage")) ), + sticky_fn=_just_finished, + sticky_value_fn=lambda rep: 100, + sticky_live_fn=lambda rep: _int(rep.get("x.com.samsung.da.progressPercentage")) or 0, + sticky_bypass_fn=_live_progress_code, ), # Only show finish time while actively running -- firmware leaves a # stale remainingTime after a cycle ends, frozen at '00:01:00'. diff --git a/custom_components/localthings/registry/entities.py b/custom_components/localthings/registry/entities.py index fdfdb02..35178f4 100644 --- a/custom_components/localthings/registry/entities.py +++ b/custom_components/localthings/registry/entities.py @@ -64,6 +64,18 @@ class SensorDesc(SamsungEntityDescription): # (see sensor.py). Only for values expected to jitter between # device-side revisions -- not a general-purpose flag. hysteresis: bool = False + # Opt-in, entity-instance-only hold -- see sensor.py's _apply_sticky + # for the full contract (arm/value/live/bypass semantics, + # edge-triggering, why this never touches the coordinator cache). + # sticky_fn arms it; sticky_value_fn picks what to freeze at that + # moment (defaults to rep_fn's own result); sticky_bypass_fn forces + # sticky_live_fn's result through and drops the hold; sticky_seconds + # bounds how long it can hold. + sticky_fn: Callable[[dict], bool] | None = None + sticky_value_fn: Callable[[dict], Any] | None = None + sticky_live_fn: Callable[[dict], Any] | None = None + sticky_bypass_fn: Callable[[dict], bool] | None = None + sticky_seconds: float = 300.0 @dataclass(frozen=True, kw_only=True) diff --git a/custom_components/localthings/sensor.py b/custom_components/localthings/sensor.py index eccbb3e..5aec7ae 100644 --- a/custom_components/localthings/sensor.py +++ b/custom_components/localthings/sensor.py @@ -2,6 +2,7 @@ from __future__ import annotations +import time from datetime import timedelta from typing import cast @@ -51,6 +52,9 @@ class LocalThingsSensor(LocalThingsEntity, SensorEntity): if desc.options: self._attr_options = list(desc.options) self._hysteresis_value = None + self._sticky_value = None + self._sticky_until: float | None = None + self._sticky_armed = False @property def native_unit_of_measurement(self): @@ -63,10 +67,68 @@ class LocalThingsSensor(LocalThingsEntity, SensorEntity): def native_value(self): raw = (self.coordinator.data or {}).get(self._state_key) desc = cast(SensorDesc, self._bound.desc) + if desc.sticky_fn is not None: + raw = self._apply_sticky(raw, desc) if not desc.hysteresis: return raw return self._apply_hysteresis(raw) + def _apply_sticky(self, raw, desc: SensorDesc): + """Freeze this entity at a value for up to `desc.sticky_seconds` + after `desc.sticky_fn` next stops matching this href's live rep + (issue #345 -- see operational.py's `_just_finished` for the + motivating case). Entity-instance state only, exactly like + `_hysteresis_value` above -- never written back to the coordinator + cache, so write_fn, diagnostics, and the observe-mode sweep + comparison keep seeing real device data throughout. + + Reads `sticky_fn` and friends against this href's own live rep, + not the already-computed `raw`, so they can be independent of + whatever rep_fn itself gates on -- notably `sticky_live_fn`, which + is *not* `raw`: `raw` is rep_fn's own (possibly differently gated) + result, e.g. progress's rep_fn shows "Idle" while paused, but a + real progress value reported while paused (adding a sock mid- + cycle) must still win over a stale hold, which means reading it + ungated here rather than through that gate. + + Edge-triggered, not level-triggered: the window only (re)starts on + a fresh False->True transition of `sticky_fn`, and -- this is the + part level-triggering alone misses -- expiry is still checked on + every call even while `sticky_fn` keeps matching. Without the + latter, a firmware that leaves the underlying field stuck matching + forever (the same class of quirk `_completion_minutes` already + works around) would show the frozen value forever too, defeating + "bounded, not indefinite". + + `sticky_bypass_fn`, when it matches, always passes + `sticky_live_fn`'s result through and drops any hold -- for a + condition where "not sticky right now" is ambiguous between "went + idle, honor the hold" and "genuinely live, different data" (e.g. a + new cycle's own real progress), which "consult the hold whenever + sticky_fn is False" alone can't tell apart. + """ + assert desc.sticky_fn is not None # native_value only calls this when set + rep = self.coordinator.resource(self._bound.href) + now = time.monotonic() + matches = desc.sticky_fn(rep) + + if matches: + if not self._sticky_armed: + self._sticky_value = ( + desc.sticky_value_fn(rep) if desc.sticky_value_fn is not None else raw + ) + self._sticky_until = now + desc.sticky_seconds + self._sticky_armed = True + else: + self._sticky_armed = False + if desc.sticky_bypass_fn is not None and desc.sticky_bypass_fn(rep): + self._sticky_until = None + return desc.sticky_live_fn(rep) if desc.sticky_live_fn is not None else raw + + if self._sticky_until is not None and now < self._sticky_until: + return self._sticky_value + return raw + def _apply_hysteresis(self, raw): """Hold the last value this entity actually reported until a new one differs by at least the configured threshold, regardless of how long diff --git a/tests/test_operational_capability.py b/tests/test_operational_capability.py index 8411557..afca8fd 100644 --- a/tests/test_operational_capability.py +++ b/tests/test_operational_capability.py @@ -1,6 +1,10 @@ """Unit tests for operational state capabilities.""" -from custom_components.localthings.registry.capabilities.operational import OPERATIONAL_STATE +from custom_components.localthings.registry.capabilities.operational import ( + OPERATIONAL_STATE, + _just_finished, + _live_progress_code, +) from custom_components.localthings.registry.entities import NumberDesc @@ -11,6 +15,55 @@ def test_machine_state_maps_samsung_to_ocf(): assert ms.value_fn("Ready") == "idle" +class TestJustFinished: + """`_just_finished` is progress/progress_percentage's sticky_fn arm + condition (issue #345) -- see sensor.py's _apply_sticky.""" + + def test_true_when_progress_is_finish_while_active(self): + assert _just_finished( + {"x.com.samsung.da.state": "Run", "x.com.samsung.da.progress": "Finish"} + ) + + def test_true_even_once_state_has_already_left_active(self): + """The exact issue #345 scenario this exists for: a washer's + `state` can already read idle by the time `progress` is observed + at 'Finish' -- unlike _is_active, this must still arm.""" + assert _just_finished( + {"x.com.samsung.da.state": "Ready", "x.com.samsung.da.progress": "Finish"} + ) + + def test_false_while_active_but_not_yet_finished(self): + assert not _just_finished( + {"x.com.samsung.da.state": "Run", "x.com.samsung.da.progress": "Spin"} + ) + + def test_false_when_progress_absent(self): + assert not _just_finished({"x.com.samsung.da.state": "Run"}) + + +class TestLiveProgressCode: + """`_live_progress_code` is progress/progress_percentage's + sticky_bypass_fn (issue #345) -- see sensor.py's _apply_sticky.""" + + def test_true_for_a_concrete_non_finish_code(self): + assert _live_progress_code({"x.com.samsung.da.progress": "Wash"}) + + def test_true_regardless_of_state(self): + """Not gated on machine_state -- a new cycle's own real progress + must win over a held hold even while paused (e.g. adding a sock + mid-hold), not just while actively running.""" + assert _live_progress_code( + {"x.com.samsung.da.state": "Pause", "x.com.samsung.da.progress": "Wash"} + ) + + def test_false_for_finish(self): + assert not _live_progress_code({"x.com.samsung.da.progress": "Finish"}) + + def test_false_when_absent_or_none(self): + assert not _live_progress_code({}) + assert not _live_progress_code({"x.com.samsung.da.progress": "None"}) + + class TestProgressPercentage: """issue #9: device firmware leaves progressPercentage stale (e.g. '1') after a cycle ends instead of resetting it, so it must be gated on diff --git a/tests/test_sensor_sticky.py b/tests/test_sensor_sticky.py new file mode 100644 index 0000000..89e48b8 --- /dev/null +++ b/tests/test_sensor_sticky.py @@ -0,0 +1,272 @@ +"""Unit tests for LocalThingsSensor's sticky gate (issue #345). + +Modeled on test_sensor_hysteresis.py: a _FakeCoordinator with just enough +surface for LocalThingsEntity/LocalThingsSensor, driven directly rather +than through a full coordinator/HA setup. Unlike that file, `data` here +is derived from the real `flatten()` over the same `resources` dict +`resource()` serves -- a single source of truth, so a test can't hide a +production desync bug by hand-computing the two independently. +""" + +from __future__ import annotations + +import time +from dataclasses import replace +from typing import cast + +from custom_components.localthings.coordinator import LocalThingsCoordinator +from custom_components.localthings.registry.adapter import flatten +from custom_components.localthings.registry.capabilities.operational import OPERATIONAL_STATE +from custom_components.localthings.registry.discovery import BoundEntity +from custom_components.localthings.sensor import LocalThingsSensor + +_PROGRESS_DESC = next(e for e in OPERATIONAL_STATE.entities if e.key == "progress") +_PROGRESS_PERCENTAGE_DESC = next( + e for e in OPERATIONAL_STATE.entities if e.key == "progress_percentage" +) +_MACHINE_STATE_DESC = next(e for e in OPERATIONAL_STATE.entities if e.key == "machine_state") + +_HREF = "/operational/state/vs/0" +_ALL_BOUND = [ + BoundEntity(href=_HREF, capability=OPERATIONAL_STATE, desc=desc) + for desc in OPERATIONAL_STATE.entities +] + + +class _FakeConfigEntry: + def __init__(self): + self.options: dict = {} + + +class _FakeCoordinator: + """Just enough surface for LocalThingsEntity/LocalThingsSensor. + + `resources` is the one source of truth (standing in for the real + coordinator's live cache); `data` and `resource()` are both derived + from it exactly as the real coordinator derives `.data` from + `flatten(self.bound, self._cache.snapshot())` and `.resource()` from + the same snapshot -- so a test can't drift them apart in a way + production couldn't. + """ + + def __init__(self): + self.device_serial = "TEST-SERIAL" + self.config_entry = _FakeConfigEntry() + self.resources: dict[str, dict] = {} + + def resource(self, href: str) -> dict: + return self.resources.get(href) or {} + + @property + def data(self) -> dict: + return flatten(_ALL_BOUND, self.resources) + + +def _sensor(desc): + coordinator = _FakeCoordinator() + bound = BoundEntity(href=_HREF, capability=OPERATIONAL_STATE, desc=desc) + sensor = LocalThingsSensor(cast(LocalThingsCoordinator, coordinator), bound) + return sensor, coordinator + + +def _apply(coordinator, **fields): + """Merge `fields` onto the href's existing rep, the same shallow + {**cached, **rep} merge ObserveManager.apply() does in production + (issue #27) -- so a field this call doesn't mention (e.g. a stale + `progress` the device didn't repeat) stays exactly as a real partial + update would leave it, rather than being silently wiped.""" + href_fields = {f"x.com.samsung.da.{k}": v for k, v in fields.items()} + coordinator.resources[_HREF] = {**coordinator.resources.get(_HREF, {}), **href_fields} + + +def _replace(coordinator, **fields): + """A full rep replacement -- for a device reporting fresh from + scratch (e.g. a full poll GET), unlike _apply's partial-update merge.""" + coordinator.resources[_HREF] = {f"x.com.samsung.da.{k}": v for k, v in fields.items()} + + +def test_holds_finish_after_state_leaves_active(): + """The #345 regression this guards: progress used to fall straight to + 'Idle' the instant machine_state left active, even right after + reporting Finish.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + _replace(coordinator, state="Ready") # device has moved on + assert sensor.native_value == "Finish" + + +def test_holds_finish_even_when_state_already_idle_at_first_observation(): + """issue #345's actual reported failure mode: state can already read + idle by the time `progress: Finish` is ever observed (the report's + washer skips a Run+Finish moment the same-family dryer still shows) -- + the hold must still arm from that single rep.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Ready", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + # Still held on a later poll, even once the device stops repeating it. + _replace(coordinator, state="Ready") + assert sensor.native_value == "Finish" + + +def test_progress_percentage_holds_100_regardless_of_the_raw_field_at_finish(): + """The hold pins 100 explicitly (sticky_value_fn), not whatever the + raw field happened to hold at the finish moment -- a device that + never populates progressPercentage at all, or (issue #9's failure + mode) leaves a stale non-100 value there, must still show 100 while + held, not None/unknown or the stale figure.""" + sensor, coordinator = _sensor(_PROGRESS_PERCENTAGE_DESC) + + _replace(coordinator, state="Run", progress="Finish") # no progressPercentage at all + assert sensor.native_value == 100 + + _replace(coordinator, state="Ready") + assert sensor.native_value == 100 + + +def test_real_data_flows_through_unheld_while_active(): + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Spin") + assert sensor.native_value == "Spin" + + _replace(coordinator, state="Run", progress="Rinse") + assert sensor.native_value == "Rinse" + + +def test_never_finished_stays_idle(): + """A device that never actually reached Finish (e.g. Stop pressed + mid-cycle) must not be held at some prior mid-cycle value.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Spin") + assert sensor.native_value == "Spin" + + _replace(coordinator, state="Ready") + assert sensor.native_value == "Idle" + + +def test_a_new_cycle_starting_overrides_the_hold(): + """Starting a second cycle must show its own real progress + immediately, not the held Finish from the previous one.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + _replace(coordinator, state="Ready") + assert sensor.native_value == "Finish" # still held + + _replace(coordinator, state="Run", progress="Wash") + assert sensor.native_value == "Wash" + + +def test_a_paused_new_cycle_also_overrides_the_hold(): + """Not just an actively-running new cycle: adding a sock and pausing + mid-cycle must also show the real, current progress rather than a + stale hold from the previous cycle -- machine_state isn't 'active' + while paused, so a bypass keyed on that alone would miss this.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + _replace(coordinator, state="Ready") + assert sensor.native_value == "Finish" # still held + + _replace(coordinator, state="Pause", progress="Wash") + assert sensor.native_value == "Wash" + + +def test_hold_expires_after_sticky_seconds(): + # A fresh desc (frozen dataclass -- replace(), not mutation, so the + # module-level _PROGRESS_DESC other tests share stays untouched) with + # a short real window -- consistent with this codebase's own + # settle-guard tests (test_observe.py's mark_write_pending(..., + # settle_s=0.05)) -- rather than a zero window, which exercises "held + # disabled" rather than "an armed hold actually expires". + desc = replace(_PROGRESS_DESC, sticky_seconds=0.05) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + _replace(coordinator, state="Ready") + assert sensor.native_value == "Finish" # still within the window + + time.sleep(0.1) + assert sensor.native_value == "Idle" + + +def test_a_progress_stuck_at_finish_does_not_hold_open_the_window_forever(): + """Edge-triggering: sticky_fn keeps matching on every poll if the + device's own `progress` field never resets on its own (the same class + of quirk _completion_minutes' remainingTime-freeze workaround already + documents) -- the hold must still expire on schedule from its first + sighting, not have its deadline pushed out by every subsequent poll + that still (correctly, from the device's perspective) reports Finish + while machine_state has already gone idle.""" + desc = replace(_PROGRESS_DESC, sticky_seconds=0.05) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + time.sleep(0.03) + # Device still (incorrectly) reports Finish on every subsequent poll -- + # must not restart the window. + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + time.sleep(0.03) # 0.06s total since the first sighting -- past 0.05s + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Idle" + + +def test_non_sticky_sensor_is_unaffected(): + """machine_state has no sticky_fn -- reads straight through, unchanged.""" + sensor, coordinator = _sensor(_MACHINE_STATE_DESC) + _replace(coordinator, state="Run") + assert sensor.native_value == "active" + _replace(coordinator, state="Ready") + assert sensor.native_value == "idle" + + +def test_cycle_active_and_machine_state_are_never_held(): + """The whole point of scoping the hold to progress/progress_percentage + only: cycle_active (Running) and machine_state must keep reflecting + the device's real-time state throughout, never claiming the appliance + is still running once it isn't.""" + progress_sensor, coordinator = _sensor(_PROGRESS_DESC) + machine_state_bound = BoundEntity( + href=_HREF, capability=OPERATIONAL_STATE, desc=_MACHINE_STATE_DESC + ) + machine_state_sensor = LocalThingsSensor( + cast(LocalThingsCoordinator, coordinator), machine_state_bound + ) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert progress_sensor.native_value == "Finish" + assert machine_state_sensor.native_value == "active" + + _replace(coordinator, state="Ready") + assert progress_sensor.native_value == "Finish" # held + assert machine_state_sensor.native_value == "idle" # real-time, unaffected + + +def test_a_partial_update_that_omits_progress_does_not_erase_the_hold(): + """A device's partial poll/notify that doesn't repeat `progress` at + all (issue #27's documented shape) merges onto the cache rather than + replacing it -- confirms the hold reads the merged rep, still showing + 'Finish' from the earlier full rep, not a wiped/absent field.""" + sensor, coordinator = _sensor(_PROGRESS_DESC) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + _apply(coordinator, state="Ready") # partial merge, doesn't restate progress + assert coordinator.resources[_HREF]["x.com.samsung.da.progress"] == "Finish" + assert sensor.native_value == "Finish"