diff --git a/custom_components/localthings/registry/capabilities/operational.py b/custom_components/localthings/registry/capabilities/operational.py index ec64a33..440cc8d 100644 --- a/custom_components/localthings/registry/capabilities/operational.py +++ b/custom_components/localthings/registry/capabilities/operational.py @@ -62,17 +62,18 @@ 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 replays a running stage ('Drying') for a few + seconds after Finish while `state` already reads idle, and a bypass + keyed on the progress code alone read that tail 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 +191,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 +205,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 +218,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..6be0266 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, 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. + # 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..f928e21 100644 --- a/custom_components/localthings/sensor.py +++ b/custom_components/localthings/sensor.py @@ -82,52 +82,47 @@ 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". + Edge-triggered, not level-triggered: the window (re)starts only on + a fresh False->True transition of `sticky_fn`, is never restarted + while already open, and is checked for expiry on every call even + while `sticky_fn` keeps matching -- so neither a field stuck + matching forever (the quirk `_completion_minutes` works around) + nor one flapping in and out can hold the value past + `sticky_seconds` from when it first armed. - `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". Being the early-release + path it should demand positive evidence of the latter; 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) + holding = self._sticky_until is not None and now < self._sticky_until - if matches: - if not self._sticky_armed: + if desc.sticky_fn(rep): + if not self._sticky_armed and not holding: 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 + holding = True 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 + return raw - if self._sticky_until is not None and now < self._sticky_until: - return self._sticky_value - return raw + 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..a68b536 100644 --- a/tests/test_sensor_sticky.py +++ b/tests/test_sensor_sticky.py @@ -165,22 +165,100 @@ 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 replays a running stage after + Finish -- observed twice, identically, as Cooling -> +60s Finish -> + +24s 'Drying' -> +4s settled, with `state` already idle throughout the + tail. The hold must survive it, 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_the_window_forward(): + """Edge-triggering stops a *stuck* Finish from extending the hold, but + a device whose progress flaps out of and back into Finish re-arms -- + an already-open window must not restart on that, or the bound stops + being a bound.""" + desc = replace(_PROGRESS_DESC, sticky_seconds=0.1) + sensor, coordinator = _sensor(desc) + + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + for _ in range(3): + time.sleep(0.03) + _replace(coordinator, state="Ready", progress="None") + assert sensor.native_value == "Finish" + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Finish" + + # 0.09s of flapping so far; past 0.1s from the *first* Finish it ends, + # rather than 0.1s from the most recent re-arm. + time.sleep(0.03) + _replace(coordinator, state="Ready", progress="Finish") + assert sensor.native_value == "Idle" + + 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