diff --git a/components/pid/TODO.md b/components/pid/TODO.md index 064bf30..1041177 100644 --- a/components/pid/TODO.md +++ b/components/pid/TODO.md @@ -99,6 +99,25 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). cases) was added under `tests/components/pid/` — see the "No automated tests" item above. +- [x] **HOLD overshoot was a permanent steady-state offset, not just a + transient windup.** Found testing the rework against a real Sud run + (`sude/sud_0030.json`): a grain-fill-in disturbance during a `HOLD` + overshot by ~0.4°C (under the `HoldCool` threshold, so the FSM stayed in + `HOLD`) and then never converged back — `temp_ist` sat in a 55.3-55.4°C + band for the rest of the 20-minute hold instead of returning to 55.0. + Cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` clamped to + exactly `0.0`, so once overshot, `heatrate_soll` = 0 ("hold flat") and + `pid_inner` actively fought the pot's own ambient loss to keep the + overshot temperature flat instead of declining back to setpoint. Fixed: + the floor is now a configurable `Outer.y_hold_min` (default `0.0`, + backward compatible), set to `-0.1` in `config.json`/both `.tpl` + templates, letting HOLD request a gentle decline that roughly matches + passive ambient cooling without reopening the `bb5af3c` limit cycle + (fully unclamped negative). See the "Follow-up" section in + `docs/overshoot_hold_windup.md` and + `TestHoldOvershootRecoversToSetpoint` in + `tests/components/pid/test_temp_controller_closed_loop.py`. + - [ ] **`kalman.py` is now dead code in production.** Neither `temp_controller.py` nor `temp_controller_smith.py` uses `Kalman` anymore; the only remaining references are diff --git a/components/pid/temp_controller_base.py b/components/pid/temp_controller_base.py index 9693b02..6d0cc83 100644 --- a/components/pid/temp_controller_base.py +++ b/components/pid/temp_controller_base.py @@ -21,6 +21,7 @@ class TempControllerBase(TempControllerFsm, APid): self.params = None self._inner_heat_params = None self._inner_hold_params = None + self._outer_hold_y_min = 0.0 self.y = -1 # Heat-rate pre-filter state (option A) - None until first process() tick self.last_theta_ist = None @@ -32,6 +33,11 @@ class TempControllerBase(TempControllerFsm, APid): self.thresholds = {**DEFAULT_THRESHOLDS, **params.get('Thresholds', {})} self.beta = params.get('beta', 0.05) self.pid_outer.set_params(params['Outer']) + # How far into "actively cool" pid_outer is allowed to reach during + # HOLD - see the floor in process_pid() below. Defaults to 0.0 + # (old flat-clamp behaviour) so configs that don't set it are + # unaffected. + self._outer_hold_y_min = params['Outer'].get('y_hold_min', 0.0) self._inner_heat_params = params['Inner']['Heat'] self._inner_hold_params = params['Inner']['Hold'] self.pid_inner_cool.set_params(params['Inner']['Cool']) @@ -135,14 +141,21 @@ class TempControllerBase(TempControllerFsm, APid): def process_pid(self, theta_err, hold_scale=1.0): self.pid_outer.process(theta_err, -self.theta_ist, hold_scale) - # In HOLD state, clamp pid_outer's output to [0, 1]: a small temperature - # overshoot makes pid_outer.y go negative, which would invert heatrate_soll - # and drive pid_inner's power to 0, causing a limit cycle (power on → - # overshoot → power off → coast down → repeat). Clamping to 0 lets the - # outer loop reduce the inner setpoint to zero but no further. + # In HOLD state, floor pid_outer's output at Outer.y_hold_min (<= 0) + # rather than letting it swing all the way to -1: a large negative + # heatrate_soll asks for a decline far steeper than the pot's own + # ambient heat loss can deliver, so pid_inner pins power at 0 for an + # extended stretch, undershoots, and swings back hard - a ~110s + # limit cycle (see git history for this clamp). A small negative + # floor instead lets HOLD ask for a gentle decline that roughly + # matches passive cooling, so a small overshoot coasts back down to + # setpoint instead of sitting in a permanent flat-clamp dead zone + # (the outer loop otherwise commands "stay flat" forever, and + # pid_inner actively fights the pot's own ambient loss to hold that + # flat line - see docs/overshoot_hold_windup.md). pid_outer_y = self.pid_outer.get_y() if self.state == States.HOLD: - pid_outer_y = max(0.0, pid_outer_y) + pid_outer_y = max(self._outer_hold_y_min, pid_outer_y) self.heatrate_soll = self.heatrate_soll_set * pid_outer_y heatrate_err = self.heatrate_soll - self.heatrate_ist diff --git a/config-real.json.tpl b/config-real.json.tpl index 2cf1812..b6ed3e9 100644 --- a/config-real.json.tpl +++ b/config-real.json.tpl @@ -8,7 +8,8 @@ "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, "Inner": { "Heat": { diff --git a/config-sim.json.tpl b/config-sim.json.tpl index ede94e6..c911a7c 100644 --- a/config-sim.json.tpl +++ b/config-sim.json.tpl @@ -8,7 +8,8 @@ "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, "Inner": { "Heat": { diff --git a/docs/overshoot2.png b/docs/overshoot2.png new file mode 100644 index 0000000..6bafb7b Binary files /dev/null and b/docs/overshoot2.png differ diff --git a/docs/overshoot_hold_windup.md b/docs/overshoot_hold_windup.md index 28f47be..9dea9cf 100644 --- a/docs/overshoot_hold_windup.md +++ b/docs/overshoot_hold_windup.md @@ -247,6 +247,60 @@ small (~0.07 in testing, well below the pre-fix disturbance's ~0.74 peak) step in `y` rather than the fully bumpless transfer described above. Not addressed here — flagged for awareness, not a blocker. +## Follow-up: steady-state overshoot in HOLD (`Outer.y_hold_min`) + +Found while testing the rework against a real Sud run (`sude/sud_0030.json`, +`logs/log_20260706T074658_Sud-0030.json`). Visible in `docs/overshoot2.png` +(`theta_ist` vs `theta_soll` across the run): at both the 55°C and 63°C +rests, `theta_ist` overshoots the step and then plateaus above `theta_soll` +for the rest of the hold instead of converging back down — most clearly at +the first rest, where it settles at ~55.5-55.6°C against a 55.0°C target. + +A grain-fill-in disturbance during "1. Rast" (mash-in rest, `HOLD` at 55°C) +pushed `temp_ist` to a ~0.4°C overshoot — well under `HoldCool`'s 1.0 +threshold, so the FSM stayed in `HOLD` throughout, same as the transient +windup case above. But this overshoot never decayed: `temp_ist` sat in a +55.30-55.44 band for the rest of the 20-minute hold instead of converging +back to 55.0. Distinct failure mode from the transient windup fixed above +(that one unwound over ~35s; this one was flat/permanent for as long as the +hold lasted). + +Root cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` (added in +`bb5af3c` to break a limit cycle - see the History section) clamped to +exactly `0.0`. Once `temp_ist > temp_soll`, that floor forces +`heatrate_soll = 0`, i.e. "hold flat" - and `pid_inner` then actively fights +the pot's own ambient heat loss to keep the *overshot* temperature flat, +rather than being allowed to request a genuine decline back toward +setpoint. With `Outer.ki = 0`, there's no integral action to null the +resulting steady-state error any other way, so the offset persists for the +rest of the hold. + +Fix: replaced the hardcoded `0.0` floor with a configurable +`Outer.y_hold_min` (default `0.0`, so configs that don't set it keep the old +behavior), set to `-0.1` in `config.json`, both `.tpl` templates, and the +three demo scripts. A small negative floor lets HOLD ask for a gentle +decline that roughly matches +passive ambient cooling, rather than the fully unclamped `[-1, 1]` range +that caused the original limit cycle (a large negative `heatrate_soll` asks +for a decline steeper than passive loss can deliver, pinning power at 0 for +an extended stretch and producing a hard undershoot/rebound). Test coverage +added: `TestHoldOvershootRecoversToSetpoint` in +`tests/components/pid/test_temp_controller_closed_loop.py` reproduces the +sud_0030 disturbance, asserts the old flat-clamp behavior still fails to +recover (regression guard) and the new floor converges close to setpoint, +plus a guard that the recovery doesn't undershoot by more than the injected +disturbance itself (i.e. doesn't reintroduce the `bb5af3c` limit cycle). + +`-0.1` was chosen from the real Sud's plant params (`Pot.mass=5.96` + +`water_mass=22` ≈ the test harness's `M=27.96`, `L=0.2`): passive ambient +loss at a ~35°C delta works out to roughly 0.1-0.15 K/min, so +`heatrate_soll_set * -0.1` lands in that same ballpark for a typical +`heatrate_soll_set` of ~1.0-1.5 K/min. Not derived from first-principles +tuning - may need adjustment per installation, same as the other PID gains. + +Confirmed against a live re-run of `sud_0030` with the fix applied, not just +the unit test above. + ## Architecture diagram A full signal-flow diagram of the cascade (`Outer`/`Inner.*` PIDs, FSM diff --git a/scripts/demos/pid/demo_temp_controller.py b/scripts/demos/pid/demo_temp_controller.py index 0c43dc5..f055828 100644 --- a/scripts/demos/pid/demo_temp_controller.py +++ b/scripts/demos/pid/demo_temp_controller.py @@ -10,7 +10,8 @@ if __name__ == '__main__': "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, "Inner": { "Heat": { diff --git a/scripts/demos/pid/demo_temp_controller_smith.py b/scripts/demos/pid/demo_temp_controller_smith.py index 71078fb..0813ddf 100644 --- a/scripts/demos/pid/demo_temp_controller_smith.py +++ b/scripts/demos/pid/demo_temp_controller_smith.py @@ -10,7 +10,8 @@ if __name__ == '__main__': "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, "Inner": { "Heat": { diff --git a/scripts/demos/sud/demo_sud.py b/scripts/demos/sud/demo_sud.py index e28c3b7..444cc78 100644 --- a/scripts/demos/sud/demo_sud.py +++ b/scripts/demos/sud/demo_sud.py @@ -21,7 +21,8 @@ if __name__ == '__main__': "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, "Inner": { "Heat": { diff --git a/sude/sud_0030.json b/sude/sud_0030.json new file mode 100644 index 0000000..d88a226 --- /dev/null +++ b/sude/sud_0030.json @@ -0,0 +1,119 @@ +{ + "Name": "Sud-0030", + "Description": "Münchner Hell", + "pot": { + "grain_mass": 0, + "water_mass": 22, + "volumen": 30 + }, + "default": { + "step": { + "descr": "Put description here", + "user_message": "Put user message here", + "user_wait_for_continue": false, + "pot": { + "grain_mass": 5.21, + "water_mass": 22 + }, + "temperature": 0, + "ramp": { + "rate": 1.0, + "stirrer": { + "speed": 50, + "interval_time": 0, + "on_ratio": 1.0 + } + }, + "hold": { + "duration": 0, + "stirrer": { + "speed": 30, + "interval_time": 60, + "on_ratio": 0.5 + } + } + } + }, + "steps": [ + { + "pot": { + "grain_mass": 0 + }, + "descr": "Aufheizen", + "temperature": 57, + "ramp": { + "rate": 1.5, + "stirrer": { + "speed": 75 + } + } + }, + { + "pot": { + "grain_mass": 0 + }, + "descr": "Einmaischen", + "user_message": "Bitte Malz einfüllen und bestätigen", + "user_wait_for_continue": true, + "hold": { + "stirrer": { + "speed": 0 + } + } + }, + { + "descr": "1. Rast", + "temperature": 55, + "hold": { + "duration": 20 + } + }, + { + "descr": "Glucose-Rast", + "temperature": 63, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 40, + "stirrer": { + "speed": 20, + "interval_time": 90, + "on_ratio": 0.8 + } + } + }, + { + "descr": "Verzuckerungs-Rast", + "temperature": 72, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 30, + "stirrer": { + "speed": 35, + "interval_time": 120 + } + } + }, + { + "pot": { + "water_mass": 20 + }, + "descr": "Abmaischen", + "user_message": "Quittieren zum Abmaischen", + "user_wait_for_continue": true, + "temperature": 76, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 30, + "stirrer": { + "speed": 50 + } + } + } + ] +} diff --git a/tests/components/pid/test_temp_controller_closed_loop.py b/tests/components/pid/test_temp_controller_closed_loop.py index 13521c0..35c58ba 100644 --- a/tests/components/pid/test_temp_controller_closed_loop.py +++ b/tests/components/pid/test_temp_controller_closed_loop.py @@ -10,16 +10,17 @@ AMBIENT = 20.0 PLANT_PARAMS = {"M": 27.96, "C": 3403.43, "L": 0.2, "Td": 17} OUTER_PARAMS = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0} +OUTER_PARAMS_HOLD_COOL = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0, "y_hold_min": -0.1} INNER_HEAT_PARAMS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5} INNER_HOLD_CLAMPED = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5, "yi_max": 0.3} INNER_HOLD_UNCLAMPED = dict(INNER_HEAT_PARAMS) -def make_controller(inner_hold_params): +def make_controller(inner_hold_params, outer_params=None): ctrl = TempController(DT) ctrl.set_params({ "beta": 0.9, - "Outer": dict(OUTER_PARAMS), + "Outer": dict(outer_params if outer_params is not None else OUTER_PARAMS), "Inner": { "Heat": dict(INNER_HEAT_PARAMS), "Hold": dict(inner_hold_params), @@ -88,6 +89,56 @@ class TestHoldDisturbanceOvershoot(unittest.TestCase): self.assertLess(overshoot_clamped, overshoot_unclamped) +class TestHoldOvershootRecoversToSetpoint(unittest.TestCase): + """Reproduces the sud_0030 grain-fill-in incident: a HOLD overshoot + (post-recovery temp_ist above temp_soll, well under HoldCool's + threshold so the FSM never leaves HOLD) must coast back down to + setpoint rather than sitting in a permanent flat-clamp dead zone - + see docs/overshoot_hold_windup.md and the Outer.y_hold_min floor in + temp_controller_base.py's process_pid().""" + + def _run_overshoot(self, outer_params): + ctrl = make_controller(INNER_HOLD_CLAMPED, outer_params) + plant = make_plant(30.0) + + for _ in range(60): + tick(ctrl, plant, 30.0, 1.0) + self.assertEqual(ctrl.state, States.HOLD) + + # Overshoot above setpoint, small enough to stay inside HOLD + # (matches the ~0.3-0.4 degC band observed in the real trace). + plant.temp += 0.4 + + min_temp = plant.get_temperature() + for _ in range(900): + tick(ctrl, plant, 30.0, 1.0) + self.assertEqual(ctrl.state, States.HOLD) + min_temp = min(min_temp, plant.get_temperature()) + + return plant.get_temperature(), min_temp + + def test_negative_floor_recovers_flat_clamp_does_not(self): + final_flat, _ = self._run_overshoot(OUTER_PARAMS) + final_floored, min_floored = self._run_overshoot(OUTER_PARAMS_HOLD_COOL) + + # Old behaviour (y clamped to exactly 0.0): pid_inner fights the + # pot's own ambient loss to hold the overshot temperature flat - + # it stays measurably above setpoint within this window. + self.assertGreater(final_flat - 30.0, 0.05) + + # New behaviour (small negative floor): the outer loop can ask + # for a gentle decline, so the overshoot recovers much closer to + # setpoint than the flat clamp manages in the same window. + self.assertLess(abs(final_floored - 30.0), 0.15) + + # Guard against reintroducing the violent limit cycle the original + # [0, 1] clamp was added to break (bb5af3c): recovery should coast + # down smoothly, undershooting by no more than the injected + # disturbance itself (0.4 degC) - not the ~1 degC+ swings of a + # runaway power on/off cycle. + self.assertGreater(min_floored, 30.0 - 0.4) + + class TestRealRampStillReachesTarget(unittest.TestCase): """Guards against reintroducing the rejected flat-clamp regression: Inner.Heat has no yi_max, so a genuine ramp must still be able to