Repository navigation
fix(cover): a timed cover with no restored state starts unknown instead of "open 50 %" - #795
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@gdluck i see you opened a huge number of pull requests. I am offcourse willing to take a look at all these requests. But please make sure that we have a physical trace for all these requests (and a seperate physical trace PR ) for each of these of these. Please let you ai agent study these old trace prs upfront. Also make sure we have our standard pr description (including state diagram) This makes it so much easier to audit and makes sure that we have high quality prs) This is the same for ownd and for myhome. All trace prs are in myhome. Maybe to get it managable put all current prs to draft mode first and then put them in phases back to non-draft. Many thanks! L |
|
They where in draft for 20 hours for tripple check. AI followed all the comments from all issues reported in past. I just started a re-recheck and re-test on my local homeassistant |
7843940 to
ec87970
Compare
|
Thanks for the guidance, and for merging #799. Here is the roadmap we are following for the whole batch, in phases as you suggested. Everything stayed in draft while it was reworked; each body now has the sections of the new template (problem, solution, state diagram, hardware verification, tests, "Where are the traces", "What does the Firmware Oracle / Encyclopedia say", checklist), the OWNd ones in the #135 layout with the same sections. Traces. The physical captures behind this batch are upstream now: #799 (F461 2.0.11, 2026-10-23, and MyHomeServer1 3.87.13, 2026-10-24) plus the earlier Phase 1 (un-drafted now, all checks green): #807 (README cross-references for #799), #796 (yaml cover Phase 2 (drafts, ready on request): OpenWebNet-HA/OWNd#137 (central-unit address guard), #143 (central_status text), #138 ( Phase 3 (drafts): #791 (setpoint in force), #794 (startup requests per firmware), #795 (cover starts unknown), #798 (4-digit area-0 addresses), #802 (unparsed-frame warning rate limit), #803 (alarm restore and Stay draft until the oracle PRs land: #792 and #793 (own-firmware-oracle OpenWebNet-HA/own-firmware-oracle#60), #804 (own-firmware-oracle OpenWebNet-HA/own-firmware-oracle#59). #797 keeps its dim-step / WHO 2 dimension frames marked "parser truth only" (no physical capture of those exists on our plants). Withdrawn: OpenWebNet-HA/OWNd#141 (OpenWebNet-HA/OWNd#123 keeps 4-digit input as typed; #798 canonicalises on the MyHOME side only). |
ec87970 to
aa1f2a9
Compare
aa1f2a9 to
e4f2420
Compare
GreenGrassBlueOcean
left a comment
There was a problem hiding this comment.
Solid bus-truthful fix — dropping the invented open / 50 % startup state when a timed WHO 2 cover has no restored state, letting set_cover_position 0 / 100 run to the end stop while refusing 1..99 with cover_position_unknown (0 frames sent), and preserving last_run_seconds on partial runs from unknown are all well-reasoned and backed by the physical F461 2.0.11 + F411/2 capture in #799.
Requesting changes for four unknown-state edge cases (reproduced against this branch on HA 2026.10.1 + OWNd==2.0.0b11), one Venetian tilt consistency question, and a PR-body Oracle / Encyclopedia correction before marking ready:
Code & State-Machine Issues (see inline comments)
_abort_motion()flips anunknowncover to"open"when bus delivery fails / times out (cover.py):_async_movesets_attr_is_closed = Falsebefore awaiting the write future, and_abort_motion()only restores_attr_is_closedwhen_attr_current_cover_position is not None— leavingstate == "open"(current_position: None) when zero frames reached the bus.MyHOMEScopeCover.is_closedreports"open"instead of"unknown"when all member timed covers startunknown(cover_scope.py:169-173):all(m.is_closed for m in members)evaluatesbool(None)asFalse, so area / general / group scope covers start as"open"while all their members are"unknown"(plus_fan_out_position_members()behind F422 interfaces raises mid-loop when members are mixed known/unknown)._promote_to_timed_strategy()leaks optimistic_attr_is_closedfromSimpleUpDownStopStrategy(cover.py:1188-1190): after a brief pulse on atravel_time: 0cover sets_attr_is_closed = True/False, promoting viamyhome.set_cover_travel_timeleavesstate == "closed"/"open"withcurrent_position: Nonewhile refusingset_cover_position(1..99)ascover_position_unknown.TimedCurtainStrategy.reanchor_on_stop_echoskips end-stop anchoring whenrun_start_pos is None(cover_strategies/timed.py:145) even whenelapsed >= self.travel_for_direction(is_opening), and logsre-anchored to None%.- Venetian blind slat tilt (
slat_tilt: true) still invents50 %whencurrent_cover_tilt_position is None(cover.pyasync_set_cover_tilt_position), makingset_cover_tilt_position: 50a silent 0-frame no-op on a fresh blind.
PR Description — Firmware Oracle & Encyclopedia (## What does the Firmware Oracle / Encyclopedia say & ## Where are the traces)
- Firmware Oracle (
WHERE = 31): The PR body currently states that no point-to-point stimulus exists in any published suite. Equivalent WHO 2 point-to-point stimuli atWHERE = 31(*#2*31*10##,*2*1*31##,*2*0*31##) are inresults/<Model>/<FW>/oracle/full/trace-corpus.tsvacross all 12 emulated firmwares and directly reinforce this change:*#2*31*10##:NACK+SILENT (Drop)onF450,F453,F453AV,H4684,MH200,MH200N(never placed on the SCS bus);ACK+OUTonF454,F459;NACK+OUTonF460,F461(1.0.29),MyHomeServer1(028206) with no Dimension-10 actuator answering;ACK+ synthetic*2*0*31##(0 bus frames) onMH202.*2*1*31##/*2*0*31##:OUT (Bus)on 11/12 gateways (all exceptF450), withF454,F459,F460,F461, andMyHomeServer1emitting the*2*1000#0*31##+*2*1000#1*31##pre-move stop/translation burst seen on the live F461.
- OpenWebNet Encyclopedia (
WHO = 2): Please include the Encyclopedia citation per.github/pull_request_template.md—functional/who-2-automation/dimensions.md(ownkb:chunk:r000453,r000454,r000456,r000473) specifies that standardWHO = 2WHAT = 0/1/2frames carry only movement state (STOP/UP/DOWN), and even advancedDIMENSION 10(shutterLevel) reserves255=Unknown positionfor uncalibrated shutters rather than fabricating50 %. - Minor trace table nit: Under
## Where are the traces,trace PR: #799 (open)→#799 (merged).
| # A positioned cover whose position is still unknown: so is its state. | ||
| self._attr_is_closed = None | ||
|
|
||
| def _abort_motion(self) -> None: |
There was a problem hiding this comment.
Bug (state corruption on failed/cancelled write from unknown):
_async_move eagerly sets self._attr_is_closed = False (~line 1528) before awaiting the gateway delivery future. If gateway.send fails, is cancelled, or hits WRITE_TIMEOUT, _abort_motion() clears _attr_is_opening / _attr_is_closing and then runs:
if self._attr_current_cover_position is not None:
self._start_position = self._attr_current_cover_position
self._attr_is_closed = (self._attr_current_cover_position == 0)When the cover started unknown (_attr_current_cover_position is None), that branch is skipped and self._attr_is_closed stays False. As a result, cover.state flips from "unknown" to "open" (current_position: None) even though zero frames reached the bus — and a subsequent set_cover_position(40) fails with "the position is unknown…" while HA shows the cover as "open".
_abort_motion() needs the matching unknown-position fallback (or snapshot/restore _attr_is_closed across _async_move so assumed_state covers roll back cleanly too):
if self._attr_current_cover_position is not None:
self._start_position = self._attr_current_cover_position
self._attr_is_closed = (self._attr_current_cover_position == 0)
elif not self._strategy.assumed_state and not self._advanced:
self._start_position = None
self._attr_is_closed = None| self._attr_is_opening = False | ||
| self._attr_is_closing = False | ||
| self._attr_is_closed = False | ||
| self._attr_is_closed = None |
There was a problem hiding this comment.
Starting timed point-to-point covers with _attr_is_closed = None and _start_position = None exposes two callers that still assume a non-None boolean / 50 fallback:
-
MyHOMEScopeCover.is_closed(custom_components/myhome/cover_scope.py:169-173):@property def is_closed(self) -> bool: if not (members := self._members()): return super().is_closed return all(m.is_closed for m in members)
When all timed members have
m.is_closed is None,all(m.is_closed for m in members)evaluatesbool(None)→False. On a fresh start with no history, every point-to-point timed cover is"unknown", while an area / general / groupMyHOMEScopeCoverwith those members falsely starts as"open"(is_closed = False,current_position = None).MyHOMEScopeCover.is_closedshould returnbool | None(matching HA Core'sCoverGroup:Nonewhen[m.is_closed for m in members if m.is_closed is not None]is empty,Falseif any known member isFalse,Trueif all known members areTrue— orNoneif any member isNoneand none isFalse).- Also check
MyHOMEScopeCover.async_set_cover_position(cover_scope.py:222-230): behind an F422 interface (_fan_out_position_members()), if member11is known (50 %) and member12isunknown,set_cover_position(40)dispatches a move to member11and then raisesServiceValidationError("cover_position_unknown")mid-loop on member12. Pre-validating before dispatching avoids partial fan-out.
-
_promote_to_timed_strategy()(custom_components/myhome/cover.py:1188-1190):
A cover configured withtravel_time: 0(SimpleUpDownStopStrategy,assumed_state = True) sets_attr_is_closed = True/Falseon any brief open/close pulse while_start_positionstaysNone. When promoted at runtime viamyhome.set_cover_travel_time,_promote_to_timed_strategy()sets_attr_assumed_state = Falseand_attr_current_cover_position = self._start_position(None) without clearingself._attr_is_closed = None, leaving the cover reporting"closed"/"open"while rejectingset_cover_position(1..99)ascover_position_unknown.
| travel_time = self.curtain_travel_for(is_opening) | ||
| reanchored: int | None = getattr(entity, "_attr_current_cover_position", None) | ||
| if travel_time > 0 and (is_opening or is_closing): | ||
| if travel_time > 0 and (is_opening or is_closing) and run_start_pos is not None: |
There was a problem hiding this comment.
Missed end-stop confirmation on relayed stop echo + "None%" debug log:
-
In
_freeze_position,end_stop(at >= run_started_at + travel) is checked beforestart_position is None, so a full-travel run fromunknownanchors100or0. Here inreanchor_on_stop_echo, ifstop_cover()was sent just beforetravel_time(e.g.5.85 son a6.0 scover, so the provisional freeze left positionNone) and the gateway's relayed*2*0*WHERE##arrives insideECHO_WINDOWatelapsed >= self.travel_for_direction(is_opening)(e.g.6.05 s),run_start_pos is Noneskips anchoring altogether — leavingcurrent_cover_positionasNoneeven thoughlast_run_secondsrecords6.05 >= 6.0.
Adding anelif travel_time > 0 and (is_opening or is_closing) and elapsed >= self.travel_for_direction(is_opening):branch to anchor100 if is_opening else 0(and callself.on_position_confirmed(entity, reanchored, "end_stop")) keeps_freeze_positionandreanchor_on_stop_echoconsistent. -
On partial runs from
unknown, the debug log at line 162 ("%s Stop status echo for %s; re-anchored to %s%% (elapsed %.2f s).") formats"re-anchored to None% (elapsed 2.05 s)."whenentity._attr_current_cover_positionisNone.
|
|
||
| curr_pos = self.current_cover_position if self.current_cover_position is not None else 50 | ||
| curr_pos = self.current_cover_position | ||
| if curr_pos is None: |
There was a problem hiding this comment.
Scope / consistency check — Venetian blind slat tilt (slat_tilt: true):
Here async_set_cover_position refuses 1..99 when curr_pos is None, whereas async_set_cover_tilt_position (~line 2021, and _follow_pulse ~line 1930) still falls back to an invented 50 %:
curr_tilt = self._interpolated_tilt(time.monotonic()) if was_tilting else self.current_cover_tilt_position
if curr_tilt is None:
curr_tilt = 0 if self.is_closed else 50On a fresh Venetian blind with no restored state (current_cover_position is None, current_cover_tilt_position is None, is_closed is None):
set_cover_tilt_position: 50computesdiff = 0(run_duration = 0) and silently drops the call with 0 bus frames, leavingcurrent_cover_tilt_positionasNone.set_cover_tilt_position: 20sends a0.6 sclosing pulse (30 %of2.0 sslat_time) timed from the invented50 %.
Should curr_tilt is None follow the same rule (0 / 100 run full slat_time against the mechanical slat stop to anchor tilt, 1..99 refused until anchored), or if slat tilt is intentionally out of scope for this PR, could we note that boundary in docs/configuration/covers.md?
|
|
||
| For standard covers without hardware position feedback, the integration provides a software estimator enabling full `open_cover`, `close_cover`, `stop_cover`, and slider-based `set_cover_position` support: | ||
|
|
||
| * **Unknown Until the First Full Run**: A timed actuator cannot report where the curtain is (an F411/2 answers `*#2*<WHERE>*10##` with an ACK and no data), so a cover with no restored state starts as `unknown`, with no position, instead of an invented `open` at 50 %. The restored position from the last run of Home Assistant is kept. The position becomes known at an end stop: `open_cover`, `close_cover`, `set_cover_position: 0` / `100` or a keypad run that lasts the full travel time; a keypad stop before that leaves it unknown. While it is unknown, `set_cover_position` with any other target is refused (*"… the position is unknown until the cover has run fully open or closed once"*) and sends nothing, because there is no position to time the run from. |
There was a problem hiding this comment.
Minor protocol wording precision: on the SCS bus an F411/2 does not emit OpenWebNet ACK frames — the gateway answers *#2*<WHERE>*10## with *#*1## and no data frames on F461 2.0.11 (or *#*0## on gateways such as MH200N / F453) because the F411/2 does not respond to Dimension 10. Suggest: (for an F411/2, the gateway answers *#2*<WHERE>*10## with no position frame).
| cover.handle_event(OWNEvent.parse("*2*0*05##")) # stopped before the end stop | ||
| assert cover.current_cover_position is not None | ||
| assert 50 < cover.current_cover_position < 100 | ||
| assert cover.is_closed is False |
There was a problem hiding this comment.
Please add regression tests here (or in test_issue_433_cover_scope.py for the scope case) covering:
- Command delivery failure / timeout from
unknown(written.set_exception(...)) keepscover.is_closed is Noneandcover.state is None("unknown"), not"open". MyHOMEScopeCoverwith freshunknowntimed members starts witharea.is_closed is Noneandarea.state is None("unknown"), not"open".travel_time=0cover pulsed closed (*2*2*05##→*2*0*05##) then promoted viaasync_set_travel_time(6.0, 6.0)resetsis_closedtoNonewhilecurrent_cover_positionisNone.stop_coverwritten just beforetravel_timewhose relayed*2*0*05##arrives atelapsed >= travel_timeanchors0/100.
`_async_move` sets `is_closed` to False before the frame is written, and `_abort_motion` only put it back when a position existed. A cover with no position (unknown, or an assumed-state cover) whose frame failed, was cancelled or timed out therefore flipped from `unknown` (or its restored `closed`) to `open` although nothing reached the bus. `_async_move` now keeps the state it found, and `_abort_motion` restores it when there is no position to derive the state from. Review 5639677952 on OpenWebNet-HA#795, finding 1. Tests: delivery failure and write timeout from unknown keep `is_closed None` and the state unknown with one frame queued and none more; a simple cover restored as `closed` goes back to `closed`. The three fail before the fix (`False is None`, `False is True`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`MyHOMEScopeCover.is_closed` took `all(m.is_closed for m in members)`, so an area / general / group cover whose timed members all start unknown reported `open`. It now returns `bool | None` as HA Core's CoverGroup does: None when no member has a known state, otherwise closed when every member with a known state is closed. `async_set_cover_position` behind an F422 fanned the target out member by member and raised `cover_position_unknown` mid-loop when one member was known and another unknown, after the first had been started. The refusal moved to `MyHOMECover._check_position_target`, which the scope runs on every member before the first frame; the point cover uses the same check. Review 5639677952 on OpenWebNet-HA#795, finding 2. Tests: fresh members give `area.is_closed None` and state unknown, one known member decides; a mixed fan-out at 40 % raises before any frame (`*2*2*11#4#02##` was sent before the fix) and 0 % reaches every member. Two fan-out tests now set a known 50 % on their members, as they test the pacing, not the refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…state A `travel_time: 0` cover (simple strategy, assumed state) sets `is_closed` from the last direction pulse. `_promote_to_timed_strategy` (`myhome.set_cover_travel_time`, calibration) carried that over while the position became None, so the timed cover reported `closed` / `open` with no position and refused `set_cover_position` 1..99 as unknown. When the promotion leaves no position, `is_closed` is now None too. Review 5639677952 on OpenWebNet-HA#795, finding 3. Test: a simple cover pulsed closed for 0.5 s, then `async_set_travel_time(6.0, 6.0)`: timed, position None, `is_closed None`, state unknown, 40 % refused with no frame. Fails before the fix (`True is None`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`TimedCurtainStrategy.reanchor_on_stop_echo` skipped anchoring when the run started at an unknown position, also when the relayed stop status arrived at or after the travel time: a stop written at 5.85 s of a 6.0 s travel whose status came at 6.05 s recorded `last_run_seconds` 6.05 and left the position None, while `_freeze_position` anchors the same run at its end stop. The echo now anchors 100 / 0 in that case and reports it to the strategy as `end_stop`. The debug line no longer prints `re-anchored to None%` for a partial run from unknown. Review 5639677952 on OpenWebNet-HA#795, finding 4. Tests: the stop-just-before, status-just-after case anchors 0 (`None == 0` before the fix); the partial case logs "position still unknown" and never "None%". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ation `async_set_cover_tilt_position` and a scope pulse's followers invented a slat angle (0 when closed, else 50) when `current_cover_tilt_position` was None, so on a fresh blind `set_cover_tilt_position: 50` was a silent 0-frame no-op and `20` sent a 0.6 s pulse timed from nowhere. The slat angle now follows the rule of the curtain position: 0 % and 100 % run a full `slat_time` against the slat stop and anchor the angle there (the strategy counts the rotation from the far end); any other target is refused with the translated `cover_tilt_position_unknown` error and sends nothing. A pulse from an unknown angle that is stopped before the slat stop leaves the angle unknown, and a follower whose angle is unknown is anchored by its leader's pulse only at a slat stop. A scope blind already took its members' mean angle; it is unknown while none of them knows theirs. Review 5639677952 on OpenWebNet-HA#795, finding 5. Tests: a fresh blind refuses 50 % and 20 % with no frame, 100 % sends `*2*1*05##` with a 2.0 s pulse and reads 100 at the stop while the curtain position stays unknown; a pulse to 0 % stopped at 1.0 s stays unknown, the next full pulse anchors 0; a scope blind with unknown members refuses 50 % and follows the members' mean once they know. Before the fix the refusals did not raise and the half pulse invented 0. One delivery-failure test sets a known angle, as it tests the failure path, not the fallback. Docs: covers.md slat tilt section; strings.json + translations/en.json (sync-en). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On the SCS bus an F411/2 emits no OpenWebNet ACK; the gateway answers `*#2*<WHERE>*10##` (`*#*1##` on an F461 2.0.11, `*#*0##` on an MH200N / F453) and relays no position frame because the actuator does not answer dimension 10. The covers.md sentence says so now, as suggested in review 5639677952 on OpenWebNet-HA#795. The release-notes delta covers the review's follow-ups (lost frame, scope state, slat angle). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the thorough pass; all five code findings, the four regression tests you listed and the description corrections are on the branch (head c53079a, one commit per finding, still draft). A second-opinion review of the diff added one defensive change (the end-stop inference from
Full suite 4517 passed at 100 % coverage (one unrelated xdist timing flake in event-session recovery passes alone), ruff / mypy-strict / HA standards / docs / translations clean. |
A WHO 2 timed cover with no restored state was created as `open` with `current_position 50`. The actuator cannot report a position (an F411/2 on a physical F461 2.0.11 answers `*#2*05*10##` with an ACK and no data), so the 50 was invented, and a `set_cover_position` before the first full run was timed from it. The position, `is_closed` and the run's start position now begin as None (HA renders `unknown`). A run from an unknown position stays unknown unless it reaches an end stop (a run of at least the travel time), which anchors 0 / 100 as before. With an unknown position, `set_cover_position` 0 / 100 run to the end stop; any other target is refused with the translated `cover_position_unknown` error and sends no frame. The restored position from the last HA run is unchanged. The stop echo of a run from an unknown position still records `last_run_seconds` but re-anchors no position. Verified on the F461 plant: fresh entity `unknown`, position 40 refused with no frame on the bus, `open_cover` -> `open 100` at the actuator's stop, then 40 -> 39 % and `close_cover` -> `closed 0`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`_async_move` sets `is_closed` to False before the frame is written, and `_abort_motion` only put it back when a position existed. A cover with no position (unknown, or an assumed-state cover) whose frame failed, was cancelled or timed out therefore flipped from `unknown` (or its restored `closed`) to `open` although nothing reached the bus. `_async_move` now keeps the state it found, and `_abort_motion` restores it when there is no position to derive the state from. Review 5639677952 on OpenWebNet-HA#795, finding 1. Tests: delivery failure and write timeout from unknown keep `is_closed None` and the state unknown with one frame queued and none more; a simple cover restored as `closed` goes back to `closed`. The three fail before the fix (`False is None`, `False is True`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`MyHOMEScopeCover.is_closed` took `all(m.is_closed for m in members)`, so an area / general / group cover whose timed members all start unknown reported `open`. It now returns `bool | None` as HA Core's CoverGroup does: None when no member has a known state, otherwise closed when every member with a known state is closed. `async_set_cover_position` behind an F422 fanned the target out member by member and raised `cover_position_unknown` mid-loop when one member was known and another unknown, after the first had been started. The refusal moved to `MyHOMECover._check_position_target`, which the scope runs on every member before the first frame; the point cover uses the same check. Review 5639677952 on OpenWebNet-HA#795, finding 2. Tests: fresh members give `area.is_closed None` and state unknown, one known member decides; a mixed fan-out at 40 % raises before any frame (`*2*2*11#4#02##` was sent before the fix) and 0 % reaches every member. Two fan-out tests now set a known 50 % on their members, as they test the pacing, not the refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…state A `travel_time: 0` cover (simple strategy, assumed state) sets `is_closed` from the last direction pulse. `_promote_to_timed_strategy` (`myhome.set_cover_travel_time`, calibration) carried that over while the position became None, so the timed cover reported `closed` / `open` with no position and refused `set_cover_position` 1..99 as unknown. When the promotion leaves no position, `is_closed` is now None too. Review 5639677952 on OpenWebNet-HA#795, finding 3. Test: a simple cover pulsed closed for 0.5 s, then `async_set_travel_time(6.0, 6.0)`: timed, position None, `is_closed None`, state unknown, 40 % refused with no frame. Fails before the fix (`True is None`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`TimedCurtainStrategy.reanchor_on_stop_echo` skipped anchoring when the run started at an unknown position, also when the relayed stop status arrived at or after the travel time: a stop written at 5.85 s of a 6.0 s travel whose status came at 6.05 s recorded `last_run_seconds` 6.05 and left the position None, while `_freeze_position` anchors the same run at its end stop. The echo now anchors 100 / 0 in that case and reports it to the strategy as `end_stop`. The debug line no longer prints `re-anchored to None%` for a partial run from unknown. Review 5639677952 on OpenWebNet-HA#795, finding 4. Tests: the stop-just-before, status-just-after case anchors 0 (`None == 0` before the fix); the partial case logs "position still unknown" and never "None%". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ation `async_set_cover_tilt_position` and a scope pulse's followers invented a slat angle (0 when closed, else 50) when `current_cover_tilt_position` was None, so on a fresh blind `set_cover_tilt_position: 50` was a silent 0-frame no-op and `20` sent a 0.6 s pulse timed from nowhere. The slat angle now follows the rule of the curtain position: 0 % and 100 % run a full `slat_time` against the slat stop and anchor the angle there (the strategy counts the rotation from the far end); any other target is refused with the translated `cover_tilt_position_unknown` error and sends nothing. A pulse from an unknown angle that is stopped before the slat stop leaves the angle unknown, and a follower whose angle is unknown is anchored by its leader's pulse only at a slat stop. A scope blind already took its members' mean angle; it is unknown while none of them knows theirs. Review 5639677952 on OpenWebNet-HA#795, finding 5. Tests: a fresh blind refuses 50 % and 20 % with no frame, 100 % sends `*2*1*05##` with a 2.0 s pulse and reads 100 at the stop while the curtain position stays unknown; a pulse to 0 % stopped at 1.0 s stays unknown, the next full pulse anchors 0; a scope blind with unknown members refuses 50 % and follows the members' mean once they know. Before the fix the refusals did not raise and the half pulse invented 0. One delivery-failure test sets a known angle, as it tests the failure path, not the fallback. Docs: covers.md slat tilt section; strings.json + translations/en.json (sync-en). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On the SCS bus an F411/2 emits no OpenWebNet ACK; the gateway answers `*#2*<WHERE>*10##` (`*#*1##` on an F461 2.0.11, `*#*0##` on an MH200N / F453) and relays no position frame because the actuator does not answer dimension 10. The covers.md sentence says so now, as suggested in review 5639677952 on OpenWebNet-HA#795. The release-notes delta covers the review's follow-ups (lost frame, scope state, slat angle). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The stop-echo end-stop inference for a run from an unknown position compared the booked elapsed time, which is raised to the scheduled auto-stop duration, with the travel time. The scheduled duration is not evidence that the motor ran that long; only the measured run is. The comparison now uses the raw elapsed time (second-opinion review point on the review 5639677952 follow-up). `last_run_seconds` is unchanged. Also covers the Venetian overshoot branch for a blind whose position is known but whose angle is not: a full tilt from unknown that runs 1 s past the rotation lifts a 50 % curtain to 75 %. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
c53079a to
85c4401
Compare
|
All checks green on 85c4401 (review points above addressed); marking ready for review. |
Interstellar0verdrive
left a comment
There was a problem hiding this comment.
Approving. Code review plus probes. On my fork a modelled cover already comes back from a restart without a position, and my wizard treats an unknown or incoherent position as unknown, so this matches what I see.
Starting from a known position nothing changes: my #702/#707/#508 probes give the same numbers. From unknown nothing invents a position, and a partial target is refused with nothing sent.
Two points, either here or in a follow-up:
- The docs say
open_cover/close_covermake the position known. They only do if the actuator's stop status comes at or aftertravel_time. A close that the actuator stops itself before that stays unknown, including on the default 25 s travel. Use the travel + 2 s settle from #702 for open/close from unknown, or qualify the docs. - Restore still turns a last state
openwith no position into 100. "Open" says nothing about height, so it should come back as open with an unknown position.
…tore "open" without a position as unknown Review of OpenWebNet-HA#795 (5642333408), both points: 1. open_cover / close_cover from an unknown position run to the end stop through _async_run_to_end, settling on travel_time + END_SETTLE_MARGIN (OpenWebNet-HA#702) like set_cover_position 0 / 100. An actuator stop status before the travel time (an F411/2 timed shorter than travel_time, 5 s on the F461 plant against the default 25 s) no longer ends that run: nothing can be frozen from it, so the settle timer anchors 100 / 0; the motor time is still recorded as last_run_seconds. A stop at or after the travel time anchors the end stop at once, as before. A cancelled settle (reversal, stop_cover, a new command) clears the flag so the next stop status ends the run normally. Scope covers keep their own run timeout; advanced and assumed-state covers are untouched. 2. A restored last state "open" with no position stays open with an unknown position instead of 100 %: "open" says nothing about the height. A restored "closed" is still 0 %. Docs and release notes say so; tests: early actuator stop settles, open without any stop status settles, known start still freezes the estimate, reversal during the settle, restored open / closed without a position. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks. Both points are in here rather than a follow-up (ea7067f, one commit):
Docs and release notes updated; the body's "not in this PR" paragraph is rewritten accordingly. Full suite 4534 passed / 1 skipped at 100 % coverage, ruff, typing ratchet, HA standards and docs sync clean. An independent second-opinion review of the diff raised the stale-flag-after-reversal case and the direction-specific travel time check; both are in. |
GreenGrassBlueOcean
left a comment
There was a problem hiding this comment.
Code quality & review historyThe implementation is careful and well-tested. Key paths (init, restore, _async_move / _async_run_to_end / _freeze_position / _abort_motion, async_set_cover_position, scope, tilt, strategies) correctly handle None start positions and the settle-from-unknown flag. Edge cases (early actuator stop, keypad reversal during settle, delivery failure, mixed known/unknown fan-out, promotion) are covered.Reviews:
GreenGrassBlueOcean requested changes (5 code findings + docs/oracle nits). All addressed in dedicated commits with failing-before / passing-after tests.
Interstellar0verdrive approved, then dismissed after the final follow-up (settle timer for open/close from unknown + restore of “open” without position). Author incorporated both points.
Review threads remain unresolved in the UI (normal until the reviewer re-approves or threads are resolved). The CHANGES_REQUESTED review is the likely reason mergeable_state is blocked; a re-review or dismissal of the old review would unblock.Strengths
Aligns with HA CoverEntity semantics (is_closed is None + not moving → unknown).
No invented bus state; only end-stop or full travel anchors position.
Excellent test coverage (new test_cover_unknown_start.py + scope tests; full suite green at 100% coverage).
Strong documentation, release-notes delta, traces, and protocol grounding.
Follow-ups folded in rather than deferred.
Minor notes / residual risk
A keypad stop inside the travel-time window on a from-unknown run is indistinguishable from the actuator’s own stop and will settle at the end position (documented).
Scope covers keep their existing run timeout; advanced and assumed-state covers are intentionally untouched.
One historical xdist timing flake in an unrelated test was noted and passes in isolation.
bcb78a9
into
OpenWebNet-HA:v2-phase1-architecture
Requested by A
What is the problem
A WHO 2 timed cover with no restored state is created as
openwithcurrent_position 50:__init__sets_attr_current_cover_position = 50,_attr_is_closed = Falseand_start_position = 50. A timed actuator cannot report where the curtain is (an F411/2 answers*#2*05*10##with an ACK and no data), so the 50 is invented, and aset_cover_positionbefore the first full run is timed from it. The bus reports no such state, so the entity should not either (the "bus-truthful state" line of #760 / #739).No issue number; found on the F461 plant while verifying the cover engine. Refs #760, #739, #702.
#778 (merged, fe60f57) also touched
cover.py; the hunks do not overlap (this PR changes__init__,_freeze_position,_async_move, the unknown branch ofasync_set_cover_positionand thehandle_eventstart positions). Rebased onto the currentv2-phase1-architecture(d8bac15).What is the solution
cover.py__init__: position,is_closedand the run's start position begin asNone(HA rendersunknown, nocurrent_positionattribute). Restore from the last HA state still wins: a restored position orclosedis kept; a restoredopenwithout a position stays open with an unknown position (review 5642333408), as "open" says nothing about the height._async_moveand the bus-started open/close branches ofhandle_event: the run's start position is the current one,Noneincluded (it used to be an invented 0 when opening and 100 when closing)._freeze_position: a run from an unknown position that did not reach an end stop (a run shorter than the travel time) staysNone; an end stop anchors 100 / 0 as before.is_closedisNonewhile the position is unknown. A run from a known position is timed exactly as before.async_set_cover_positionwith an unknown position: 0 and 100 run to the end stop (_async_run_to_end, which anchors the position); any other target raisesServiceValidationError(cover_position_unknown, added tostrings.json/translations/en.json) and sends no frame. Refusing was chosen over "run to an end, then to the target" because the second run would be chained through the settle task, which the actuator's own stop status cancels, so the target would be lost silently.cover_strategies/timed.py: the stop echo of a run from an unknown position still recordslast_run_secondsbut re-anchors no position;current_positionreturnsNonewhile such a run is moving.base.pydocuments thatctx["run_start_position"]may beNone.tests/test_component_cover.py::test_handle_eventnow sets a known 50 explicitly, as it depended on the invented start.covers.md, "Unknown Until the First Full Run") and the release-notes delta.Follow-up folded in (review 5642333408, ea7067f): on the plant the F411/2 stops itself after 5.1-5.9 s while the yaml says
travel_time: 6, and the default travel is 25 s, so aclose_coverfrom unknown that waited for a stop status at or after the travel time stayed unknown.open_cover/close_coverfrom an unknown position now run through_async_run_to_endand settle on travel time + 2 s likeset_cover_position: 0/100(#702); an actuator stop status before the travel time does not end that run (its motor time is still recorded aslast_run_seconds), one at or after it anchors the end stop at once. A cancelled settle (reversal,stop_cover, a new command) drops the rule so the next stop ends the run normally. A keypad stop inside the window cannot be told from the actuator's own stop and settles at the end position too (documented). Scope covers keep their own run timeout; advanced and assumed-state covers are untouched.State Diagram
stateDiagram-v2 direction TB state "1. Before: timed cover, no restored state" as Before { [*] --> Open50 : __init__ invents\nposition 50, is_closed False Open50 --> TimedFrom50 : set_cover_position 40\n*2*2*05## timed from 50 } state "2. After: None until an end stop" as After { [*] --> Restored : last HA state , open 30\nunchanged [*] --> Unknown : no restored state\nposition None, is_closed None Unknown --> Unknown : set_cover_position 1..99\nServiceValidationError cover_position_unknown\nno frame sent [*] --> Unknown : simple cover promoted to timed\nno position, is_closed None Unknown --> Running : open_cover / close_cover / set 0 or 100\n*2*1*05## or *2*2*05##\nor a keypad run *2*1*05## Running --> Unknown : direction frame never written\nfailed, cancelled or timed out\nstate as before the command Running --> Unknown : *2*0*05## before travel_time\n, HA stop or keypad stop\nlast_run_seconds recorded Running --> Known : *2*0*05## at or after travel_time\nend stop, position 100 / 0\nalso the relayed status of an HA stop Known --> Known : timed runs from a known position\nas before , regression test }Review follow-up (review 5639677952)
One commit per finding, each with tests that fail before it and pass after it; the full-diff second-opinion review added the last one.
_abort_motion()flipped an unknown cover toopenwhen the frame never reached the bus → 82c64d1:_async_movekeeps theis_closedit found and_abort_motionrestores it when there is no position (an assumed-state cover restored asclosedgoes back toclosedtoo). Tests: delivery failure and write timeout from unknown keepis_closed None, state unknown, one frame queued and none more (3 tests,False is None/False is Truebefore).MyHOMEScopeCover.is_closedread unknown members as open; the F422 fan-out raised mid-loop → 6b1858f:is_closedisbool | Noneas HA Core'sCoverGroup(None while no member has a known state, else closed when every known member is), and the point cover's refusal moved to_check_position_target, which the scope runs on every member before the first frame. Tests: fresh members givearea.state None, one known member decides; member 11 known at 50 and 12 unknown,set_cover_position: 40raises before any frame (*2*2*11#4#02##was on the bus before) and0reaches both (2 tests). Two fan-out pacing tests now set a known 50 on their members._promote_to_timed_strategy()kept the simple strategy's optimisticis_closed→ 3d1f90a: when the promotion leaves no position,is_closedis None. Test: atravel_time: 0cover pulsed closed for 0.5 s, thenasync_set_travel_time(6.0, 6.0): timed, position None,is_closed None, 40 refused with no frame (True is Nonebefore).reanchor_on_stop_echoskipped the end stop whenrun_start_pos is None, and loggedNone%→ c8f527a: a run from unknown whose relayed stop status arrives at or after the travel time anchors 100 / 0 and is reported to the strategy asend_stop, as_freeze_positiondoes; the debug line saysposition still unknowninstead. Tests: stop written at 5.85 s of a 6.0 s travel, status at 6.05 s → 0 andlast_run_seconds6.05 (None == 0before); the partial case never logsNone%(2 tests). c53079a (second-opinion point, accepted): the inference compares the measured run with the travel time, not the booked one that is raised to the scheduled auto-stop duration; a strategy-level test stops a 6 s booked run at 2.05 s and gets no end stop.set_cover_tilt_position: 0/100(andopen_cover_tilt/close_cover_tilt) run a fullslat_timeagainst the slat stop and anchor the angle there (the strategy counts the rotation from the far end); 1..99 are refused with the translatedcover_tilt_position_unknown(strings.json, translations/en.json viamanage_translations.py sync-en) and send nothing; a pulse from unknown stopped before the stop leaves the angle unknown; a scope pulse's followers with an unknown angle are anchored only at a slat stop; the scope blind already took its members' mean angle and is unknown while none knows theirs. Tests: a fresh blind refuses 50 / 20 with no frame, 100 sends*2*1*05##with a 2.0 s pulse and reads 100 at the stop with the curtain still unknown; a pulse to 0 stopped at 1.0 s stays unknown and the next full pulse anchors 0; a scope blind with unknown members refuses 50 and follows their mean; a blind with a known 50 % curtain and no angle whose full tilt runs 1 s long lifts to 75 % (4 tests,DID NOT RAISE/0 is Nonebefore). Docs: covers.md slat tilt section.*#2*<WHERE>*10##with no position frame"; the release-notes delta covers the follow-ups.Review follow-up (review 5642333408)
open_cover/close_coverfrom unknown only became known when the actuator's stop came at or aftertravel_time→ ea7067f: they settle on the travel time + 2 s (feat(cover): motor start-up floor and full runs at 0/100% (#466) #702) as described above; the early stop status is kept as the measured run and no longer ends it. Tests: the F411/2 stopping at 5.0 s withtravel_time: 6leaves the cover closing with no position until the timer, then 0 % with only the direction frame sent;open_coverwith a direction echo and no stop status at all reads 100 % after the timer; a known start stopped halfway still freezes 50 %; a keypad reversal during the settle followed by a stop ends the reversed run (no stuckclosing). Docs and release notes updated.openwithout a position into 100 → ea7067f: it stays open with an unknown position,set_cover_position: 50is refused and sends nothing;closedwithout a position is still 0 %. The twotest_component_cover.pyrestore cases now assert that.Full suite 4534 passed / 1 skipped at 100 % coverage; ruff, typing ratchet, HA standards and docs sync clean.
Hardware Verification
Live F461 2.0.11 plant (local Home Assistant, combined build of all our PRs, 2026-10-24) and, for status reads, the MyHomeServer1 3.87.13 plant; captures in MyHOME #799 (merged).
Live retest (2026-10-24)
Consensus label: startup claim confirmed; no regression (combined build of all open PRs, F461 2.0.11, HA 2026.10.2, cover restore entry removed before the first start).
unknown,current_position None(base:open 50).set_cover_position: 40→ HTTP 400 "… the position is unknown until the cover has run fully open or closed once", nothing on the bus.tx *2*1*05##or*2*2*05##, the F461's*2*1000#0*05##,*2*0*05##,*2*1000#1*05##/#2,*2*1*05##/*2*2*05##, then the actuator's ownrx *2*0*05##5.42-5.67 s later.travel_time: 6the entity returned tounknownafter every run: expected, the motor stops 0.3-0.6 s before the configured travel time and this PR has no end-stop window by design (the fix(cover): confirm end_stop on full-travel bus runs and scope run timeouts #760 / feat(cover): motor start-up floor and full runs at 0/100% (#466) #702 follow-up above). Aftermyhome.calibrate_cover(5.81 / 5.80 s measured) the next run endedopen 100; a laterclose_coverran 5.13 s and ended atopen 12, the known "12 %" case of a close from 100 on this plant. A regression claim would need a base-vs-PR A/B at identical calibration; none was made.How is it tested
tests/test_cover_unknown_start.py(17, of which 7 from the first push): no history →unknown; close from unknown with the actuator's stop after a full travel → 0 / closed; HAstop_coverduring a run from unknown → still unknown,last_run_seconds2.05 recorded from the relayed stop status; keypad partial run → still unknown, then a full keypad run → 100;set_cover_position40 refused withcover_position_unknownand no frame, 0 →*2*2*05##, 100 →*2*1*05##; restored 30 % kept; regression guard: a known start (50) with a keypad run stopped after 2 s keeps a known position between 50 and 100 andis_closed is False(the unknown handling must not leak into the known case).v2-phase1-architecture(ddc6d60) for the first push: 5 fail, 1 error (the refused-target test starts a real move on the invented 50), 2 pass (restore, and the regression guard, which passes before and after by design). The review follow-up adds 10 tests here and 3 intests/test_issue_433_cover_scope.py(scope state, mixed fan-out, scope tilt); each fails on the commit before its fix as listed under "Review follow-up".mastercheckout (f04d8aa) onPYTHONPATH, HA 2026.10.2, Python 3.14,-n 4: 4517 passed, 1 skipped, 1 failed on the final tree; the failure istests/test_event_session_recovery.py::test_failed_reconnect_cycles_count_as_progress, which passes 3 of 3 runs alone and whose code this PR does not touch (an-n 4timing flake); the run before the last commit was 4516 passed, 1 skipped (first push: 4425 passed, 1 skipped; base ddc6d60: 4418 passed, 1 skipped).scripts/verify_ownd_coverage.py: 100 % statement coverage (16,117 / 16,117 statements).ruff check .,scripts/typing_ratchet.py(mypy --strict, 0 errors),scripts/verify_ha_standards.py,scripts/sync_documentation.py --check,scripts/manage_translations.py check(en.json in sync),tests/test_fixture_privacy.pypass (no fixture is added, soscripts/anonymize_plant_fixture.pydoes not apply).Where are the traces
tests/fixtures/traces/f461_live_2026-10-23/myhome_trace_F461_live_2026-10-23.json, runsrun6_event_session_ha_fixed_integrationandrun7_localha_bus_monitor;tests/test_f461_live_2026_10_23_trace.py::test_f411_2_shutter_05_stops_itself_after_each_runWHERE 05, yamltravel_time: 6*2*1000#0*05##,*2*0*05##, `21000#<1run1_oracle_replay_readonly;::test_cover_05_dimension_reads*#2*05*10##ACK with no data (no position available),*#2*05*11##NACK,*#2*05##and*#2*0##answer*2*0*05##only: nothing on the bus says where the curtain is, which is why no position is invented.The combined-build re-test of 2026-10-24 (same plant, HA 2026.10.2) is summarised under Testing; its raw recordings are not a fixture.
What does the Firmware Oracle / Encyclopedia say
The change sends no new frame: the only difference on the bus is that a refused
set_cover_position/set_cover_tilt_positionsends nothing. The published suites carry the same WHO 2 point-to-point stimuli atWHERE = 31(results/<P>/<V>/oracle/full/trace-corpus.tsvat OpenWebNet-HA/own-firmware-oracle 53f9514; the 12 emulated firmwaresF450/020010,F453/010100,F453AV/030014,F454/020051,F459/020105,F460/020012,F461/020011,H4684/020054,MH200/020102,MH200N/010108,MH202/010024,MyHomeServer1/028206), and they say what this PR assumes: a gateway relays movement, never a position.trace-corpus.tsv)*#2*31*10##(dimension 10 read)*2*0*31##and no bus frame on MH202. No firmware produces a position frame.*2*1*31##/*2*0*31##(up / stop)*2*1000#0*31##+*2*1000#1*31##pre-move stop / translation burst that the live F461 2.0.11 capture in #799 shows before every run (MH202 emits one of the two per command).OpenWebNet Encyclopedia,
functional/who-2-automation/dimensions.md(ownkb:chunk:r000453,r000454,r000456,r000473): the standardWHO = 2WHAT = 0 / 1 / 2frames carry the movement state only (stop / up / down), and even the advancedDIMENSION 10(shutter level) reserves255= unknown position for an uncalibrated shutter rather than a made-up value. An integration that has onlyWHATframes therefore has no position to report until an end stop.Checklist
What is the problem,What is the solution,State Diagram,How is it tested,Where are the traces,What does the Firmware Oracle / Encyclopedia say).MyHOMECoverkeeps itshandle_event()path; noasync_dispatcher_connectadded.pytest,ruff, andscripts/verify_ha_standards.pypass.🤖 Generated with Claude Code