diff --git a/custom_components/localthings/registry/capabilities/operational.py b/custom_components/localthings/registry/capabilities/operational.py index ec64a33..e54db0c 100644 --- a/custom_components/localthings/registry/capabilities/operational.py +++ b/custom_components/localthings/registry/capabilities/operational.py @@ -62,17 +62,21 @@ def _just_finished(rep): 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.""" +def _new_cycle_running(rep): + """progress/progress_percentage's sticky_bypass_fn: drop the #345 hold + early once a new cycle is genuinely running. + + Gated on `state == 'active'`, unlike _just_finished's arm condition + above: issue #358's dryer resets `progress` to its course's first + stage ('Drying') in the same moment `state` goes idle, ~4s before + settling to 'None' -- confirmed by the reporter's machine_state + history, which flips to idle on the exact second progress reads + 'Drying', in both captured cycles. A bypass keyed on the progress + code alone read that as a new cycle and republished it. Releasing + late costs nothing -- an unreleased hold still expires on its own -- + so this side takes the stronger signal.""" v = rep.get("x.com.samsung.da.progress") - return v is not None and v not in ("None", "Finish") + return _state_is_active(rep) and v is not None and v not in ("None", "Finish") def _remaining_seconds(raw): @@ -190,14 +194,10 @@ OPERATIONAL_STATE = Capability( # 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. + # see sensor.py's _apply_sticky. rep_fn below is unchanged and + # stays the only definition of a live value -- the hold decides + # only *whether* to freeze. A second, ungated one here is what + # let issue #358's post-Finish tail reach the entity. SensorDesc( key="progress", icon="mdi:progress-wrench", @@ -208,8 +208,7 @@ OPERATIONAL_STATE = Capability( ), 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, + sticky_bypass_fn=_new_cycle_running, ), SensorDesc( key="progress_percentage", @@ -222,8 +221,7 @@ OPERATIONAL_STATE = Capability( ), 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, + sticky_bypass_fn=_new_cycle_running, ), # 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 35178f4..57e6149 100644 --- a/custom_components/localthings/registry/entities.py +++ b/custom_components/localthings/registry/entities.py @@ -65,15 +65,15 @@ class SensorDesc(SamsungEntityDescription): # 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). + # for the full contract (arm/value/bypass semantics, one window per + # bypass, 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. + # moment (defaults to rep_fn's own result); sticky_bypass_fn drops the + # hold and lets rep_fn's own live result through; sticky_seconds + # bounds how long it can hold. There is deliberately no hook for + # computing a live value differently from rep_fn -- see issue #358. 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 diff --git a/custom_components/localthings/sensor.py b/custom_components/localthings/sensor.py index 5aec7ae..7f6e58e 100644 --- a/custom_components/localthings/sensor.py +++ b/custom_components/localthings/sensor.py @@ -54,7 +54,7 @@ class LocalThingsSensor(LocalThingsEntity, SensorEntity): self._hysteresis_value = None self._sticky_value = None self._sticky_until: float | None = None - self._sticky_armed = False + self._sticky_spent = False @property def native_unit_of_measurement(self): @@ -82,52 +82,49 @@ class LocalThingsSensor(LocalThingsEntity, SensorEntity): 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. + `sticky_fn`/`sticky_bypass_fn` read this href's live rep rather + than the already-computed `raw`, so they can key on fields rep_fn + has collapsed away -- but they never compute a value. `raw` and + the frozen `sticky_value` are the only things returned here, so a + held entity and a free-running one agree on what "live" means; a + hook that broke that rule caused issue #358. - 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". + At most one window per `sticky_bypass_fn` cycle: arming marks the + hold spent, and only the bypass clears that. So a `sticky_fn` that + keeps matching (firmware leaving the field stuck -- the quirk + `_completion_minutes` works around) can't extend the window, and + one flapping in and out can't restart it either, before or after + expiry. Expiry alone doesn't re-open the door: without something + the calibre of "a new cycle is actually running" in between, a + second Finish is the same Finish, and re-arming on it would strobe + the entity between held and live once per window -- exactly the + repeated announcements #345 and #358 are about. - `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. + `sticky_bypass_fn` drops the hold and returns `raw`, for when "not + sticky right now" is ambiguous between "went idle, honor the hold" + and "genuinely moved on to new data". It is both the early release + and the only re-arm, so it should demand positive evidence of that + move; when unsure, letting the window run out is the cheaper + mistake. """ 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: + if desc.sticky_fn(rep): + if not self._sticky_spent: 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 + self._sticky_spent = True + elif desc.sticky_bypass_fn is not None and desc.sticky_bypass_fn(rep): + self._sticky_until = None + self._sticky_spent = False + return raw - if self._sticky_until is not None and now < self._sticky_until: - return self._sticky_value - return raw + holding = self._sticky_until is not None and now < self._sticky_until + return self._sticky_value if holding else raw def _apply_hysteresis(self, raw): """Hold the last value this entity actually reported until a new one diff --git a/tests/test_operational_capability.py b/tests/test_operational_capability.py index afca8fd..4afebca 100644 --- a/tests/test_operational_capability.py +++ b/tests/test_operational_capability.py @@ -3,7 +3,7 @@ from custom_components.localthings.registry.capabilities.operational import ( OPERATIONAL_STATE, _just_finished, - _live_progress_code, + _new_cycle_running, ) from custom_components.localthings.registry.entities import NumberDesc @@ -41,27 +41,46 @@ class TestJustFinished: 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.""" +class TestNewCycleRunning: + """`_new_cycle_running` is progress/progress_percentage's + sticky_bypass_fn -- the early-release condition for the #345 hold. + 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_for_a_concrete_non_finish_code_while_active(self): + assert _new_cycle_running( + {"x.com.samsung.da.state": "Run", "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( + def test_false_for_a_running_stage_reported_after_state_left_active(self): + """Issue #358, the whole reason for the `state` gate: the reporting + dryer replays a running stage ('Drying', its first supportedProgress + entry) for a few seconds after Finish while winding down. That is + the finished cycle's tail, not a new cycle -- releasing the hold on + it is what produced 'Drying, Cooling, Finish, Drying, Idle'.""" + assert not _new_cycle_running( + {"x.com.samsung.da.state": "Ready", "x.com.samsung.da.progress": "Drying"} + ) + + def test_false_while_paused(self): + """Paused is not evidence a new cycle is running, and the tail + above can't be told apart from it. rep_fn shows 'Idle' whenever + state isn't active anyway, so there is no live value being + withheld here -- only a hold that expires on its own instead of + being released early.""" + assert not _new_cycle_running( {"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"}) + assert not _new_cycle_running( + {"x.com.samsung.da.state": "Run", "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"}) + assert not _new_cycle_running({}) + assert not _new_cycle_running( + {"x.com.samsung.da.state": "Run", "x.com.samsung.da.progress": "None"} + ) class TestProgressPercentage: diff --git a/tests/test_sensor_sticky.py b/tests/test_sensor_sticky.py index 89e48b8..0627a9b 100644 --- a/tests/test_sensor_sticky.py +++ b/tests/test_sensor_sticky.py @@ -165,22 +165,134 @@ def test_a_new_cycle_starting_overrides_the_hold(): 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) +def test_a_running_stage_after_finish_does_not_break_the_hold(): + """Issue #358: the reporting dryer resets `progress` to its course's + first stage in the same moment `state` goes idle -- observed twice, + identically, as Cooling -> +60s Finish -> +24s 'Drying' -> +4s + settled, with the reporter's machine_state history flipping to idle on + the exact second progress reads 'Drying'. The hold must survive that, + so the cycle still reads Drying, Cooling, Finish, Idle rather than the + reported Drying, Cooling, Finish, Drying, Idle.""" + desc = replace(_PROGRESS_DESC, sticky_seconds=0.2) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Run", progress="Drying", progressPercentage="40") + assert sensor.native_value == "Drying" + + _replace(coordinator, state="Run", progress="Cooling", progressPercentage="95") + assert sensor.native_value == "Cooling" + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == "Finish" + + # The tail: a running stage again, state already idle. + _replace(coordinator, state="Ready", progress="Drying", progressPercentage="100") + assert sensor.native_value == "Finish" + + # ...then the device settles, still inside the window. + _replace(coordinator, state="Ready", progress="None") + assert sensor.native_value == "Finish" + + time.sleep(0.25) + assert sensor.native_value == "Idle" + + +def test_progress_percentage_survives_the_same_tail(): + """#358's tail hits progress_percentage through the identical bypass; + it must stay pinned at 100 rather than being released back to a raw + mid-cycle figure.""" + desc = replace(_PROGRESS_PERCENTAGE_DESC, sticky_seconds=0.2) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Run", progress="Finish", progressPercentage="100") + assert sensor.native_value == 100 + + _replace(coordinator, state="Ready", progress="Drying", progressPercentage="40") + assert sensor.native_value == 100 + + time.sleep(0.25) + assert sensor.native_value == 0 + + +def test_a_paused_new_cycle_is_left_to_the_window_rather_than_released(): + """'Paused' isn't positive evidence of a new cycle, and #358's tail is + indistinguishable from it. Nothing live is withheld by waiting -- + rep_fn shows 'Idle' while paused with or without a hold -- so the + stale Finish just expires on schedule instead of being cut short.""" + 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 held _replace(coordinator, state="Pause", progress="Wash") + assert sensor.native_value == "Finish" # held out, not released + + time.sleep(0.1) + assert sensor.native_value == "Idle" # what a paused appliance always shows + + _replace(coordinator, state="Run", progress="Wash") assert sensor.native_value == "Wash" +def test_a_flapping_finish_cannot_ratchet_an_open_window_forward(): + """Edge-triggering stops a *stuck* Finish from extending the hold; a + progress that flaps out of and back into Finish must not restart it + either, or the bound stops being a bound.""" + desc = replace(_PROGRESS_DESC, sticky_seconds=0.3) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + for _ in range(3): + time.sleep(0.05) + _replace(coordinator, state="Ready", progress="None") + assert sensor.native_value == "Finish" + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + # 0.15s of flapping so far -- the window still ends 0.3s after the + # first Finish, not 0.3s after the most recent re-entry. + time.sleep(0.2) + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Idle" + + +def test_a_finish_after_the_window_closes_does_not_re_arm_it(): + """Expiry doesn't re-open the door: with no new cycle in between, a + second Finish is the same Finish. Re-arming on it would strobe the + entity Finish -> Idle -> Finish once per window, re-firing exactly the + announcements #345 is about -- so it takes a bypass (a cycle actually + running) to make the hold available again. + + Distinct from the flap test above: there the window is still open, and + the last read before expiry leaves the sticky condition *matching*. + Here it has already closed, and the flap ends on a non-matching read, + which is the state a spent-on-arm-only guard would let re-arm.""" + 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.1) + assert sensor.native_value == "Idle" + + _replace(coordinator, state="Ready", progress="None") + assert sensor.native_value == "Idle" + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Idle" + + # A real cycle in between is what makes it available again. + _replace(coordinator, state="Run", progress="Drying") + assert sensor.native_value == "Drying" + _replace(coordinator, state="Run", progress="Finish") + assert sensor.native_value == "Finish" + _replace(coordinator, state="Ready", progress="None") + assert sensor.native_value == "Finish" + + 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