From 37bac79ad8ccd7f8be0769c53eb186e5b3e1040b Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 21:44:05 +0200 Subject: [PATCH 1/6] SAFETY-04: clamp the integrator, not only the output The loop was in velocity form: the accumulator WAS the commanded power, so "clamp the integrator" and "clamp the output" were the same line of code and could not be set apart. This makes the accumulator an explicit carried value (`i_w`), bounds it with its own `integrator_max_w`, and keeps the output clamp where it was. Killing either mechanism now still leaves the other holding - which is the point of the ticket, and what the new regression test asserts. Freeze semantics: while saturated the integrator may unwind but not wind further. A strict freeze would strand the command at whatever it reached, because the condition that releases it is the inverter tracking again, and not tracking is exactly what saturation means. The integrator is re-seeded from the arbiter's actual output whenever the loop did not get what it asked for, so entering any failsafe (all of which resolve to 0 W) zeroes it, and the first cycle after release does not dump the stale period as power. test_control.py: 24 -> 33 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/DOCS.md | 1 + goodwe_controller/app/control.py | 57 +++++++++++++++++++++++++-- goodwe_controller/app/main.py | 22 +++++++++++ goodwe_controller/config.yaml | 2 + goodwe_controller/test_control.py | 64 +++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 3 deletions(-) diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index c6f91fb..854a322 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -60,6 +60,7 @@ phase having charged nothing. | `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. **Do not set to 1** | +| `integrator_max_w` | 3000 | Bound on the loop's accumulator, separate from `max_w`. Caps how much stale error can be waiting to unwind when the sign flips. **Keep it above `max_w`, and do not set it equal to `max_w`** | | `heartbeat_s` | 10 | Refresh interval; must stay well under the firmware watchdog | | `stale_input_s` | 15 | How long inputs may be missing before commanding 0 W | | `auto_start` | false | Start controlling on boot (only after commissioning) | diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 0830b60..f12ae7d 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -28,6 +28,16 @@ class Tuning: step_w: int = 10 saturation_w: float = 500.0 saturation_cycles: int = 3 + # ⚠️ The integrator's OWN bound, and deliberately not max_w. A commercial + # controller on this same site clamped only its output and still reported + # 14 768 W: with the inverter switched off its integrator climbed ~130 W + # every 4 s past 10 kW while the output sat on the 5 kW rail, so the moment + # the error flipped there were minutes of accumulated wind to burn off + # before the command moved at all. Bounding the accumulator is what makes + # recovery time finite; bounding the output only hides it. + # Headroom above max_w is wanted (a legitimate large error must not be + # truncated at the rail), headroom without limit is the bug. + integrator_max_w: float = 3000.0 # What the meter should rest at, in W. Negative = a slight export. # ⚠️ The deadband is a one-way ratchet: any resting point inside it holds # forever, and the meter's IMPORT register counts every positive one with @@ -44,6 +54,10 @@ class Decision: sat_count: int frozen: bool reason: str + # The integrator AFTER this cycle, pre-clamp-to-max_w. Carry it back in as + # `i_w` next cycle; that is what keeps it a separate quantity from the + # command, which is the whole point of the bound above. + i_w: float = 0.0 def compute( @@ -52,12 +66,14 @@ def compute( actual_w: float, tuning: Tuning, sat_count: int = 0, + i_w: float | None = None, ) -> Decision: """One control cycle. A cycle is one meter update (~5 s on a HomeWizard P1). `prev_w` what we last commanded `grid_w` net grid power, + = importing `actual_w` what the inverter reports it is doing, + = discharging + `i_w` the carried integrator, or None to seed it from `prev_w` """ reason = "tracking" @@ -71,6 +87,16 @@ def compute( # on EVERY large correction, because the plant itself needs 3-6 s to settle # while a cycle is ~5 s. Requiring N consecutive saturated cycles is what # lets slew be larger than saturation_w. + # + # The spec states this window twice and differently: "> 10 s" (§11.2) and + # "3 samples" (§10.3). Cycles are authoritative here because this function + # has no clock - it is driven one cycle per meter update by run_control(), + # which only calls cycle() when the meter value changes. At the ~5 s + # HomeWizard P1 cadence the default 3 cycles is ~15 s, i.e. the stricter + # reading of the two. On a faster meter it is not, so saturation_cycles is + # configurable and must be raised to keep the window over 10 s. + # ponytail: a seconds-based window would mean plumbing wall-clock or dt + # into a pure function whose whole value is that it has neither. 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 @@ -83,11 +109,36 @@ def compute( # 70 s" is how a healthy loop is recognised at a glance, and a command # frozen where it should not be is how two real bugs were caught. error = grid_w - tuning.target_grid_w + + # --- the integrator ---------------------------------------------------- + # This loop is in velocity form: the accumulator IS the commanded power, so + # for years "the integrator" and "the output" were one variable and could + # not be bounded apart. `i_w` is that accumulator made explicit. A caller + # that passes nothing gets the old behaviour exactly - seeded from the last + # command every cycle - and main.py carries it instead, which is what turns + # the two clamps below into two independent limits. + if i_w is None: + i_w = float(prev_w) + if abs(error) < tuning.deadband_w: - want = prev_w reason = "deadband" else: - want = prev_w + tuning.gain * error + step_i = tuning.gain * error + # ⚠️ Freeze means "may not wind FURTHER", not "may not move". A strict + # freeze would strand the command at whatever it had reached until the + # inverter started tracking again - and the inverter is not tracking, + # that is what saturation means, so nothing would ever release it. The + # unwind direction is the escape route and stays open; the same rule is + # applied again to the output below. + if not frozen or abs(i_w + step_i) < abs(i_w): + i_w = i_w + step_i + + # ⚠️ Applied EVERY cycle, frozen or not, and before the output clamp: the + # freeze is conditional, this bound is not. Order matters only in that the + # command below is derived from the already-bounded integrator, so no + # accumulated value can reach the wire even once. + i_w = max(-tuning.integrator_max_w, min(tuning.integrator_max_w, i_w)) + want = i_w # ⚠️ Maintenance shaping (charge-only, cheap-window floor) used to live # here. It now belongs to arbiter.py as limit claims, so that precedence @@ -123,7 +174,7 @@ def compute( step = max(1, int(tuning.step_w)) target = round(target / step) * step - return Decision(float(target), sat_count, frozen, reason) + return Decision(float(target), sat_count, frozen, reason, float(i_w)) def maintenance_charge_floor( diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 1e2750e..e9128fc 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -70,6 +70,7 @@ class Controller: step_w=int(opts.get("step_w", 10)), saturation_w=float(opts.get("saturation_w", 500)), saturation_cycles=int(opts.get("saturation_cycles", 3)), + integrator_max_w=float(opts.get("integrator_max_w", 3000)), ) self.maint = Maintenance( MaintConfig( @@ -89,6 +90,8 @@ class Controller: self.auto = bool(store.data.get("auto", opts.get("auto_start", False))) self.target = 0.0 self.sat_count = 0 + self.i_w = 0.0 # the loop's integrator, carried between cycles + self.loop_w = None # what the loop asked for last cycle, or None self.reason = "starting" self.grid = self.soc = self.batt = None self.peak_fc = None @@ -231,17 +234,36 @@ class Controller: # wish. If something outranked the loop, that is what the hardware # actually did, and the controller must track reality or it jumps # the moment it regains control. + # + # ⚠️ The integrator has to track the same reality, and it is no + # longer prev_w, so it needs saying out loud: if the arbiter did not + # give the loop what it asked for last cycle, the loop's + # accumulated error belongs to a command that never happened. + # Re-seed from what the hardware was actually told. This is the + # failsafe case too - every layer-1 stop resolves to 0 W, so + # entering failsafe re-seeds the integrator to zero and the first + # cycle after release starts from zero instead of dumping the whole + # stale period as power. + if self.loop_w is None or self.target != self.loop_w: + self.i_w = self.target decision = compute( prev_w=self.target, grid_w=self.grid, actual_w=self.batt, tuning=self.tuning, sat_count=self.sat_count, + i_w=self.i_w, ) if decision.frozen and self.sat_count < self.tuning.saturation_cycles: self.log_event(f"saturation freeze ({self.target:.0f} W vs {self.batt:.0f} W)") self.sat_count = decision.sat_count + self.i_w = decision.i_w + self.loop_w = decision.target_w claims.append(Claim.set("loop", P_LOOP, decision.target_w, decision.reason)) + else: + # Stopped or blind: no accumulation may survive the outage. + self.i_w = 0.0 + self.loop_w = None resolution = resolve(claims) if resolution.contradiction: diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index 30892ec..e50a5fc 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -48,6 +48,7 @@ options: step_w: 10 saturation_w: 500 saturation_cycles: 3 + integrator_max_w: 3000 heartbeat_s: 10 stale_input_s: 15 auto_start: false @@ -88,6 +89,7 @@ schema: step_w: int(1,100) saturation_w: int(100,2000) saturation_cycles: int(1,10) + integrator_max_w: int(100,15000) heartbeat_s: int(2,25) stale_input_s: int(5,120) auto_start: bool diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 478629b..3a85516 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -73,6 +73,70 @@ check("freeze still allows magnitude to fall", d.target_w < 2000) d = compute(prev_w=0, grid_w=7, actual_w=0, tuning=Tuning(deadband_w=1, step_w=10)) check("quantised to step_w", d.target_w % 10 == 0) +print("SAFETY-04: the integrator is bounded apart from the output") + +# The historical runaway, with its real numbers. A commercial controller on +# this site, with the inverter switched OFF, wound ~130 W every 4 s past 10 kW +# and reported 14 768 W while its output clamp sat at 5 kW. At gain 0.6 that +# rate is a standing error of 130/0.6 ≈ 217 W that never resolves, because the +# inverter is not there to resolve it. 150 cycles is past the ~113 it took to +# reach 14 768 W at that rate. +RUNAWAY_ERROR = 130.0 / 0.6 +RUNAWAY_CYCLES = 150 +HISTORICAL_W = 14768.0 + + +def runaway(tuning): + """Inverter off: it reports 0 W forever, the error never clears.""" + prev, i_w, sat = 0.0, 0.0, 0 + worst_i, worst_cmd = 0.0, 0.0 + for _ in range(RUNAWAY_CYCLES): + d = compute(prev_w=prev, grid_w=RUNAWAY_ERROR, actual_w=0.0, + tuning=tuning, sat_count=sat, i_w=i_w) + prev, i_w, sat = d.target_w, d.i_w, d.sat_count + worst_i = max(worst_i, abs(i_w)) + worst_cmd = max(worst_cmd, abs(prev)) + return worst_i, worst_cmd + + +TR = Tuning(max_w=2000, integrator_max_w=3000) +wi, wc = runaway(TR) +check(f"runaway: integrator plateaus at {wi:.0f} W (<= 3000)", wi <= TR.integrator_max_w) +check(f"runaway: emitted command peaks at {wc:.0f} W (<= 2000)", wc <= TR.max_w) +check("runaway: nowhere near the historical 14 768 W", wc < HISTORICAL_W / 4) + +# ...and with the saturation detector deliberately defeated, so that only the +# clamp is holding. This is the AC that says the two mechanisms are +# independent: kill one, the other still bounds it. +TD = Tuning(max_w=2000, integrator_max_w=3000, saturation_w=1e9) +wi, wc = runaway(TD) +check(f"runaway with the detector defeated: integrator still <= 3000 ({wi:.0f} W)", + wi <= TD.integrator_max_w) +check("runaway with the detector defeated: command still <= max_w", wc <= TD.max_w) + +# The bound is not max_w. If someone "simplifies" them into one key this fails. +d = compute(prev_w=0, grid_w=6000, actual_w=0, + tuning=Tuning(max_w=2000, integrator_max_w=3000, slew_w=5000)) +check("integrator bound is separate from the output clamp", + d.i_w == 3000 and d.target_w == 2000) + +# Freeze = does not accumulate. Same input twice; the integrator must not move. +TF = Tuning(saturation_w=500, saturation_cycles=3) +f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) +check("frozen: integration does not accumulate", f1.i_w == 2000.0 and f1.frozen) +f2 = compute(prev_w=2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) +check("frozen: unwinding is still allowed", f2.i_w < 2000.0) + +# False-positive guard: a normal 2 kW load step must not trip the detector, +# because the plant needs several cycles to catch up on every one of them. +prev, actual, sat, i_w, froze = 0.0, 0.0, 0, 0.0, False +for _ in range(12): + d = compute(prev, 2000.0 - actual, actual, T, sat, i_w) + prev, sat, i_w = d.target_w, d.sat_count, d.i_w + actual = actual + 0.94 * (prev - actual) + froze = froze or d.frozen +check("a normal 2 kW load step does not trip the saturation freeze", not froze) + print("capacity tariff") check("no forecast means no cap", maintenance_charge_floor(2500, None, 3500) == 2500) check("headroom caps the charge", maintenance_charge_floor(2500, 2000, 3500) == 1500) From e46175559bbff29b5510a83e811b2e8ec4fe6760 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 22:13:55 +0200 Subject: [PATCH 2/6] SAFETY-04 review fixes: the freeze deadlocked, the bound was too loose S-1. The frozen branch admitted a correction only if it shrank |i_w|. That is unsatisfiable for BOTH signs of error whenever |correction| > 2*|i_w|, i.e. whenever the integrator is near zero, so the loop stopped moving and the freeze could never clear - it clears when the inverter tracks, and not tracking is what saturation means. Measured: 0 W held into a 2 kW import indefinitely, where release/1.0 recovers on the next cycle. Re-encoded as the same asymmetric rule the output freeze has always used: may not wind further in the direction it is already pushing, may fall, cross zero or reverse. Same interpretation, an encoding that cannot deadlock. S-2. integrator_max_w defaulted to 1.5x max_w, which ADDED windup: in release/1.0 the accumulator was the post-clamp command and could never pass the rail. Default is now "follow max_w" (config 0 = unset). Measured on the 4000 W load-drop sim, first cycle after the drop: 1000 W at the new default, 1800 W at 3000. DOCS row inverted - the useful direction is below max_w, and the 14 768 W anecdote is a vendor controller, not evidence about this code. S-3. The claim that i_w=None preserved release/1.0 exactly was false, because the S-1 gate ran regardless of seeding. It is true again, and now asserted rather than asserted-about: 3024-case exhaustive comparison against a transcription of the old law, over both freeze states, both signs and either side of the deadband. Added the carried-i_w convergence/overshoot sim that the shipped configuration was missing. S-4. Cycles are distinct meter values, not seconds: cycle() runs only when the meter reading changes, so the window has no wall-clock bound. Comment and DOCS corrected; the stall is detection latency, not a windup hazard, because the same condition stalls the whole loop. test_control.py: 33 -> 41 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/DOCS.md | 2 +- goodwe_controller/app/control.py | 107 ++++++++++++++++----------- goodwe_controller/app/main.py | 6 +- goodwe_controller/config.yaml | 4 +- goodwe_controller/test_control.py | 115 ++++++++++++++++++++++++++---- 5 files changed, 176 insertions(+), 58 deletions(-) diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 854a322..15961b6 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -60,7 +60,7 @@ phase having charged nothing. | `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. **Do not set to 1** | -| `integrator_max_w` | 3000 | Bound on the loop's accumulator, separate from `max_w`. Caps how much stale error can be waiting to unwind when the sign flips. **Keep it above `max_w`, and do not set it equal to `max_w`** | +| `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 | | `stale_input_s` | 15 | How long inputs may be missing before commanding 0 W | | `auto_start` | false | Start controlling on boot (only after commissioning) | diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index f12ae7d..9dbcae6 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -28,16 +28,22 @@ class Tuning: step_w: int = 10 saturation_w: float = 500.0 saturation_cycles: int = 3 - # ⚠️ The integrator's OWN bound, and deliberately not max_w. A commercial - # controller on this same site clamped only its output and still reported - # 14 768 W: with the inverter switched off its integrator climbed ~130 W - # every 4 s past 10 kW while the output sat on the 5 kW rail, so the moment - # the error flipped there were minutes of accumulated wind to burn off - # before the command moved at all. Bounding the accumulator is what makes - # recovery time finite; bounding the output only hides it. - # Headroom above max_w is wanted (a legitimate large error must not be - # truncated at the rail), headroom without limit is the bug. - integrator_max_w: float = 3000.0 + # The integrator's own bound. None means "follow max_w", which is the + # default and the recommended setting. + # + # ⚠️ DO NOT RAISE THIS ABOVE max_w without a measurement to justify it. + # Every watt of integrator above the rail is a watt of wind that has to be + # burned off before the command can start moving the other way, i.e. extra + # cycles of discharge into an already-exporting meter after every + # saturation event. Measured on the closed-loop sim, 4000 W load dropped to + # 0: at integrator_max_w == max_w the command is 1000 W two cycles later; at + # 1.5x max_w it is 1800 W. The output clamp already bounds what reaches the + # wire, so headroom here buys nothing but unwind latency. + # + # It is a separate key because it has to be able to be SMALLER than max_w, + # which is the only direction that buys anything: it caps unwind latency + # below what the rail implies. Merging it into max_w would take that away. + integrator_max_w: float | None = None # What the meter should rest at, in W. Negative = a slight export. # ⚠️ The deadband is a one-way ratchet: any resting point inside it holds # forever, and the meter's IMPORT register counts every positive one with @@ -54,9 +60,9 @@ class Decision: sat_count: int frozen: bool reason: str - # The integrator AFTER this cycle, pre-clamp-to-max_w. Carry it back in as - # `i_w` next cycle; that is what keeps it a separate quantity from the - # command, which is the whole point of the bound above. + # The integrator AFTER this cycle, before the output clamp, the slew limit + # and quantisation. Carry it back in as `i_w` next cycle; that is what keeps + # it a separate quantity from the command. i_w: float = 0.0 @@ -89,14 +95,19 @@ 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). Cycles are authoritative here because this function - # has no clock - it is driven one cycle per meter update by run_control(), - # which only calls cycle() when the meter value changes. At the ~5 s - # HomeWizard P1 cadence the default 3 cycles is ~15 s, i.e. the stricter - # reading of the two. On a faster meter it is not, so saturation_cycles is - # configurable and must be raised to keep the window over 10 s. - # ponytail: a seconds-based window would mean plumbing wall-clock or dt - # into a pure function whose whole value is that it has neither. + # "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. + # + # 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. 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 @@ -112,32 +123,48 @@ def compute( # --- the integrator ---------------------------------------------------- # This loop is in velocity form: the accumulator IS the commanded power, so - # for years "the integrator" and "the output" were one variable and could - # not be bounded apart. `i_w` is that accumulator made explicit. A caller - # that passes nothing gets the old behaviour exactly - seeded from the last - # command every cycle - and main.py carries it instead, which is what turns - # the two clamps below into two independent limits. + # "the integrator" and "the output" were one variable and could not be + # bounded apart. `i_w` is that accumulator made explicit; main.py carries it + # between cycles, which is what turns the two clamps into two limits. + # + # Passing i_w=None re-seeds it from the last command every cycle. With + # integrator_max_w following max_w that reduces this function to the exact + # velocity form it replaced, frozen branch included - asserted by an + # exhaustive comparison against a transcription of the old law in + # test_control.py, not by inspection. Break either the gate or the bound + # below and that test is what tells you the equivalence went with it. if i_w is None: i_w = float(prev_w) + limit = tuning.max_w if tuning.integrator_max_w is None else tuning.integrator_max_w if abs(error) < tuning.deadband_w: reason = "deadband" else: - step_i = tuning.gain * error - # ⚠️ Freeze means "may not wind FURTHER", not "may not move". A strict - # freeze would strand the command at whatever it had reached until the - # inverter started tracking again - and the inverter is not tracking, - # that is what saturation means, so nothing would ever release it. The - # unwind direction is the escape route and stays open; the same rule is - # applied again to the output below. - if not frozen or abs(i_w + step_i) < abs(i_w): - i_w = i_w + step_i + moved = i_w + tuning.gain * error + # ⚠️ Freeze means "may not wind FURTHER in the direction it is already + # pushing". It may fall, cross zero, or reverse outright. + # + # It must NOT be encoded as "only corrections that shrink |i_w|": that + # is unsatisfiable for BOTH signs of error whenever the correction is + # larger than twice the integrator, i.e. every time the integrator is + # near zero. The loop then sits at its last value forever, because what + # clears the freeze is the inverter tracking again and not-tracking is + # the definition of saturation. Measured on that encoding: 0 W held + # indefinitely into a 2 kW import, where this form recovers next cycle. + # + # This is the same asymmetric rule the output freeze uses below, which + # has been in service on real hardware. It is applied here as well + # because the requirement is that the INTEGRATOR stop accumulating, not + # only the command. + if not frozen: + i_w = moved + else: + i_w = min(moved, i_w) if i_w > 0 else max(moved, i_w) - # ⚠️ Applied EVERY cycle, frozen or not, and before the output clamp: the - # freeze is conditional, this bound is not. Order matters only in that the - # command below is derived from the already-bounded integrator, so no - # accumulated value can reach the wire even once. - i_w = max(-tuning.integrator_max_w, min(tuning.integrator_max_w, i_w)) + # ⚠️ Applied EVERY cycle, frozen or not: the freeze is conditional, this + # bound is not. It is what makes the worst-case unwind time finite and + # knowable instead of a function of how long the error happened to stand. + i_w = max(-limit, min(limit, i_w)) want = i_w # ⚠️ Maintenance shaping (charge-only, cheap-window floor) used to live diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index e9128fc..f42ec5c 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -70,7 +70,11 @@ class Controller: step_w=int(opts.get("step_w", 10)), saturation_w=float(opts.get("saturation_w", 500)), saturation_cycles=int(opts.get("saturation_cycles", 3)), - integrator_max_w=float(opts.get("integrator_max_w", 3000)), + # 0 / unset means "follow max_w", which is the recommended + # value. Read the note in control.py before raising it above + # max_w: every watt above the rail is unwind latency. + integrator_max_w=(float(opts["integrator_max_w"]) + if opts.get("integrator_max_w") else None), ) self.maint = Maintenance( MaintConfig( diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index e50a5fc..f27b673 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -48,7 +48,7 @@ options: step_w: 10 saturation_w: 500 saturation_cycles: 3 - integrator_max_w: 3000 + integrator_max_w: 0 heartbeat_s: 10 stale_input_s: 15 auto_start: false @@ -89,7 +89,7 @@ schema: step_w: int(1,100) saturation_w: int(100,2000) saturation_cycles: int(1,10) - integrator_max_w: int(100,15000) + integrator_max_w: int(0,15000) heartbeat_s: int(2,25) stale_input_s: int(5,120) auto_start: bool diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 3a85516..1cd1e07 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -78,7 +78,7 @@ print("SAFETY-04: the integrator is bounded apart from the output") # The historical runaway, with its real numbers. A commercial controller on # this site, with the inverter switched OFF, wound ~130 W every 4 s past 10 kW # and reported 14 768 W while its output clamp sat at 5 kW. At gain 0.6 that -# rate is a standing error of 130/0.6 ≈ 217 W that never resolves, because the +# rate is a standing error of 130/0.6 = 217 W that never resolves, because the # inverter is not there to resolve it. 150 cycles is past the ~113 it took to # reach 14 768 W at that rate. RUNAWAY_ERROR = 130.0 / 0.6 @@ -99,34 +99,53 @@ def runaway(tuning): return worst_i, worst_cmd -TR = Tuning(max_w=2000, integrator_max_w=3000) +TR = Tuning(max_w=2000) # integrator_max_w unset => follows max_w wi, wc = runaway(TR) -check(f"runaway: integrator plateaus at {wi:.0f} W (<= 3000)", wi <= TR.integrator_max_w) +check(f"runaway: integrator plateaus at {wi:.0f} W (<= 2000)", wi <= TR.max_w) check(f"runaway: emitted command peaks at {wc:.0f} W (<= 2000)", wc <= TR.max_w) check("runaway: nowhere near the historical 14 768 W", wc < HISTORICAL_W / 4) # ...and with the saturation detector deliberately defeated, so that only the -# clamp is holding. This is the AC that says the two mechanisms are -# independent: kill one, the other still bounds it. -TD = Tuning(max_w=2000, integrator_max_w=3000, saturation_w=1e9) +# clamp is holding. Kill one mechanism, the other still bounds it. +TD = Tuning(max_w=2000, saturation_w=1e9) wi, wc = runaway(TD) -check(f"runaway with the detector defeated: integrator still <= 3000 ({wi:.0f} W)", - wi <= TD.integrator_max_w) +check(f"runaway with the detector defeated: integrator still bounded ({wi:.0f} W)", + wi <= TD.max_w) check("runaway with the detector defeated: command still <= max_w", wc <= TD.max_w) -# The bound is not max_w. If someone "simplifies" them into one key this fails. +# The bound is a separate quantity, and the useful direction is BELOW max_w: +# there it binds first and caps unwind latency tighter than the rail does. d = compute(prev_w=0, grid_w=6000, actual_w=0, - tuning=Tuning(max_w=2000, integrator_max_w=3000, slew_w=5000)) -check("integrator bound is separate from the output clamp", - d.i_w == 3000 and d.target_w == 2000) + tuning=Tuning(max_w=2000, integrator_max_w=1000, slew_w=5000)) +check("integrator bound binds independently of the output clamp", + d.i_w == 1000 and d.target_w == 1000) -# Freeze = does not accumulate. Same input twice; the integrator must not move. +# Freeze = may not wind further in the direction it is already pushing. TF = Tuning(saturation_w=500, saturation_cycles=3) f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) -check("frozen: integration does not accumulate", f1.i_w == 2000.0 and f1.frozen) +check("frozen: integration does not wind further", f1.i_w == 2000.0 and f1.frozen) f2 = compute(prev_w=2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) check("frozen: unwinding is still allowed", f2.i_w < 2000.0) +# ⚠️ REGRESSION, and the reason the first cut of SAFETY-04 was rejected. A +# freeze encoded as "only corrections that shrink |i_w|" is unsatisfiable for +# BOTH signs of error whenever |correction| > 2*|i_w|, so near zero the loop +# stops moving forever - the freeze cannot clear, because clearing it needs the +# inverter to track and not-tracking is what saturation means. Measured on that +# encoding: 0 W held into a 2 kW import for as long as the sim ran. +z = compute(prev_w=0, grid_w=2000, actual_w=600, tuning=T, sat_count=3, i_w=0.0) +check("frozen at i_w=0: a 2 kW import still moves the command", + z.frozen and z.target_w == 1000) +# ...and the next cycle the inverter is inside saturation_w of the command, so +# the freeze clears on its own. Deadlock would show up here as frozen=True. +z2 = compute(prev_w=1000, grid_w=1000, actual_w=600, tuning=T, + sat_count=z.sat_count, i_w=z.i_w) +check("frozen at i_w=0: the freeze then clears", not z2.frozen) +# Same stranding on the other side: a small positive integrator against export. +z3 = compute(prev_w=100, grid_w=-1000, actual_w=800, tuning=T, sat_count=3, i_w=100.0) +check("frozen at i_w=+100: a 1 kW export still moves the command", + z3.frozen and z3.target_w < 0) + # False-positive guard: a normal 2 kW load step must not trip the detector, # because the plant needs several cycles to catch up on every one of them. prev, actual, sat, i_w, froze = 0.0, 0.0, 0, 0.0, False @@ -137,6 +156,74 @@ for _ in range(12): froze = froze or d.frozen check("a normal 2 kW load step does not trip the saturation freeze", not froze) +# The convergence sim below runs WITHOUT a carried integrator. This is the same +# 2 kW step in the configuration that actually ships, where main.py carries it. +prev, actual, sat, i_w = 0.0, 0.0, 0, 0.0 +carried = 0 +for _ in range(12): + d = compute(prev, 2000.0 - actual, actual, T, sat, i_w) + prev, sat, i_w = d.target_w, d.sat_count, d.i_w + actual = actual + 0.94 * (prev - actual) + carried += 1 + if abs(2000.0 - actual) < T.deadband_w: + break +check(f"carried integrator converges in {carried} cycles (<=6)", carried <= 6) +check("carried integrator does not overshoot the load", actual <= 2000.0 + T.deadband_w) + +# ⚠️ REGRESSION: an integrator allowed to wind past the rail buys nothing (the +# output clamp already bounds the wire) and costs extra cycles of +# wrong-direction power after every saturation event. 4000 W load held to +# saturation, then dropped to 0; the figure is the command on the first cycle +# after the drop. This is what makes the DOCS advice checkable. +def unwind(t): + prev, actual, sat, i_w, load = 0.0, 0.0, 0, 0.0, 4000.0 + for c in range(16): + if c == 15: + load = 0.0 + d = compute(prev, load - actual, actual, t, sat, i_w) + prev, sat, i_w = d.target_w, d.sat_count, d.i_w + actual = actual + 0.94 * (prev - actual) + return prev + + +tight, loose = unwind(Tuning(max_w=2000)), unwind(Tuning(max_w=2000, integrator_max_w=3000)) +check(f"after saturation ends the command is {tight:.0f} W (<= 1000)", tight <= 1000) +check(f"headroom above max_w makes that worse ({loose:.0f} W) - hence the default", + loose > tight) + +print("SAFETY-04: the i_w=None path is still release/1.0, exactly") + + +def legacy(prev, grid, actual, t, sat_count): + """release/1.0's control law, transcribed. Do not 'improve' this.""" + sc = min(sat_count + 1, 10) if abs(prev - actual) > t.saturation_w else 0 + frozen = sc >= t.saturation_cycles + error = grid - t.target_grid_w + want = prev if abs(error) < t.deadband_w else prev + t.gain * error + target = max(-t.max_w, min(t.max_w, want)) + target = max(prev - t.slew_w, min(prev + t.slew_w, target)) + if frozen: + target = min(target, prev) if prev > 0 else max(target, prev) + step = max(1, int(t.step_w)) + return float(round(target / step) * step), sc + + +# Exhaustive over the interesting corners, both freeze states, both signs, and +# either side of the deadband. This is what makes the claim in control.py's +# integrator comment a checked fact rather than an assertion. +diffs = [] +for tune in (Tuning(), Tuning(target_grid_w=-10.0), Tuning(max_w=5000, slew_w=5000)): + for prev in (-2000.0, -500.0, -100.0, 0.0, 100.0, 500.0, 2000.0): + for grid in (-6000.0, -1000.0, -500.0, -14.0, 0.0, 14.0, 500.0, 1000.0, 6000.0): + for actual in (-2000.0, 0.0, 600.0, 2000.0): + for sc in (0, 2, 3, 9): + d = compute(prev, grid, actual, tune, sc) # i_w defaults to None + lt, lsc = legacy(prev, grid, actual, tune, sc) + if (d.target_w, d.sat_count) != (lt, lsc): + diffs.append((prev, grid, actual, sc, d.target_w, lt)) +check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases" + + (f" (first diff {diffs[0]})" if diffs else ""), not diffs) + print("capacity tariff") check("no forecast means no cap", maintenance_charge_floor(2500, None, 3500) == 2500) check("headroom caps the charge", maintenance_charge_floor(2500, 2000, 3500) == 1500) From 7123aa00a496b9a7fcb524c308e40df0da60b1f4 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 22:29:10 +0200 Subject: [PATCH 3/6] SAFETY-04: revive the clamp reason, and compare reasons in the sweep `want = i_w` after the integrator bound, so at the default limit == max_w the output clamp can never fire and `reason == "clamped"` had become unreachable. Observability only today - nothing gates on the string - but SAFETY-03 exists to alarm on exactly that engagement, so its hook was dead before it was built. The integrator bound now reports "i-clamped", and that is the signal SAFETY-03 must watch: it is the one that fires on a default install. "clamped" stays reachable for a configuration that lets the integrator run above the rail, where both fire and the output clamp - which describes the value actually emitted - is the one reported. Two names because the two events want different alarms: the loop winding, versus a command that came out over the rating. The real fix is the second half. The equivalence sweep compared (target_w, sat_count), which is how a dead reason survived 3024 cases. It now compares (target_w, sat_count, frozen, reason) and it catches this defect: dropping the emit turns it red. Deliberate rename aliased explicitly, so any OTHER reason divergence still fails. Result of adding reason to the tuple: 105 of 3024 cases differ, and every one of them is the i-clamped/clamped rename. Zero value divergences, `frozen` included. Nothing else surfaced. test_control.py: 41 -> 43 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/app/control.py | 13 +++++++- goodwe_controller/test_control.py | 53 +++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 9dbcae6..0b7b29f 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -164,7 +164,18 @@ def compute( # ⚠️ Applied EVERY cycle, frozen or not: the freeze is conditional, this # bound is not. It is what makes the worst-case unwind time finite and # knowable instead of a function of how long the error happened to stand. - i_w = max(-limit, min(limit, i_w)) + bounded = max(-limit, min(limit, i_w)) + if bounded != i_w: + # ⚠️ SAFETY-03 (alarm whenever the loop winds into a rail) must watch + # for THIS, not for "clamped" below. At the default limit == max_w the + # integrator bound is reached first and the command derived from it can + # then never exceed max_w, so "clamped" is unreachable on a default + # install - it survives only for a configuration that deliberately lets + # the integrator run above the rail. Two reasons rather than one + # because the two events want different alarms: "i-clamped" is the loop + # winding, "clamped" is a command that came out over the rating anyway. + reason = "i-clamped" + i_w = bounded want = i_w # ⚠️ Maintenance shaping (charge-only, cheap-window floor) used to live diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 1cd1e07..5bcff99 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -196,34 +196,67 @@ print("SAFETY-04: the i_w=None path is still release/1.0, exactly") def legacy(prev, grid, actual, t, sat_count): """release/1.0's control law, transcribed. Do not 'improve' this.""" + reason = "tracking" sc = min(sat_count + 1, 10) if abs(prev - actual) > t.saturation_w else 0 frozen = sc >= t.saturation_cycles error = grid - t.target_grid_w - want = prev if abs(error) < t.deadband_w else prev + t.gain * error + if abs(error) < t.deadband_w: + want, reason = prev, "deadband" + else: + want = prev + t.gain * error target = max(-t.max_w, min(t.max_w, want)) - target = max(prev - t.slew_w, min(prev + t.slew_w, target)) + if target != want: + reason = "clamped" + slewed = max(prev - t.slew_w, min(prev + t.slew_w, target)) + if slewed != target: + reason = "slew-limited" + target = slewed if frozen: target = min(target, prev) if prev > 0 else max(target, prev) + reason = "saturated-freeze" step = max(1, int(t.step_w)) - return float(round(target / step) * step), sc + return float(round(target / step) * step), sc, frozen, reason -# Exhaustive over the interesting corners, both freeze states, both signs, and -# either side of the deadband. This is what makes the claim in control.py's -# integrator comment a checked fact rather than an assertion. +# ⚠️ Compare EVERYTHING observable, not just the number. A previous version of +# this sweep compared (target_w, sat_count) only and passed 3024 cases while +# `reason` had silently lost a value - which is the kind of thing a sweep this +# broad exists to catch. `frozen` and `reason` are both in the tuple now. +# +# The one deliberate rename: what release/1.0 called "clamped" is now +# "i-clamped", because the truncation happens on the integrator before the +# command is derived from it. Aliased here rather than papered over - if any +# OTHER reason ever diverges, this check goes red. +ALIAS = {"i-clamped": "clamped"} diffs = [] +seen = set() for tune in (Tuning(), Tuning(target_grid_w=-10.0), Tuning(max_w=5000, slew_w=5000)): for prev in (-2000.0, -500.0, -100.0, 0.0, 100.0, 500.0, 2000.0): for grid in (-6000.0, -1000.0, -500.0, -14.0, 0.0, 14.0, 500.0, 1000.0, 6000.0): for actual in (-2000.0, 0.0, 600.0, 2000.0): for sc in (0, 2, 3, 9): d = compute(prev, grid, actual, tune, sc) # i_w defaults to None - lt, lsc = legacy(prev, grid, actual, tune, sc) - if (d.target_w, d.sat_count) != (lt, lsc): - diffs.append((prev, grid, actual, sc, d.target_w, lt)) -check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases" + seen.add(d.reason) + got = (d.target_w, d.sat_count, d.frozen, + ALIAS.get(d.reason, d.reason)) + if got != legacy(prev, grid, actual, tune, sc): + diffs.append((prev, grid, actual, sc, got, + legacy(prev, grid, actual, tune, sc))) +check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases, reason included" + (f" (first diff {diffs[0]})" if diffs else ""), not diffs) +# ...and the rename is not a quiet deletion: the signal SAFETY-03 alarms on has +# to actually occur in that sweep, or its hook is dead. +check("the integrator clamp reports itself as 'i-clamped'", "i-clamped" in seen) + +# "clamped" stays reachable, but only where the integrator is deliberately +# allowed above the rail - then BOTH fire and the output clamp, which describes +# the value actually emitted, is the one reported. +dc = compute(prev_w=0, grid_w=6000, actual_w=0, + tuning=Tuning(max_w=2000, integrator_max_w=3000, slew_w=5000)) +check("the output clamp still reports 'clamped' when it is the binding one", + dc.reason == "clamped" and dc.i_w == 3000 and dc.target_w == 2000) + print("capacity tariff") check("no forecast means no cap", maintenance_charge_floor(2500, None, 3500) == 2500) check("headroom caps the charge", maintenance_charge_floor(2500, 2000, 3500) == 1500) From 53d301b92026c989a40fe1070c390b35a38d1b9b Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 22:49:01 +0200 Subject: [PATCH 4/6] SAFETY-04: exactly zero is its own case in the freeze tie-break `min(moved, i_w) if i_w > 0 else max(moved, i_w)` files i_w == 0.0 under rising-only, so the first push toward charging from exactly zero was blocked permanently - the S-1 deadlock again, mirrored in sign. main.py resets i_w to exactly 0.0 on every stop and every reseed, so it is a normal state. Zero is now handled explicitly and both directions are allowed: nothing is wound, so "may not wind further" has no referent, and a first step from zero is bounded by the gain, the output clamp and the slew limit like any other. Measured before the fix, at i_w == 0.0 and frozen: 12 800 of 25 920 ticks held the integrator and 8 304 of those changed the emitted command, worst case abandoning a 2 kW charge into a 4 kW export. Note this is NOT the same as the reported symptom: at prev_w == 0 the command holds at 0 W either way, because the output freeze forbids starting a charge while saturated, and that rule is release/1.0's and unchanged. There is now a test asserting it deliberately. Tests. The durable part is a property rather than more points: over 13 041 frozen states the integrator may be held ONLY by a correction pushing it further from zero on the side it already sits, and any other hold fails. Both signs at exactly 0.0. Mirrors added everywhere the suite tested one direction of two - freeze wind/unwind while charging, i_w=-100, the export-direction runaway, the negative clamp and slew. DOCS: the cycles-vs-seconds deviation is now written down as a deviation - the "> 10 s" criterion is not met as literally written, a cycle is one CHANGED meter reading, and there is no guaranteed wall-clock window. test_control.py: 43 -> 55 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/DOCS.md | 25 +++++++++++- goodwe_controller/app/control.py | 23 ++++++++++- goodwe_controller/test_control.py | 65 ++++++++++++++++++++++++++++++- 3 files changed, 108 insertions(+), 5 deletions(-) diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 15961b6..9e0de76 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -59,12 +59,35 @@ phase having charged nothing. | `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. **Do not set to 1** | +| `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** | | `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 | | `stale_input_s` | 15 | How long inputs may be missing before commanding 0 W | | `auto_start` | false | Start controlling on boot (only after commissioning) | +#### Saturation is counted in cycles, not seconds + +The specification states the saturation window as **"> 10 s"**. This add-on counts +**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. + +Two reasons that is acceptable: + +- 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. + +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". + #### Why `target_grid_w` is not zero The deadband is a one-way ratchet: any resting point inside it holds until diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 0b7b29f..8c00dca 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -156,10 +156,29 @@ def compute( # has been in service on real hardware. It is applied here as well # because the requirement is that the INTEGRATOR stop accumulating, not # only the command. - if not frozen: + # + # ⚠️ EXACTLY ZERO IS ITS OWN CASE, and it must be handled explicitly + # rather than falling into one of the two branches. "May not wind + # further in the direction it is already pushing" has no referent at + # zero: nothing is wound, and neither direction is "further". Writing + # this as `if i_w > 0 ... else ...` silently files zero under + # rising-only and permanently blocks the first push toward charging - + # the same deadlock as the shrink-only encoding above, mirrored in sign, + # and reachable because main.py resets i_w to exactly 0.0 on every stop + # and every reseed. Measured before the fix: 12 800 of 25 920 frozen + # ticks at i_w == 0.0 held the integrator, 8 304 of them changing the + # emitted command, worst case abandoning a 2 kW charge into a 4 kW + # export. + # + # Freezing at zero would also be pointless: the freeze exists to stop + # accumulation running away, and a first step from zero is bounded by + # the gain, the output clamp and the slew limit like any other. + if not frozen or i_w == 0.0: i_w = moved + elif i_w > 0: + i_w = min(moved, i_w) else: - i_w = min(moved, i_w) if i_w > 0 else max(moved, i_w) + i_w = max(moved, i_w) # ⚠️ Applied EVERY cycle, frozen or not: the freeze is conditional, this # bound is not. It is what makes the worst-case unwind time finite and diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 5bcff99..3436d8f 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -48,6 +48,11 @@ check("clamped to max_w", d.target_w == 2000) # Slew: from 0 with a huge error, no more than slew_w in one cycle. d = compute(prev_w=0, grid_w=5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) check("slew limits one cycle", d.target_w == 1000) +d = compute(prev_w=-1900, grid_w=-1000, actual_w=-1900, + tuning=Tuning(max_w=2000, slew_w=5000)) +check("clamped to -max_w", d.target_w == -2000) +d = compute(prev_w=0, grid_w=-5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) +check("slew limits one cycle, charging", d.target_w == -1000) # Saturation needs DURATION: one diverging cycle must NOT freeze. t = Tuning(saturation_w=500, saturation_cycles=3) @@ -86,12 +91,12 @@ RUNAWAY_CYCLES = 150 HISTORICAL_W = 14768.0 -def runaway(tuning): +def runaway(tuning, sign=1): """Inverter off: it reports 0 W forever, the error never clears.""" prev, i_w, sat = 0.0, 0.0, 0 worst_i, worst_cmd = 0.0, 0.0 for _ in range(RUNAWAY_CYCLES): - d = compute(prev_w=prev, grid_w=RUNAWAY_ERROR, actual_w=0.0, + d = compute(prev_w=prev, grid_w=sign * RUNAWAY_ERROR, actual_w=0.0, tuning=tuning, sat_count=sat, i_w=i_w) prev, i_w, sat = d.target_w, d.i_w, d.sat_count worst_i = max(worst_i, abs(i_w)) @@ -113,6 +118,12 @@ check(f"runaway with the detector defeated: integrator still bounded ({wi:.0f} W wi <= TD.max_w) check("runaway with the detector defeated: command still <= max_w", wc <= TD.max_w) +# The mirror: the same runaway driving the other way. An export that never +# clears winds the integrator negative just as hard. +wi, wc = runaway(Tuning(max_w=2000), sign=-1) +check(f"runaway (export direction): integrator bounded at {wi:.0f} W", wi <= 2000) +check("runaway (export direction): emitted command <= max_w", wc <= 2000) + # The bound is a separate quantity, and the useful direction is BELOW max_w: # there it binds first and caps unwind latency tighter than the rail does. d = compute(prev_w=0, grid_w=6000, actual_w=0, @@ -126,6 +137,12 @@ f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=20 check("frozen: integration does not wind further", f1.i_w == 2000.0 and f1.frozen) f2 = compute(prev_w=2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) check("frozen: unwinding is still allowed", f2.i_w < 2000.0) +# ...and the same two on the charging side. Every freeze rule in this file has +# a mirror, because the one that did not is the defect that got through review. +f3 = compute(prev_w=-2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=-2000.0) +check("frozen (charging): integration does not wind further", f3.i_w == -2000.0) +f4 = compute(prev_w=-2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=-2000.0) +check("frozen (charging): unwinding is still allowed", f4.i_w > -2000.0) # ⚠️ REGRESSION, and the reason the first cut of SAFETY-04 was rejected. A # freeze encoded as "only corrections that shrink |i_w|" is unsatisfiable for @@ -145,6 +162,50 @@ check("frozen at i_w=0: the freeze then clears", not z2.frozen) z3 = compute(prev_w=100, grid_w=-1000, actual_w=800, tuning=T, sat_count=3, i_w=100.0) check("frozen at i_w=+100: a 1 kW export still moves the command", z3.frozen and z3.target_w < 0) +z4 = compute(prev_w=-100, grid_w=1000, actual_w=-800, tuning=T, sat_count=3, i_w=-100.0) +check("frozen at i_w=-100: a 1 kW import still moves the command", + z4.frozen and z4.target_w > 0) + +# ⚠️ EXACTLY ZERO, BOTH DIRECTIONS. This boundary has a history: the first cut +# deadlocked here under import, and the fix for it deadlocked here under export +# because `if i_w > 0 ... else ...` files 0.0 under rising-only. main.py resets +# i_w to exactly 0.0 on every stop and every reseed, so it is a normal state, +# not a corner. +zi = compute(prev_w=0, grid_w=2000, actual_w=600, tuning=T, sat_count=3, i_w=0.0) +check("frozen at i_w=0.0: an import push moves the integrator", + zi.frozen and zi.i_w > 0) +ze = compute(prev_w=0, grid_w=-2000, actual_w=-600, tuning=T, sat_count=3, i_w=0.0) +check("frozen at i_w=0.0: an export push moves the integrator", + ze.frozen and ze.i_w < 0) +# The COMMAND still holds at 0 W in that second case, and that is release/1.0's +# rule, not a leftover: at prev_w == 0 the output freeze forbids starting to +# charge while saturated, because commanding 0 while the inverter reports +# hundreds of watts means something else is driving the bus. Asserted so that +# nobody "fixes" it by accident - the integrator moving is what this ticket +# owns, the command rule belongs to the output freeze. +check("frozen at i_w=0.0: the output freeze still blocks a charge from 0 W", + ze.target_w == 0.0) +# Where prev_w is already charging the output freeze does NOT block, and there +# the difference reaches the wire: held at 0.0 the integrator abandons the +# charge mid-export. +zc = compute(prev_w=-2000, grid_w=-4000, actual_w=-600, tuning=T, sat_count=3, i_w=0.0) +check("frozen at i_w=0.0: a charge is not abandoned during heavy export", + zc.target_w == -2000.0) + +# The general property, rather than another handful of points: while frozen the +# integrator may be held ONLY when the correction would push it further from +# zero on the side it already sits. Any other hold is a deadlock. +stuck = [] +for i0 in [x * 25.0 for x in range(-80, 81)]: + for g in [x * 100.0 for x in range(-40, 41)]: + err = g - T.target_grid_w + if abs(err) < T.deadband_w: + continue + dd = compute(prev_w=0.0, grid_w=g, actual_w=1500.0, tuning=T, sat_count=3, i_w=i0) + if dd.i_w == i0 and not ((i0 > 0 and err > 0) or (i0 < 0 and err < 0)): + stuck.append((i0, g)) +check(f"frozen integrator never deadlocks, over {161*81} states" + + (f" (e.g. {stuck[0]})" if stuck else ""), not stuck) # False-positive guard: a normal 2 kW load step must not trip the detector, # because the plant needs several cycles to catch up on every one of them. From 389d9ecd6dfaf31d0cf458d45b9ce39b52d21f5b Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 22:59:29 +0200 Subject: [PATCH 5/6] SAFETY-04: stop the clamps standing in for the mechanisms under test AC 3 - "integration freezes while saturated" - had no non-vacuous test. Deleting the integrator freeze outright failed 0 of 55 checks: the two checks that name it used a fixture at max_w 2000 with the integrator bound following it, so a wound value was truncated back to exactly 2000 and the assertion passed on the clamp instead. Fixture lifted to max_w 5000, clear of every rail. Deleting the freeze now fails 2. Then swept the whole function for the same pattern, one mechanism at a time: delete it, count which checks notice. It found a second instance - the OUTPUT clamp. `clamped to max_w` and `clamped to -max_w` were both satisfied by the integrator bound truncating first, so removing the output clamp failed only the reason-string check. Those two fixtures now set integrator_max_w above max_w so the mechanism they name is the binding one; the output clamp goes from 1 failure to 3. Every mechanism in compute() is now caught by a check that names it: freeze 2, integrator clamp 6, bound-follows-max_w 4, output clamp 3, slew 4, output freeze 2, deadband 5, quantise 2, detector 10, duration 2, counter reset 6, grid bias 3, None-seeding 6. No mechanism at zero. Method note: the audit disables bytecode caching. Rewriting control.py inside one second leaves a stale app/__pycache__ entry and silently under-reports - it under-reported one mutation as 2 failures where the true figure is 6. test_control.py stays at 55 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/test_control.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 3436d8f..ec1943b 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -41,15 +41,19 @@ check("proportional step (gain 0.6)", d.target_w == 300) d = compute(prev_w=0, grid_w=-500, actual_w=0, tuning=T) check("export drives charging", d.target_w == -300) -# Clamp -d = compute(prev_w=1900, grid_w=1000, actual_w=1900, tuning=Tuning(max_w=2000, slew_w=5000)) +# Clamp. +# ⚠️ integrator_max_w is lifted clear of max_w so that the OUTPUT clamp is the +# mechanism under test. Left at the default the integrator bound truncates +# first, these two assertions pass on that alone, and deleting the output clamp +# fails nothing - the same masking that hid the integrator freeze. +TCLAMP = Tuning(max_w=2000, slew_w=5000, integrator_max_w=5000) +d = compute(prev_w=1900, grid_w=1000, actual_w=1900, tuning=TCLAMP) check("clamped to max_w", d.target_w == 2000) # Slew: from 0 with a huge error, no more than slew_w in one cycle. d = compute(prev_w=0, grid_w=5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) check("slew limits one cycle", d.target_w == 1000) -d = compute(prev_w=-1900, grid_w=-1000, actual_w=-1900, - tuning=Tuning(max_w=2000, slew_w=5000)) +d = compute(prev_w=-1900, grid_w=-1000, actual_w=-1900, tuning=TCLAMP) check("clamped to -max_w", d.target_w == -2000) d = compute(prev_w=0, grid_w=-5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) check("slew limits one cycle, charging", d.target_w == -1000) @@ -132,7 +136,12 @@ check("integrator bound binds independently of the output clamp", d.i_w == 1000 and d.target_w == 1000) # Freeze = may not wind further in the direction it is already pushing. -TF = Tuning(saturation_w=500, saturation_cycles=3) +# ⚠️ max_w is raised WELL above the fixtures on purpose. At the default 2000 +# the integrator bound truncates a wound value back to exactly 2000 and +# satisfies these assertions on its own, so deleting the freeze outright +# failed nothing - the clamp was standing in for the mechanism under test. +# Any fixture here must sit clear of every rail, or it tests the rail. +TF = Tuning(saturation_w=500, saturation_cycles=3, max_w=5000) f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) check("frozen: integration does not wind further", f1.i_w == 2000.0 and f1.frozen) f2 = compute(prev_w=2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0) From 680461c9bfc408970f6ce3bb2fc57844907418ef Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 23:07:58 +0200 Subject: [PATCH 6/6] SAFETY-04: record the coverage audit as a comment, with its invariant The audit found a dead mechanism twice, both times a clamp standing in for the mechanism under test, so the technique has to survive this ticket. Not as a script: the only cheap way to automate it is to key on source lines, that goes stale silently, and a green audit that has quietly stopped testing anything is this ticket's own failure mode one level up. Automating it properly would mean decomposing compute() to make its statements separately addressable, which is a refactor of the most safety-critical function in the repo for the benefit of test tooling. So it goes in as a comment block next to the checks it describes, carrying the commit it was measured at, the thirteen figures, and the invariant with the teeth in it: every mechanism must be noticed by at least two checks when it is deleted, and adding a mechanism means re-running the audit. A comment cannot go stale-green, because it never claims to be running. Also recorded: fixtures must sit clear of every rail they are not testing, which is the rule both misses violated; and the `python -B` / clear-pycache discipline, with the reason (CPython invalidates on source mtime-in-seconds plus size, so a same-second same-size rewrite reuses stale bytecode) and the reason it casts no doubt on the figures (the error is one-directional, so every number is a lower bound). Figures are the lead's independent reproduction. I re-measured the one that differed: the detector is 11 for `saturated_now = False` and 10 for the weaker `frozen = False` form, so the table names the form. test_control.py stays at 55 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/test_control.py | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index ec1943b..9dd7458 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -24,6 +24,63 @@ def check(name, cond): fails.append(name) +# --------------------------------------------------------------------------- +# COVERAGE AUDIT - measured, not executed. Read this before adding a mechanism. +# +# THE INVARIANT: every mechanism in compute() must be noticed by AT LEAST TWO +# checks when it is deleted. If you add a mechanism to compute(), re-run the +# audit and add it to the table. If a figure here drops, a check has started +# passing for a reason other than the one it names. +# +# THE TECHNIQUE, because there is no script to run: replace one mechanism in +# control.py with a no-op, run this file, count the failures, restore. That is +# the converse of the usual mutation - not "does a wrong value fail?" but "does +# anyone notice when the mechanism is GONE?". It is kept as a comment rather +# than as tooling on purpose: the only cheap way to automate it is to key on +# source lines, which goes stale silently, and a green audit that has quietly +# stopped testing anything is precisely the failure this ticket exists to fix. +# A comment cannot go stale-green, because it never claims to be running. +# +# Measured at 389d9ec. Numbers are the lead's independent reproduction. +# +# mechanism in compute() checks that fail when deleted +# ------------------------------------------ ----------------------------- +# integrator freeze (AC 3) 2 +# integrator clamp (AC 1) 6 +# integrator bound follows max_w 4 +# output clamp 3 +# slew limit 4 +# output freeze 2 +# deadband 5 +# quantisation 2 +# saturation detector, `saturated_now = False` 11 +# saturation duration (AC 2), fires instantly 2 +# sat counter reset on a good cycle 6 +# target_grid_w bias 3 +# i_w=None seeding from prev_w 6 +# +# The detector figure is for the `saturated_now = False` form specifically; +# disabling it further down as `frozen = False` is a weaker mutation and gives +# 10. Reproduce the same form or the number will not match. +# +# ⚠️ IT HAS FOUND A DEAD MECHANISM TWICE, BOTH THE SAME WAY: a clamp standing in +# for the mechanism under test. Deleting the integrator freeze once failed +# NOTHING, because the fixtures sat at max_w 2000 and the integrator bound +# truncated a wound value back to exactly 2000 - the assertion passed on the +# clamp. The output clamp was masked the same way by the integrator bound. +# Hence: A FIXTURE MUST SIT CLEAR OF EVERY RAIL IT IS NOT TESTING. Where a test +# names one mechanism, make that mechanism the binding one (see TCLAMP and TF). +# +# ⚠️ RUN MUTATIONS WITH `python -B` AND CLEAR app/__pycache__. CPython +# invalidates a .pyc on (source mtime in whole seconds, source size), so a +# same-second rewrite that also preserves the file size reuses stale bytecode +# and the suite reports on code you are no longer running. It under-reported one +# mutation here as 2 where the true figure is 6. The error is one-directional - +# stale bytecode can only under-report - so every figure above is a lower bound +# at worst, and the two zeros ever recorded were both confirmed by fixing them +# and watching the count rise, which a caching artefact cannot do. +# --------------------------------------------------------------------------- + print("control law") # Deadband: inside meter noise, hold exactly - do not drift.