Merge pull request #359 from mbillow/claude/pr-346-regression-debug-a3x3pj

laundry: a post-Finish running stage is the cycle ending, not a new one

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):
+def _new_cycle_running(rep):
-    """progress/progress_percentage's sticky_bypass_fn: a concrete,
+    """progress/progress_percentage's sticky_bypass_fn: drop the #345 hold
-    non-Finish progress code being reported right now -- e.g. a new
+    early once a new cycle is genuinely running.
-    cycle's own real 'Wash'/'Spin' -- must win over a still-open hold from
+
-    the previous cycle immediately. Not keyed on `state` (unlike
+    Gated on `state == 'active'`, unlike _just_finished's arm condition
-    _is_active): _just_finished's whole premise is that `state` can't be
+    above: issue #358's dryer resets `progress` to its course's first
-    trusted to still say 'active' while a fresh, real progress value is
+    stage ('Drying') in the same moment `state` goes idle, ~4s before
-    already there, and the same applies to recognizing when it's moved on
+    settling to 'None' -- confirmed by the reporter's machine_state
-    to a new one -- including while paused, e.g. adding a sock mid-hold."""
+    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
+        # see sensor.py's _apply_sticky. rep_fn below is unchanged and
-        # unchanged; the hold is entirely a read-side, per-entity concern,
+        # stays the only definition of a live value -- the hold decides
-        # deliberately not gated on machine_state (_just_finished's
+        # only *whether* to freeze. A second, ungated one here is what
-        # docstring explains why). sticky_live_fn reads the raw field the
+        # let issue #358's post-Finish tail reach the entity.
-        # 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",
@@ -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=_new_cycle_running,
-            sticky_bypass_fn=_live_progress_code,
         ),
         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=_new_cycle_running,
-            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'.

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,
+    # for the full contract (arm/value/bypass semantics, one window per
-    # edge-triggering, why this never touches the coordinator cache).
+    # 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
+    # moment (defaults to rep_fn's own result); sticky_bypass_fn drops the
-    # sticky_live_fn's result through and drops the hold; sticky_seconds
+    # hold and lets rep_fn's own live result through; sticky_seconds
-    # bounds how long it can hold.
+    # 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
 

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,
+        `sticky_fn`/`sticky_bypass_fn` read this href's live rep rather
-        not the already-computed `raw`, so they can be independent of
+        than the already-computed `raw`, so they can key on fields rep_fn
-        whatever rep_fn itself gates on -- notably `sticky_live_fn`, which
+        has collapsed away -- but they never compute a value. `raw` and
-        is *not* `raw`: `raw` is rep_fn's own (possibly differently gated)
+        the frozen `sticky_value` are the only things returned here, so a
-        result, e.g. progress's rep_fn shows "Idle" while paused, but a
+        held entity and a free-running one agree on what "live" means; a
-        real progress value reported while paused (adding a sock mid-
+        hook that broke that rule caused issue #358.
-        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
+        At most one window per `sticky_bypass_fn` cycle: arming marks the
-        a fresh False->True transition of `sticky_fn`, and -- this is the
+        hold spent, and only the bypass clears that. So a `sticky_fn` that
-        part level-triggering alone misses -- expiry is still checked on
+        keeps matching (firmware leaving the field stuck -- the quirk
-        every call even while `sticky_fn` keeps matching. Without the
+        `_completion_minutes` works around) can't extend the window, and
-        latter, a firmware that leaves the underlying field stuck matching
+        one flapping in and out can't restart it either, before or after
-        forever (the same class of quirk `_completion_minutes` already
+        expiry. Expiry alone doesn't re-open the door: without something
-        works around) would show the frozen value forever too, defeating
+        the calibre of "a new cycle is actually running" in between, a
-        "bounded, not indefinite".
+        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_bypass_fn` drops the hold and returns `raw`, for when "not
-        `sticky_live_fn`'s result through and drops any hold -- for a
+        sticky right now" is ambiguous between "went idle, honor the hold"
-        condition where "not sticky right now" is ambiguous between "went
+        and "genuinely moved on to new data". It is both the early release
-        idle, honor the hold" and "genuinely live, different data" (e.g. a
+        and the only re-arm, so it should demand positive evidence of that
-        new cycle's own real progress), which "consult the hold whenever
+        move; when unsure, letting the window run out is the cheaper
-        sticky_fn is False" alone can't tell apart.
+        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 desc.sticky_fn(rep):
-            if not self._sticky_armed:
+            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
+                self._sticky_spent = True
-        else:
+        elif desc.sticky_bypass_fn is not None and desc.sticky_bypass_fn(rep):
-            self._sticky_armed = False
+            self._sticky_until = None
-            if desc.sticky_bypass_fn is not None and desc.sticky_bypass_fn(rep):
+            self._sticky_spent = False
-                self._sticky_until = None
+            return raw
-                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:
+        holding = self._sticky_until is not None and now < self._sticky_until
-            return self._sticky_value
+        return self._sticky_value if holding else raw
-        return raw
 
     def _apply_hysteresis(self, raw):
         """Hold the last value this entity actually reported until a new one

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:
+class TestNewCycleRunning:
-    """`_live_progress_code` is progress/progress_percentage's
+    """`_new_cycle_running` is progress/progress_percentage's
-    sticky_bypass_fn (issue #345) -- see sensor.py's _apply_sticky."""
+    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):
+    def test_true_for_a_concrete_non_finish_code_while_active(self):
-        assert _live_progress_code({"x.com.samsung.da.progress": "Wash"})
+        assert _new_cycle_running(
+            {"x.com.samsung.da.state": "Run", "x.com.samsung.da.progress": "Wash"}
+        )
 
-    def test_true_regardless_of_state(self):
+    def test_false_for_a_running_stage_reported_after_state_left_active(self):
-        """Not gated on machine_state -- a new cycle's own real progress
+        """Issue #358, the whole reason for the `state` gate: the reporting
-        must win over a held hold even while paused (e.g. adding a sock
+        dryer replays a running stage ('Drying', its first supportedProgress
-        mid-hold), not just while actively running."""
+        entry) for a few seconds after Finish while winding down. That is
-        assert _live_progress_code(
+        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 _new_cycle_running({})
-        assert not _live_progress_code({"x.com.samsung.da.progress": "None"})
+        assert not _new_cycle_running(
+            {"x.com.samsung.da.state": "Run", "x.com.samsung.da.progress": "None"}
+        )
 
 
 class TestProgressPercentage:

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():
+def test_a_running_stage_after_finish_does_not_break_the_hold():
-    """Not just an actively-running new cycle: adding a sock and pausing
+    """Issue #358: the reporting dryer resets `progress` to its course's
-    mid-cycle must also show the real, current progress rather than a
+    first stage in the same moment `state` goes idle -- observed twice,
-    stale hold from the previous cycle -- machine_state isn't 'active'
+    identically, as Cooling -> +60s Finish -> +24s 'Drying' -> +4s
-    while paused, so a bypass keyed on that alone would miss this."""
+    settled, with the reporter's machine_state history flipping to idle on
-    sensor, coordinator = _sensor(_PROGRESS_DESC)
+    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