Restore Ropener stall handling lost in the valar-core rewire (v2.7.0 regression) (#34)

* 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>

* fix(firmware): mark the cover as device_class curtain

The cover had no device_class, so Home Assistant fell back to generic up/down
arrow controls. A Ropener draws sideways; "curtain" gets the horizontal
open/close controls, the right icon, and better voice-assistant phrasing.

Note this does NOT change the device's own web UI: web_server v3 hardcodes
the cover glyphs ("up", "stop", "down" in render_cover) and never reads
device_class. Home Assistant only.

Both gates clean -- entity list and all automation bodies identical; the
resolved config differs by exactly this one line. Included in v2.7.1 because
the bench-tested build had it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(firmware): stop rebooting every 15 min without a Home Assistant client

The API reboot_timeout defaults to 15 minutes: with no *API* client connected
the device reboots. It counts native-API clients only -- a browser sitting on
the web UI is not one.

The Ropener is sold as working without Home Assistant, so any customer driving
it from the browser had a controller that silently rebooted every 15 minutes,
stopping the curtain mid-travel if it happened to be moving at the time.
Observed on the bench unit: "[E][api:127] No clients; rebooting", uptime
resetting on a 15 minute cycle.

Not a rewire regression -- v2.6.4 resolves the same 15min default. It is
pre-existing in every Ropener release; the bench session just made it visible.

Set at the product layer rather than in valar-core: core declares a bare
`api:`, so the option merges in cleanly with no !remove needed, and this ships
without a valar-motion retag. It should move into valar-core later, since the
whole family is sold as HA-optional.

Trade-off: this also removes the watchdog that recovers a wedged API
connection. Acceptable -- the users it was rebooting are precisely the ones not
using the API -- and safe_mode still covers boot loops.

Both gates clean; the resolved config differs by exactly one line
(reboot_timeout 15min -> 0s).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: daniel-frenkel

firmware/VAL3000/Ropener-VAL3000.yml

@@ -12,7 +12,9 @@ substitutions:
   log_uart: "USB_SERIAL_JTAG"
   ota_repo: "Valar-Systems/Ropener"
   ota_asset: "Ropener-VAL3000"
-  fw_version: "2.7.0"
+  # "dev" for local/branch builds; the release workflow overrides it with the
+  # git tag (-s fw_version) so shipped units report the version they run.
+  fw_version: "dev"
   pin_uart_tx: "6"
   pin_uart_rx: "5"
   pin_enn: "8"

firmware/VAL3100/Ropener-VAL3100.yml

@@ -13,7 +13,9 @@ substitutions:
   log_uart: "USB_SERIAL_JTAG"
   ota_repo: "Valar-Systems/Ropener"     # field units self-update from Ropener's own releases
   ota_asset: "Ropener-VAL3100"
-  fw_version: "2.7.0"
+  # "dev" for local/branch builds; the release workflow overrides it with the
+  # git tag (-s fw_version) so shipped units report the version they run.
+  fw_version: "dev"
   pin_uart_tx: "0"
   pin_uart_rx: "1"
   pin_enn: "5"

firmware/common/ropener-product.yaml

@@ -29,6 +29,19 @@ substitutions:
 # stepper; here we resync the derived step count and restore the saved position
 # so the cover comes up accurate without a re-home. (scheduling re-applies
 # tz/location at priority -100, after this.)
+# The API's reboot_timeout defaults to 15 min: if no *API* client connects in
+# that window the device reboots. It counts native-API clients only — a browser
+# on the web UI is not one. The Ropener is sold as working without Home
+# Assistant, so a customer driving it purely from the browser got a controller
+# that rebooted every 15 minutes, stopping the curtain mid-travel if it happened
+# to be moving. 0 = never reboot for lack of a client.
+#
+# Trade-off: this also removes the watchdog that recovers a wedged API
+# connection. Acceptable here — the users it was rebooting are the ones not
+# using the API at all — and safe_mode still covers boot loops.
+api:
+  reboot_timeout: 0s
+
 esphome:
   on_boot:
     - priority: 400.0
@@ -58,6 +71,40 @@ globals:
   - { id: b1_tap_count,   type: int,      restore_value: no, initial_value: '0' }
   - { id: b1_last_tap_ms, type: uint32_t, restore_value: no, initial_value: '0' }
 
+# --- Stall handling (homing) ------------------------------------------------
+# The Ropener homes sensorlessly: drive at the closed end until StallGuard fires,
+# and treat THAT stall as position 0. Core's default on_stall zeroes the position
+# unconditionally, which is wrong here — a stall mid-travel (curtain snags) would
+# silently redefine that spot as home and corrupt the cover's position reference.
+#
+# `!extend` APPENDS to core's action list rather than replacing it, so extending
+# alone would leave core's unconditional zeroing firing before ours. The two
+# entries below are deliberate and order-dependent: the first deletes core's
+# handler outright, the second installs the Ropener's conditional one. Verified
+# against the resolved config — exactly one on_stall survives. Keep both.
+stepper:
+  - id: !extend driver
+    on_stall: !remove
+  - id: !extend driver
+    on_stall:
+      - stepper.stop: driver
+      - if:
+          condition:
+            lambda: 'return id(global_state) == 3;'
+          then:
+            - stepper.report_position: { id: driver, position: 0 }
+            - stepper.set_target: { id: driver, target: 0 }
+            - globals.set: { id: global_state, value: "0" }
+            - cover.template.publish: { id: ropener_cover, current_operation: IDLE }
+            - logger.log: "Homed cover"
+          # Stall outside homing (curtain snagged, or it hit the far end): stop
+          # and settle the UI back to idle, but LEAVE the position alone — this
+          # point is not home, and zeroing here is what corrupted v2.7.0.
+          else:
+            - globals.set: { id: global_state, value: "0" }
+            - cover.template.publish: { id: ropener_cover, current_operation: IDLE }
+            - logger.log: "Stall detected outside homing — motor stopped, position unchanged"
+
 script:
   # Recompute the derived step cache from the persisted centimeters (one place).
   - id: recompute_distance
@@ -298,6 +345,27 @@ binary_sensor:
           - delay: 1s
           - lambda: "App.safe_reboot();"
 
+# --- Preserve the shipped "Motor Direction" option strings --------------------
+# Field units (and any Home Assistant automation that selects a direction) know
+# this option as "Reversed ↺" — the glyph is part of the string, not decoration.
+# valar-core dropped it to a plain "Reversed", which would silently break those
+# automations and leave existing units holding an option that no longer exists.
+# `options` is a list, so !extend would APPEND and yield three choices; remove
+# then re-add instead. `lambda` is a scalar, so extending overwrites it cleanly.
+# Core's set_action matches on `x.find("Reversed")`, which still hits — no
+# override needed there.
+select:
+  - id: !extend sel_direction
+    options: !remove
+  - id: !extend sel_direction
+    options:
+      - "Normal"
+      - "Reversed ↺"
+    lambda: |-
+      if (id(global_stepper_direction) == esphome::tmc2209::ShaftDirection::COUNTERCLOCKWISE)
+        return std::string("Reversed ↺");
+      return std::string("Normal");
+
 # --- Rename core tuning entities to Ropener's existing entity_ids -------------
 number:
   - id: !extend num_irun
@@ -333,6 +401,10 @@ cover:
   - platform: template
     id: ropener_cover
     name: Ropener
+    # A curtain draws sideways, not up and down. With no device_class the UI
+    # falls back to generic up/down arrows; "curtain" gets the horizontal
+    # open/close controls (and the right icon in Home Assistant).
+    device_class: curtain
     web_server: { sorting_weight: 1, sorting_group_id: group_control }
     has_position: true
     assumed_state: false

tools/regression-gate/README.md

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