Skip to content

Commit 3ea6d12

Browse files
fix(firmware): restore Ropener stall handling lost in the valar-core rewire
v2.7.0 rewired onto valar-core as a remote package. The entity list came through clean, so it shipped -- but the product's on_stall handler had been silently replaced by core's default, which zeroes the stepper position on EVERY stall. Two live bugs resulted: * The device wedged in HOMING. start_homing sets global_state=3 and nothing cleared it, so after any home the State sensor read HOMING forever, the cover stayed CLOSING, every button gesture died (all guard on state==0) and the schedule refused to run (guards on state!=3). Only the manual Start-Stop Homing button recovered it. * Any stall corrupted the position reference. A curtain snagging mid-travel silently redefined that point as home, poisoning the cover percentage and the persisted position. !extend APPENDS to core's action list rather than replacing it, so extending alone would have left core's unconditional zeroing firing alongside the fix. Remove core's handler first, then install the product's -- verified against the resolved config: exactly one on_stall survives, byte-identical to v2.6.4 including the non-homing else branch. Also in this change: * Restore the "Reversed ↺" Motor Direction option. Core had dropped the glyph; the string is what Home Assistant automations select by, and existing units hold it as their persisted state. * fw_version defaults to "dev" instead of a hardcoded "2.7.0", so local and branch builds stop misreporting themselves. Release builds still get the real version injected by the workflow via -s fw_version. * Add tools/regression-gate -- an entity diff AND a behavioural diff of the resolved config (on_stall, on_press, script bodies, on_boot, intervals, *_action, lambdas). The entity gate alone passed v2.7.0 clean; the behavioural gate is what catches this class of regression, and it is what caught the Motor Direction glyph and a missing else branch in the first draft of this very fix. Verified on both boards: esphome config clean, entity gate shows only the 7 intended valar-core diagnostics, behavioural gate shows only the documented scheduling/on_boot refactors and nothing dropped. VAL3100 compiles to a full factory.bin. OTA asset names and the GitHub-OTA button URL are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 7616db7 commit 3ea6d12

5 files changed

Lines changed: 355 additions & 2 deletions

File tree

‎firmware/VAL3000/Ropener-VAL3000.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ substitutions:
1212
log_uart: "USB_SERIAL_JTAG"
1313
ota_repo: "Valar-Systems/Ropener"
1414
ota_asset: "Ropener-VAL3000"
15-
fw_version: "2.7.0"
15+
# "dev" for local/branch builds; the release workflow overrides it with the
16+
# git tag (-s fw_version) so shipped units report the version they run.
17+
fw_version: "dev"
1618
pin_uart_tx: "6"
1719
pin_uart_rx: "5"
1820
pin_enn: "8"

‎firmware/VAL3100/Ropener-VAL3100.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ substitutions:
1313
log_uart: "USB_SERIAL_JTAG"
1414
ota_repo: "Valar-Systems/Ropener" # field units self-update from Ropener's own releases
1515
ota_asset: "Ropener-VAL3100"
16-
fw_version: "2.7.0"
16+
# "dev" for local/branch builds; the release workflow overrides it with the
17+
# git tag (-s fw_version) so shipped units report the version they run.
18+
fw_version: "dev"
1719
pin_uart_tx: "0"
1820
pin_uart_rx: "1"
1921
pin_enn: "5"

‎firmware/common/ropener-product.yaml‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,40 @@ globals:
5858
- { id: b1_tap_count, type: int, restore_value: no, initial_value: '0' }
5959
- { id: b1_last_tap_ms, type: uint32_t, restore_value: no, initial_value: '0' }
6060

61+
# --- Stall handling (homing) ------------------------------------------------
62+
# The Ropener homes sensorlessly: drive at the closed end until StallGuard fires,
63+
# and treat THAT stall as position 0. Core's default on_stall zeroes the position
64+
# unconditionally, which is wrong here — a stall mid-travel (curtain snags) would
65+
# silently redefine that spot as home and corrupt the cover's position reference.
66+
#
67+
# `!extend` APPENDS to core's action list rather than replacing it, so extending
68+
# alone would leave core's unconditional zeroing firing before ours. The two
69+
# entries below are deliberate and order-dependent: the first deletes core's
70+
# handler outright, the second installs the Ropener's conditional one. Verified
71+
# against the resolved config — exactly one on_stall survives. Keep both.
72+
stepper:
73+
- id: !extend driver
74+
on_stall: !remove
75+
- id: !extend driver
76+
on_stall:
77+
- stepper.stop: driver
78+
- if:
79+
condition:
80+
lambda: 'return id(global_state) == 3;'
81+
then:
82+
- stepper.report_position: { id: driver, position: 0 }
83+
- stepper.set_target: { id: driver, target: 0 }
84+
- globals.set: { id: global_state, value: "0" }
85+
- cover.template.publish: { id: ropener_cover, current_operation: IDLE }
86+
- logger.log: "Homed cover"
87+
# Stall outside homing (curtain snagged, or it hit the far end): stop
88+
# and settle the UI back to idle, but LEAVE the position alone — this
89+
# point is not home, and zeroing here is what corrupted v2.7.0.
90+
else:
91+
- globals.set: { id: global_state, value: "0" }
92+
- cover.template.publish: { id: ropener_cover, current_operation: IDLE }
93+
- logger.log: "Stall detected outside homing — motor stopped, position unchanged"
94+
6195
script:
6296
# Recompute the derived step cache from the persisted centimeters (one place).
6397
- id: recompute_distance
@@ -298,6 +332,27 @@ binary_sensor:
298332
- delay: 1s
299333
- lambda: "App.safe_reboot();"
300334

335+
# --- Preserve the shipped "Motor Direction" option strings --------------------
336+
# Field units (and any Home Assistant automation that selects a direction) know
337+
# this option as "Reversed ↺" — the glyph is part of the string, not decoration.
338+
# valar-core dropped it to a plain "Reversed", which would silently break those
339+
# automations and leave existing units holding an option that no longer exists.
340+
# `options` is a list, so !extend would APPEND and yield three choices; remove
341+
# then re-add instead. `lambda` is a scalar, so extending overwrites it cleanly.
342+
# Core's set_action matches on `x.find("Reversed")`, which still hits — no
343+
# override needed there.
344+
select:
345+
- id: !extend sel_direction
346+
options: !remove
347+
- id: !extend sel_direction
348+
options:
349+
- "Normal"
350+
- "Reversed ↺"
351+
lambda: |-
352+
if (id(global_stepper_direction) == esphome::tmc2209::ShaftDirection::COUNTERCLOCKWISE)
353+
return std::string("Reversed ↺");
354+
return std::string("Normal");
355+
301356
# --- Rename core tuning entities to Ropener's existing entity_ids -------------
302357
number:
303358
- id: !extend num_irun

‎tools/regression-gate/README.md‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
# Firmware regression gate
2+
3+
Two diffs between the **old** and **new** resolved firmware config. Run both
4+
before every release. A clean `esphome compile` is necessary but **not
5+
sufficient** — it proves the YAML is valid, not that the firmware still behaves
6+
the same.
7+
8+
## Why the behaviour gate exists
9+
10+
v2.7.0 rewired the Ropener onto `valar-core` as a remote package. The entity
11+
gate passed perfectly: no entity dropped, renamed, or retyped. It shipped.
12+
13+
But the product's `on_stall` handler had been silently replaced by
14+
`valar-core`'s default. Two live bugs resulted:
15+
16+
- the device wedged in `HOMING` forever after any home (killing the button
17+
gestures and the schedule, both of which guard on `global_state`), and
18+
- **every** stall zeroed the position, so a curtain snagging mid-travel
19+
silently redefined that point as home.
20+
21+
No entity name changed, so nothing caught it. The behaviour gate diffs the
22+
*automation bodies* — `on_stall`, `on_press`/`on_click`/`on_release`, script
23+
bodies, `on_boot`, intervals, `*_action`, lambdas — anchored to stable
24+
identities rather than list positions.
25+
26+
It also caught an unrelated change in the same release: the `Motor Direction`
27+
option string `"Reversed ↺"` had lost its glyph, which would have broken any
28+
Home Assistant automation selecting it by value.
29+
30+
## Usage
31+
32+
```sh
33+
# 1. Resolve both configs (from a checkout of each version)
34+
esphome config firmware/VAL3100/Ropener-VAL3100.yml > /tmp/old-3100.yaml
35+
esphome config firmware/VAL3100/Ropener-VAL3100.yml > /tmp/new-3100.yaml
36+
37+
# 2. Diff them
38+
python tools/regression-gate/gate.py /tmp/old-3100.yaml /tmp/new-3100.yaml
39+
40+
# 3. Inspect anything it flags
41+
python tools/regression-gate/gate.py /tmp/old-3100.yaml /tmp/new-3100.yaml \
42+
--detail on_stall
43+
```
44+
45+
Exit code `0` = both gates clean, `1` = something differs. **A difference is not
46+
automatically a failure** — additions and refactors are often intended. The
47+
gate's job is to guarantee nothing changes *unnoticed*. Every flagged line must
48+
be explained in the release notes.
49+
50+
Run it for **each board** (VAL3000 and VAL3100); they share a product layer but
51+
resolve differently.
52+
53+
Requires `pyyaml` — use the interpreter ESPHome is installed under.
54+
55+
## Known-benign differences after the valar-core rewire
56+
57+
Expected when diffing a pre-rewire release (≤ v2.6.4) against a rewired one:
58+
59+
| Anchor | Why |
60+
|---|---|
61+
| 7 diagnostic entities added | `valar-core` shared diagnostics (Restart, Uptime, WiFi Signal, ESP Internal Temperature, IP/SSID/MAC) |
62+
| `esphome:on_boot` split 600 / 400 / -100 | core / product / scheduling layers; relative order preserved |
63+
| `script:schedule_open`, `script:schedule_close` | scheduling-mixin contract; the `global_state != 3` homing guard moved into these scripts |
64+
| `datetime:Open Time`/`Close Time` `on_time`, `interval:60s` | now call the schedule scripts instead of inlining `cover.open`/`cover.close` |
65+
66+
Anything **outside** this table needs justification before release.
67+
68+
## Before Glasscalibur
69+
70+
This tool should move to `valar-motion` so both products share one copy rather
71+
than duplicating it — same rule as the firmware itself. Glasscalibur's overrides
72+
(limit switches, TMP1075 thermal cutoff, LIS2DH12 tamper, buzzer) carry the same
73+
silent-replacement risk as `on_stall` did here, and there a dropped thermal
74+
cutoff or tamper handler is a safety regression, not a UX one.

‎tools/regression-gate/gate.py‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
#!/usr/bin/env python3
2+
"""Firmware regression gate: diff two `esphome config` dumps.
3+
4+
Compiling proves the YAML is valid. It does not prove the firmware still DOES
5+
what it did. This runs two independent diffs:
6+
7+
ENTITY GATE -- every user-facing entity (domain, name, category).
8+
Catches dropped/renamed/added entities, which break
9+
Home Assistant entity_ids.
10+
11+
BEHAVIOUR GATE -- the automation bodies attached to those entities
12+
(on_stall, on_press, script bodies, on_boot, intervals,
13+
*_action, lambdas). Catches an entity that survived by
14+
name while its behaviour was silently replaced.
15+
16+
The behaviour gate exists because of the v2.7.0 homing regression: the entity
17+
gate passed clean while `on_stall` had been swapped for valar-core's default,
18+
which zeroed the cover's position on every stall. Entity names alone cannot see
19+
that class of bug.
20+
21+
Usage:
22+
python gate.py OLD.yaml NEW.yaml # human-readable report
23+
python gate.py OLD.yaml NEW.yaml --detail ANCHOR_SUBSTRING
24+
25+
Exit code is 0 if both gates are clean, 1 if either reports a difference.
26+
Differences are not automatically failures -- intended changes must be reviewed
27+
and called out. The gate's job is to make sure nothing changes UNNOTICED.
28+
"""
29+
import argparse, json, sys, yaml
30+
31+
# --- shared loader -----------------------------------------------------------
32+
33+
DOMAINS = [
34+
"binary_sensor", "sensor", "text_sensor", "switch", "number", "select",
35+
"button", "cover", "light", "datetime", "text", "fan", "lock", "climate",
36+
"valve", "event", "update", "stepper", "output", "alarm_control_panel",
37+
"media_player",
38+
]
39+
40+
41+
class Loader(yaml.SafeLoader):
42+
"""SafeLoader that tolerates ESPHome's custom tags (!lambda, !secret, ...)."""
43+
44+
45+
def _passthrough(loader, tag_suffix, node):
46+
if isinstance(node, yaml.ScalarNode):
47+
return loader.construct_scalar(node)
48+
if isinstance(node, yaml.SequenceNode):
49+
return loader.construct_sequence(node)
50+
return loader.construct_mapping(node)
51+
52+
53+
Loader.add_multi_constructor("!", _passthrough)
54+
55+
56+
def load(path):
57+
with open(path, "r", encoding="utf-8") as fh:
58+
return yaml.load(fh, Loader=Loader)
59+
60+
61+
# --- entity gate -------------------------------------------------------------
62+
63+
def entities(cfg):
64+
rows = set()
65+
for domain in DOMAINS:
66+
block = cfg.get(domain)
67+
if not isinstance(block, list):
68+
continue
69+
for item in block:
70+
if not isinstance(item, dict):
71+
continue
72+
if item.get("name") is not None:
73+
rows.add(f"{domain}\t{item['name']}\t{item.get('entity_category') or ''}")
74+
# Multi-entity platforms (wifi_info, ...) nest one sub-entity per key.
75+
for sub in item.values():
76+
if isinstance(sub, dict) and "name" in sub:
77+
rows.add(f"{domain}\t{sub['name']}\t{sub.get('entity_category') or ''}")
78+
return rows
79+
80+
81+
# --- behaviour gate ----------------------------------------------------------
82+
83+
def is_behaviour_key(key):
84+
return key.startswith("on_") or key.endswith("_action") or key in (
85+
"lambda", "then", "condition")
86+
87+
88+
def strip_comments(text):
89+
"""Drop whole-line C++ comments. Only lines that START with // are removed --
90+
a trailing-comment rule would truncate URLs like https://... in literals."""
91+
return "\n".join(
92+
ln for ln in text.splitlines() if not ln.lstrip().startswith("//"))
93+
94+
95+
def canon(obj):
96+
"""Whitespace- and comment-insensitive, so reformatting or a reworded
97+
comment is not reported as a behaviour change."""
98+
if isinstance(obj, str):
99+
return " ".join(strip_comments(obj).split())
100+
if isinstance(obj, list):
101+
return [canon(v) for v in obj]
102+
if isinstance(obj, dict):
103+
return {k: canon(v) for k, v in sorted(obj.items())}
104+
return obj
105+
106+
107+
def behaviours(cfg):
108+
"""anchor -> body. Anchors are stable identities (entity name, script id,
109+
boot priority) rather than list positions, so a reordered package merge
110+
does not look like a change."""
111+
out = {}
112+
113+
def put(anchor, key, body):
114+
out[(anchor, key)] = json.dumps(canon(body), sort_keys=True)
115+
116+
for boot in (cfg.get("esphome") or {}).get("on_boot") or []:
117+
if isinstance(boot, dict):
118+
put("esphome:on_boot", str(boot.get("priority", "?")), boot.get("then"))
119+
120+
for scr in cfg.get("script") or []:
121+
if isinstance(scr, dict):
122+
put(f"script:{scr.get('id','?')}", "then", scr.get("then"))
123+
124+
for iv in cfg.get("interval") or []:
125+
if isinstance(iv, dict):
126+
put(f"interval:{iv.get('interval','?')}", "then", iv.get("then"))
127+
128+
for domain in DOMAINS:
129+
block = cfg.get(domain)
130+
if not isinstance(block, list):
131+
continue
132+
for item in block:
133+
if not isinstance(item, dict):
134+
continue
135+
anchor = f"{domain}:{item.get('name') or item.get('id') or '?'}"
136+
for key, val in sorted(item.items()):
137+
if is_behaviour_key(key):
138+
put(anchor, key, val)
139+
elif isinstance(val, dict):
140+
for k2, v2 in sorted(val.items()):
141+
if is_behaviour_key(k2):
142+
put(f"{anchor}.{key}", k2, v2)
143+
return out
144+
145+
146+
# --- reporting ---------------------------------------------------------------
147+
148+
def report_entities(old, new):
149+
removed, added = sorted(old - new), sorted(new - old)
150+
print("=" * 72)
151+
print("ENTITY GATE")
152+
print("=" * 72)
153+
if not removed and not added:
154+
print(" clean - entity lists identical\n")
155+
return True
156+
for row in removed:
157+
print(f" REMOVED {row}")
158+
for row in added:
159+
print(f" ADDED {row}")
160+
print(f"\n {len(removed)} removed, {len(added)} added -- each must be intended and called out.\n")
161+
return False
162+
163+
164+
def report_behaviours(old, new):
165+
changed = sorted(k for k in set(old) & set(new) if old[k] != new[k])
166+
dropped = sorted(set(old) - set(new))
167+
gained = sorted(set(new) - set(old))
168+
print("=" * 72)
169+
print("BEHAVIOUR GATE")
170+
print("=" * 72)
171+
if not changed and not dropped and not gained:
172+
print(" clean - all automation bodies identical\n")
173+
return True
174+
for k in changed:
175+
print(f" CHANGED {k[0]} :: {k[1]}")
176+
for k in dropped:
177+
print(f" DROPPED {k[0]} :: {k[1]}")
178+
for k in gained:
179+
print(f" NEW {k[0]} :: {k[1]}")
180+
print(f"\n {len(changed)} changed, {len(dropped)} dropped, {len(gained)} new.")
181+
print(" Re-run with --detail <anchor> to see the bodies side by side.\n")
182+
return False
183+
184+
185+
def detail(old, new, needle):
186+
for key in sorted(set(old) | set(new)):
187+
if needle.lower() not in f"{key[0]} {key[1]}".lower():
188+
continue
189+
o, n = old.get(key), new.get(key)
190+
if o == n:
191+
continue
192+
print("=" * 72)
193+
print(f"{key[0]} :: {key[1]}")
194+
print("-" * 30 + " OLD " + "-" * 30)
195+
print(json.dumps(json.loads(o), indent=2) if o else "(absent)")
196+
print("-" * 30 + " NEW " + "-" * 30)
197+
print(json.dumps(json.loads(n), indent=2) if n else "(absent)")
198+
199+
200+
def main():
201+
ap = argparse.ArgumentParser()
202+
ap.add_argument("old")
203+
ap.add_argument("new")
204+
ap.add_argument("--detail", help="show bodies for anchors matching this substring")
205+
args = ap.parse_args()
206+
207+
old_cfg, new_cfg = load(args.old), load(args.new)
208+
old_b, new_b = behaviours(old_cfg), behaviours(new_cfg)
209+
210+
if args.detail:
211+
detail(old_b, new_b, args.detail)
212+
return 0
213+
214+
ok_e = report_entities(entities(old_cfg), entities(new_cfg))
215+
ok_b = report_behaviours(old_b, new_b)
216+
return 0 if (ok_e and ok_b) else 1
217+
218+
219+
if __name__ == "__main__":
220+
sys.exit(main())

0 commit comments

Comments
 (0)