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

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

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(