From 88cd97cc7a228c3b18197cef02c1564857f2c55c Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 20:29:16 +0200 Subject: [PATCH] SAFETY-12: the loop thinks on a clock, not only when the number moves run_control() ran cycle() only when the meter value differed from the previous poll. A meter that repeats a value therefore ran no control cycles at all - while the heartbeat kept re-emitting the last command and the firmware kept putting it on the wire every 2 s. Layer 1 stayed fed and every indicator read healthy, so a command computed from an old reading went on being executed through whatever the load did next. TEL-05 measured it: 70 s at a literally constant 350.0 W, 14 arrivals, zero cycles. cycle_floor_s (default 15 s) is a deadline on the GAP between cycles, reset by every cycle whichever trigger fired - not a second scheduler. On a healthy meter it never elapses, so the cycle rate and the commanded sequence are unchanged; test_loop.py asserts that by driving the real loop twice on one binary (a huge cycle_floor_s reproduces the old behaviour) and diffing the writes. 15 s is bracketed: above the ~11 s worst gap on a merely-unlucky healthy meter (~5 s arrivals, 1 s detection, one dropped poll retried on the next tick), below meter_max_age_s and the firmware's 30 s ingest veto, and equal to stale_input_s. A floor cycle runs the full law. Freshness is already decided upstream - past meter_max_age_s the reading is None, which is a P_SAFETY 0 W claim - and the integrator advances per cycle, not per second, so bounding the gap bounds the accumulation. Not a freeze detector: nothing here decides whether a repeated reading is true. DOCS.md's "accepted deviation" passage is qualified rather than deleted - the window is now bounded at 3 x cycle_floor_s instead of open-ended, and its "nothing accumulates while it is stalled" argument is called out as having covered windup and not staleness. Same correction in control.py's comment. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/CHANGELOG.md | 56 ++++++++++ goodwe_controller/DOCS.md | 50 ++++++--- goodwe_controller/app/control.py | 28 ++--- goodwe_controller/app/main.py | 45 +++++++- goodwe_controller/config.yaml | 2 + goodwe_controller/test_loop.py | 174 +++++++++++++++++++++++++++++++ 6 files changed, 327 insertions(+), 28 deletions(-) create mode 100644 goodwe_controller/test_loop.py diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 8f996c3..5457dfd 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -67,6 +67,62 @@ A refusal is legible as a refusal: a banner that says in words that this is a configuration problem and nothing is broken, the numbered problems, no traceback, and exit status 78 (`EX_CONFIG`). +**SAFETY-12.** The control loop now evaluates on a clock as well as on a change. +`run_control()` ran `cycle()` only when the meter value differed from the +previous poll, so a meter that repeated a value ran **no control cycles at +all** - while the heartbeat went on re-emitting the last command and the +firmware went on putting it on the wire every 2 s. Layer 1 stayed fed, every +indicator read healthy, and a command computed from an old reading kept being +executed through whatever the load did next. Measured on the rig during TEL-05: +70 s at a literally constant 350.0 W with 14 arrivals and zero cycles. + +`cycle_floor_s` (default **15 s**) is a deadline on the gap between cycles, not +a second scheduler: every cycle resets it, whichever trigger fired. On a healthy +meter it never elapses, so the cycle rate, the write sequence and therefore the +per-cycle gain the tuning was measured against are unchanged - asserted in +`test_loop.py` by driving the real loop twice on one binary (a huge +`cycle_floor_s` reproduces the old behaviour exactly) and comparing the +commanded values. + +15 s is bracketed, not round. Below it: arrivals are ~5 s (NOTES.md measures +4.97 s), detected up to 1 s late by the 1 s poll, and one dropped poll is +retried on the next tick - about 11 s worst case on a meter that is merely +unlucky, which the floor must not fire on. Above it: `meter_max_age_s` is 30 s, +past which there is no reading to evaluate, and the firmware's ingest veto is +also 30 s - so the loop is guaranteed to have re-decided at least twice inside +any window in which layer 1 might otherwise be the only thing deciding. It is +also exactly `stale_input_s`, this system's existing answer to "how long may we +hold without re-deciding". + +A floor cycle runs the **full** law on the same inputs, integrator included. The +freshness question is already answered upstream and answered harder - past +`meter_max_age_s` the reading is None, which is "inputs missing", which is a +`P_SAFETY` 0 W claim - so a floor cycle can only ever run on a reading this +system already calls fresh. The integrator advances per *cycle*, not per second, +so bounding the gap bounds the accumulation; `integrator_max_w` bounds it after +that. A gentler second law for floor cycles would be a second set of dynamics +nobody can test on this hardware. + +⚠️ **This is not a freeze detector**, and TEL-05 established one cannot be built +here: at the converged -10 W target a 1 Wh register needs ~6 minutes to move +while the power figure legitimately repeats, so any threshold on `unchanged_s` +false-trips at the exact operating point we run at. Nothing in this change +decides whether a repeated reading is *true*. It stops "the number is the same" +from meaning "do not think". The consequence, stated plainly: against a meter +genuinely frozen at a wrong value the loop will now correct toward that wrong +value rather than hold a stale command, bounded by `max_w`, the arbiter and the +device rating. That was already what happened to a lying meter whose value +merely jittered; bit-exact repetition was never a safety property. + +`DOCS.md`'s "Saturation is counted in cycles, not seconds" section documented +the old behaviour as an accepted deviation. It is **qualified, not deleted**: +the deviation survives (the window is still counted in cycles, and 3 x 15 s is +still not the specification's 10 s) but it is now bounded at 45 s instead of +open-ended, and the passage's second justification - "nothing accumulates while +it is stalled" - is called out as having covered integrator windup and not +staleness, which was the actual hazard. The same wrong argument in +`control.py`'s saturation comment is corrected too. + **TEL-04.** `meter_source: ha_signed` reads a single *signed* power entity, which is the shape a HomeWizard P1 publishes. Both transports 0.3.0 shipped require two unsigned registers and refuse negatives, so neither fits the meter installed here. diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index a6f8718..82c3731 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -272,9 +272,10 @@ behind it would trip the firmware watchdog on a system that is working fine. | `target_grid_w` | -10 | What the meter should rest at. Negative = a slight export | | `step_w` | 10 | Quantisation | | `saturation_w` | 500 | Divergence that counts as "the inverter is at a limit" | -| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one *changed* meter reading, not a fixed period - see the note below. **Do not set to 1** | +| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one meter update or `cycle_floor_s`, whichever comes first - see the note below. **Do not set to 1** | | `integrator_max_w` | 0 | Bound on the loop's accumulator, and 0 means "same as `max_w`". Caps how much stale error can be waiting to unwind when the sign flips. **Do not raise it above `max_w`** - the output clamp already bounds what is commanded, so the only thing extra headroom buys is more cycles of wrong-direction power after every saturation event. Lowering it below `max_w` is the useful direction | | `heartbeat_s` | 10 | Refresh interval; must stay well under the firmware watchdog | +| `cycle_floor_s` | 15 | The longest the loop may go without evaluating. A deadline on the gap between cycles, reset by every cycle — on a healthy meter it never elapses and changes nothing. Raise it only if your meter is slower than ~10 s; it must stay under `meter_max_age_s`, past which there is no reading to evaluate anyway | | `stale_input_s` | 15 | How long inputs may be missing before commanding 0 W. In P1 mode this clock starts only *after* `meter_max_age_s` has already expired — the two stack, see "How long a dead meter takes to reach 0 W" | | `auto_start` | false | Start controlling on boot (only after commissioning) | @@ -284,22 +285,43 @@ The specification states the saturation window as **"> 10 s"**. This add-on coun **cycles** instead, and that is a deliberate, accepted deviation rather than an oversight - the acceptance criterion is not met as literally written. -A cycle here is one *changed* meter reading: the controller only runs the loop when the -meter value differs from the previous poll. At the reference P1's ~5 s update rate the -default of 3 cycles is usually around 15 s, but there is **no guaranteed wall-clock -window** - a meter that repeats the same value stalls the counter for as long as it -repeats. +A cycle here is one meter update **or `cycle_floor_s` elapsing, whichever comes +first**. At the reference P1's ~5 s update rate the default of 3 cycles is usually +around 15 s, and the worst case is bounded at `3 x cycle_floor_s` = 45 s. It is still +not the 10 s the specification asks for, so the deviation stands - but it is now a +bounded number rather than an open one. -Two reasons that is acceptable: +⚠️ **This passage used to say something different, and the second half of it was +wrong.** Until SAFETY-12 the loop ran only when the meter value *changed*, so a meter +that repeated a value stalled the counter for as long as it repeated, with no upper +bound at all. That was defended here on the grounds that "a stalled counter is a +detection-latency limit and not a runaway risk - the condition that stalls it stops the +whole loop, so nothing accumulates while it is stalled". That argument covers +**integrator windup and nothing else**. The hazard it misses is **staleness**: while the +loop was stopped the heartbeat kept re-emitting the last command and the firmware kept +putting it on the wire every 2 s, so a command computed from an old reading went on +being executed through a real load change, with every indicator reading healthy. +Measured on the rig during TEL-05: 70 s at a literally constant 350.0 W, 14 arrivals, +zero control cycles. -- the control law is a pure function with no clock, which is what makes it testable - without hardware, and a seconds-based window would have to live in the controller; -- a stalled counter is a detection-latency limit and not a runaway risk. The condition - that stalls it - an unchanging meter - stops the whole loop, so nothing accumulates - while it is stalled. +The half that survives is the first one: the control law is still a pure function with +no clock, and the wall-clock window still lives in `Controller`, which has one - see +`run_control()`. What changed is that the controller now enforces a floor on the gap +between cycles, so `saturation_cycles` finally means a bounded number of seconds. -If a guaranteed window matters on your site, raise `saturation_cycles` for a fast meter, -and treat the figure as "N meter updates" rather than "N seconds". +If a tighter window matters on your site, lower `cycle_floor_s` (not below your meter's +update interval, or the floor starts firing on a healthy meter) or raise +`saturation_cycles` for a fast meter. + +**This is not a freeze detector, and it deliberately cannot be one.** Nothing here +decides whether a repeated reading is true — see `unchanged_s` in the meter section for +why no threshold on that is safe at the converged operating point. The floor only stops +"the number is the same" from meaning "do not think". The consequence is worth stating +plainly: against a meter that is genuinely frozen at a wrong value, the loop will now +correct toward that wrong value instead of holding a stale command. It is bounded by +`max_w`, the arbiter and the device rating, and it is the same thing that already +happened to a lying meter whose value merely jittered — bit-exact repetition was never a +safety property. #### Why `target_grid_w` is not zero diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 8c00dca..d34c205 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -95,19 +95,23 @@ def compute( # lets slew be larger than saturation_w. # # The spec states this window twice and differently: "> 10 s" (§11.2) and - # "3 samples" (§10.3). This counts CYCLES, and a cycle is not a unit of - # time: run_control() calls cycle() only when the meter value CHANGES - # (`if self.grid != last_grid`), so three cycles is three distinct meter - # readings and nothing more. At the reference P1's ~5 s update rate that is - # usually ~15 s, but there is no upper bound on it - a meter that repeats a - # value stalls the counter. + # "3 samples" (§10.3). This counts CYCLES, and a cycle is still not a unit + # of time - but it is now a BOUNDED one. run_control() runs a cycle on a + # changed meter value OR on `cycle_floor_s` elapsing, whichever comes first + # (SAFETY-12), so three cycles is at most 3 x cycle_floor_s = 45 s at the + # defaults, and usually ~15 s at the reference P1's ~5 s update rate. # - # That is a detection-latency limit, not a windup hazard: the same - # condition that stalls the counter stalls the whole loop, so nothing - # accumulates in the meantime either. If a wall-clock window is ever - # required, it belongs in Controller (which has a clock) and not here. - # ponytail: this function is worth keeping clockless; the ceiling is that - # saturation_cycles cannot express a guaranteed number of seconds. + # ⚠️ THIS COMMENT USED TO CLAIM there was no upper bound and that it did not + # matter, "because the same condition that stalls the counter stalls the + # whole loop, so nothing accumulates in the meantime". The first clause was + # true and the second was the bug: a stalled loop does not stop commanding, + # it goes on emitting its last command through the heartbeat. Windup was + # never the hazard; staleness was. + # + # The wall-clock window lives in Controller, which has a clock, exactly as + # this comment always said it should. ponytail: this function stays + # clockless; the remaining ceiling is that saturation_cycles expresses a + # bound in seconds rather than an exact number of them. saturated_now = abs(prev_w - actual_w) > tuning.saturation_w sat_count = min(sat_count + 1, 10) if saturated_now else 0 frozen = sat_count >= tuning.saturation_cycles diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index f4a20cd..4c34014 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -342,11 +342,52 @@ class Controller: last_grid = object() heartbeat = float(self.o.get("heartbeat_s", 10)) last_beat = 0.0 + # ⚠️ SAFETY-12. The floor is a DEADLINE ON THE GAP BETWEEN CYCLES, not a + # second scheduler running alongside the arrival trigger. Every cycle + # resets it, whichever trigger fired, so on a healthy changing meter it + # never elapses and not one existing cycle moves - no beat frequency + # between the two triggers, no doubled cycle rate, and therefore no + # change to the per-cycle gain the tuning was measured against. + # + # Why it has to exist: `self.grid != last_grid` alone means a meter that + # repeats a value runs NO control cycles at all, while the heartbeat + # below keeps re-emitting the last command and the firmware keeps + # putting it on the wire - so every indicator reads healthy while the + # loop has not evaluated and the load may have moved underneath it. + # Measured on the rig during TEL-05: 70 s at a literally constant + # 350.0 W, 14 arrivals, zero cycles. + # + # This is NOT a freeze detector, and deliberately so - see + # HomeWizardLocalSource.unchanged_s for why one cannot be built here (at + # the converged -10 W target a 1 Wh register needs ~6 minutes to move, + # so any freeze threshold false-trips at the exact operating point we + # run at). Nothing here decides whether a repeated reading is true. It + # only stops "the number is the same" from meaning "do not think". + floor = float(self.o.get("cycle_floor_s", 15)) + last_cycle = None while not self.stopping: await self.read_inputs() - # A cycle is one meter update, exactly as on the reference install. - if self.grid != last_grid: + now_mono = asyncio.get_running_loop().time() + # A cycle is one meter update, exactly as on the reference install - + # or the floor elapsing, whichever comes first. + overdue = last_cycle is None or (now_mono - last_cycle) >= floor + if self.grid != last_grid or overdue: last_grid = self.grid + last_cycle = now_mono + # ⚠️ The FULL cycle, not a degraded one. A floor cycle runs the + # same law on the same inputs as an arrival cycle, because the + # freshness question is already answered upstream and answered + # harder: past `meter_max_age_s` (30 s) p1.net_w is None, which + # is "inputs missing", which becomes a P_SAFETY 0 W claim. So a + # floor cycle can only ever run on a reading this system already + # calls fresh - it cannot run the PI on a 60 s old figure, + # because at 60 s there is no figure. And the integrator + # advances per CYCLE, not per second (control.py), so bounding + # the gap bounds the accumulation: at most one extra + # gain x error per floor interval, under the same + # integrator_max_w as every other cycle. A second, gentler + # control law for floor cycles would be a second set of + # dynamics nobody can test on this hardware. await self.cycle() # ⚠️ The heartbeat is not an optimisation. The ESP32 treats silence # longer than ~30 s as "the controller is gone" and zeroes the diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index 350e3fe..a30ac79 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -90,6 +90,7 @@ options: integrator_max_w: 0 heartbeat_s: 10 stale_input_s: 15 + cycle_floor_s: 15 auto_start: false # --- maintenance ----------------------------------------------------------- @@ -151,6 +152,7 @@ schema: integrator_max_w: int(0,15000) heartbeat_s: int(2,25) stale_input_s: int(5,120) + cycle_floor_s: int(5,60) auto_start: bool maintenance_enabled: bool diff --git a/goodwe_controller/test_loop.py b/goodwe_controller/test_loop.py new file mode 100644 index 0000000..01b1da6 --- /dev/null +++ b/goodwe_controller/test_loop.py @@ -0,0 +1,174 @@ +"""Runnable check for the control loop's cadence. `python3 test_loop.py` + +SAFETY-12: the loop must evaluate on a clock as well as on a change. Before the +fix, `run_control()` ran `cycle()` only when the meter value differed from the +previous poll, so a meter that repeats a value ran NO control cycles while the +heartbeat kept re-emitting the last command - measured on the rig during TEL-05 +as 70 s at a literally constant 350.0 W with 14 arrivals and zero cycles. + +⚠️ THIS TEST TAKES ~35 SECONDS OF WALL CLOCK, on purpose. The thing under test +is a wall-clock floor inside a loop that sleeps 1 s; mocking the clock would +leave the assertion testing the mock. It drives the real `run_control()` and +counts what actually happened. + +`cycle_floor_s` is read from options with no lower bound in code, so a huge +value reproduces the pre-fix behaviour exactly. Every claim below is therefore +an A/B on one binary, not a comparison against a remembered old one. +""" + +import asyncio +import sys +import time + +from app.main import Controller + +fails = [] +NEVER = 1e9 # cycle_floor_s so large the floor can never elapse: pre-fix + + +def check(name, cond): + print(f" {'ok ' if cond else 'FAIL'} {name}") + if not cond: + fails.append(name) + + +class FakeHass: + """A scripted meter and a plant that tracks the command instantly.""" + + def __init__(self, meter): + self.meter = meter + self.batt = 0.0 + self.writes = [] + self.t0 = time.monotonic() + + async def limits(self, entity): + return (-5000.0, 5000.0, 1.0) + + async def number(self, entity, invert=False): + if entity == "sensor.grid": + return self.meter(time.monotonic() - self.t0) + if entity == "sensor.batt": + return self.batt + if entity == "sensor.soc": + return 50.0 + return None + + async def set_number(self, entity, value): + self.writes.append(float(value)) + self.batt = float(value) + return True + + +class _Store: + data = {} + + def set(self, *a): + pass + + +class _Pub: + enabled = False + + def publish(self, *a, **k): + pass + + def close(self): + pass + + +BASE = {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": 2000, "gain": 0.6, "slew_w": 1000, + "deadband_w": 15, "target_grid_w": -10, "step_w": 10, + "heartbeat_s": 10, "stale_input_s": 15} + + +def drive(meter, floor, seconds): + """Run the real control loop for `seconds`; return (cycle times, writes).""" + async def go(): + hass = FakeHass(meter) + c = Controller(dict(BASE, cycle_floor_s=floor), hass, _Store(), _Pub(), + stop=asyncio.Event()) + c.auto = True + stamps = [] + real = c.cycle + + async def counted(): + stamps.append(time.monotonic() - hass.t0) + await real() + + c.cycle = counted + task = asyncio.create_task(c.run_control()) + await asyncio.sleep(seconds) + c.stopping = True + await asyncio.wait_for(task, timeout=10) + return stamps, hass.writes + return asyncio.run(go()) + + +def worst_gap(stamps, seconds): + """Longest stretch with no cycle, including the tail after the last one.""" + edges = [0.0] + list(stamps) + [seconds] + return max(b - a for a, b in zip(edges, edges[1:])) + + +# --------------------------------------------------------------------------- # +print("a meter that repeats the same value") +FLAT = lambda t: 350.0 # noqa: E731 +FLOOR, SECONDS = 3.0, 12.0 + +pre_stamps, pre_writes = drive(FLAT, NEVER, SECONDS) +# The defect itself, demonstrated rather than described: one cycle at startup +# and then nothing for the rest of the window, while writes keep happening. +check("pre-fix: a constant meter runs exactly one cycle, at startup", + len(pre_stamps) == 1) +check("...while the heartbeat keeps writing anyway, so nothing looks wrong", + len(pre_writes) >= 2) + +post_stamps, post_writes = drive(FLAT, FLOOR, SECONDS) +check("post-fix: the same constant meter runs cycles throughout", + len(post_stamps) >= int(SECONDS / FLOOR) - 1) +# The actual acceptance criterion: a BOUND on the gap, not a count. +gap = worst_gap(post_stamps, SECONDS) +check(f"...with no gap longer than the floor ({gap:.1f}s <= {FLOOR + 1.5:.1f}s)", + gap <= FLOOR + 1.5) +check("...and the loop actually corrected, instead of holding a stale command", + len(post_writes) > len(pre_writes)) + +# --------------------------------------------------------------------------- # +print("a meter that changes normally - nothing may move") +# ⚠️ THE REGRESSION GUARD, and the more important half. The floor is a deadline +# on the gap between cycles, reset by every cycle whichever trigger fired - not +# a second scheduler. On a healthy meter it must never elapse, so the cycle +# count, the write count and the commanded values must be IDENTICAL either side +# of the fix. Anything else is a new write cadence, which SAFETY-12 forbids: +# the deadband, step_w and the 10 s force-write decide what reaches the wire, +# and this change is only allowed to alter when the loop thinks. +STEPS = [0.0, 300.0, 850.0, -400.0, 120.0, -900.0, 60.0] +LIVE = lambda t: STEPS[int(t // 2) % len(STEPS)] # noqa: E731 + +a_stamps, a_writes = drive(LIVE, NEVER, 12.0) +b_stamps, b_writes = drive(LIVE, FLOOR, 12.0) +check("a changing meter runs the same number of cycles as before the fix", + len(a_stamps) == len(b_stamps)) +check("...commanding exactly the same sequence of values", a_writes == b_writes) + +# --------------------------------------------------------------------------- # +print("a meter parked at the target - the converged operating point") +# The case TEL-05 warned about: at the converged target the power figure +# legitimately repeats for minutes. The loop must still run, and must still +# write nothing but the heartbeat, because the deadband - not the arrival +# trigger - is what holds the command still. +AT_TARGET = lambda t: -10.0 # noqa: E731 +c_stamps, c_writes = drive(AT_TARGET, NEVER, 12.0) +d_stamps, d_writes = drive(AT_TARGET, FLOOR, 12.0) +check("the loop evaluates at the converged point too", len(d_stamps) > len(c_stamps)) +check("...and the deadband still holds the command, so writes are unchanged", + d_writes == c_writes) + +# --------------------------------------------------------------------------- # +print() +if fails: + print(f"{len(fails)} FAILED: " + "; ".join(fails)) + sys.exit(1) +print("all checks passed")