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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa

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

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(

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:

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

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)