laundry: a post-Finish running stage is the cycle ending, not a new one
Fixes #358, a regression from #346. That PR's sticky_bypass_fn released the Finish/100 hold on any concrete non-Finish progress code, ungated on machine_state, reasoning that a new cycle's own progress can appear before state catches up. But the reporting DA_WM_TP1_21_COMMON dryer replays a running stage on the way *out* of a cycle: the issue's history shows Cooling -> +60s Finish -> +24s 'Drying' -> +4s settled, twice, identically. The bypass read that tail as a new cycle, dropped the hold, and republished 'Drying' -- so progress read Drying, Cooling, Finish, Drying, Idle instead of ending at Finish, Idle. The tail is not new: rep_fn has always masked progress while state isn't active, which is why it was invisible before #346. What surfaced it was sticky_live_fn, a second, ungated view of the same field that the bypass returned in rep_fn's place -- letting the hold publish a value the entity otherwise never shows. Both halves are fixed: - The bypass (now _new_cycle_running) requires state == 'active' alongside the progress code. The arm condition stays ungated -- failing to arm loses the Finish entirely (#345), while releasing late costs nothing, since the hold expires on its own. - sticky_live_fn is gone. rep_fn is the only definition of a live value; the hold decides only whether to freeze, and the bypass returns rep_fn's own result. Also stop an already-open window from being restarted by a progress that flaps in and out of Finish, so sticky_seconds is measured from the first Finish of a cycle and the documented bound actually holds. A paused new cycle no longer cuts the hold short (it did under the old ungated bypass). Nothing live is withheld by that: rep_fn shows Idle while paused with or without a hold, so the only change is a stale Finish expiring on schedule -- and 'paused' cannot be told apart from this tail.
This commit is contained in:
Marc Billow
@@ -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'.
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user