diff --git a/components/pid/TODO.md b/components/pid/TODO.md index a3e43e8..62a7c9b 100644 --- a/components/pid/TODO.md +++ b/components/pid/TODO.md @@ -32,13 +32,17 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). hold_scale)` — the overshoot-compensation scheduling logic now lives entirely in the temp controller, not the generic `Pid` block. -- [ ] **No automated tests.** `tests/components/pid/` was added once +- [ ] **No automated tests for the heat-rate filtering or Smith + correction specifically.** An earlier `tests/components/pid/` suite (stdlib `unittest`, no pytest) covering FSM transitions, anti-windup - clamping, and Kalman convergence, but was removed again before the - Kalman-filter removal/Smith rewrite, so none of the current - `temp_controller*.py` behavior (backward-difference + low-pass - filtered heat rate, the two-model Smith correction) has test - coverage. This drives a physical heater — worth re-adding. + clamping, and Kalman convergence was removed before the Kalman-filter + removal/Smith rewrite. A new `tests/components/pid/` suite was added + alongside the HOLD-windup fix below (`test_pid.py`, + `test_temp_controller_closed_loop.py`) covering the `yi_max` clamp and + closed-loop disturbance/ramp/transition behavior, but + `_compute_heatrate()`'s backward-difference + low-pass filtering and + the two-model Smith correction itself still have no dedicated + coverage. This drives a physical heater — worth extending. - [ ] **`set_model_power` isn't defined on every controller, but `brewpi.py` wires it unconditionally.** `brewpi.py` always does @@ -63,7 +67,7 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). before that file was removed entirely — `Pot` dropped `gain` entirely, see `components/plant/TODO.md`. -- [ ] **`pid_heat` can wind up during a `HOLD`-state disturbance with no +- [x] **`pid_heat` can wind up during a `HOLD`-state disturbance with no anti-windup engagement.** A cold-water disturbance while holding drove `pid_heat`'s integral term up without ever saturating `y` (peaked at `y≈0.74` of the `1.0` ceiling), so the existing back-calculation @@ -75,16 +79,108 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). disturbance itself peaked at, so no single clamp value can suppress the windup without also capping legitimate ramps. An FSM-gating alternative (freeze the loop's output in `HOLD` unless engaged) was also superseded. - Current plan: rename `pid_hold`/`pid_heat`/`pid_cool` to `pid_outer`/ + Fixed: renamed `pid_hold`/`pid_heat`/`pid_cool` to `pid_outer`/ `pid_inner`/`pid_inner_cool` (matching what actually runs when), and split the inner loop's config into `Inner.Heat`/`Inner.Hold`/ `Inner.Cool` so the *same* PID instance gets a tight `yi_max` only while `HOLD` is driving it and stays unclamped for real `HEAT` ramps — no freeze/thaw, bumpless transfer preserved for free. See - `docs/overshoot_hold_windup.md` for the full writeup and test plan. This - is also a breaking config change (`Hold`/`Heat`/`Cool` → `Outer`/ - `Inner.*`) — every deployed `config.json` needs migrating, not just the - repo templates. + `docs/overshoot_hold_windup.md` for the full writeup, and + `docs/fsm_states.png` for a static screenshot of the FSM-states panel + of the cascade architecture diagram (states/thresholds, and the + HOLD→HEAT no-reset note). The full interactive diagram (signal-flow + cascade + FSM inset + config-mapping table) is a Claude Artifact, + not a repo file: https://claude.ai/code/artifact/a32e4752-b4a6-4153-b344-eb2423eb6512 + — only reachable by whoever has access to the Claude account/session + that created it, not a durable link for the team; `docs/fsm_states.png` + is the durable copy. This was also a breaking config change (`Hold`/`Heat`/`Cool` → + `Outer`/`Inner.*`) — `config.json`, the templates, and the demo scripts + were all migrated. Test coverage (closed-loop disturbance/ramp/transition + 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`. + +- [ ] **Architecture isn't ready for a real active cooler yet, only a + heat-only actuator.** Asked (not yet built): would the cascade/FSM + design still hold up if an active cooler (compressor/chiller, some + finite cooling power) replaced "heater off + passive ambient loss"? + Mostly yes at the PID/plant-model level: `Pid.y_min`/`y_max` are + already symmetric `-1.0`/`1.0` (`pid.py:12-13`), `Pot.set_power()`/ + `process()` are sign-agnostic so the simulator already models negative + power correctly, and the FSM already has a dedicated `COOL` state with + its own PID instance (`pid_inner_cool`, `temp_controller_fsm.py:35-39`) + whose comment explicitly anticipates this ("the actuator ... is + responsible for clamping the resulting negative power to whatever it's + actually capable of"). + The gap is at the actuator boundary: today `y` only ever reaches one + consumer, `tasks/heater.py:59`'s `self.power_actor = max(0, ... * y)`, + which silently discards every negative command — that's what's made + tuning mismatches free so far. A real cooler needs (a) a second + actuator/device class consuming the negative half instead of that + `max(0, ...)` clamp — there's no `Cooler`/chiller abstraction anywhere + yet (`grep -rniE "cooler|chiller|compressor"` across all `.py` is + empty, fully greenfield), and `heater_hendi.py:80`'s `set_power` only + clamps to a max, not a min, so feeding it a negative value today would + call `setPowerWatts(negative)` on real hardware with undefined result; + and (b) its own power ceiling, since one normalized `y` scaled by one + `get_power_max()` breaks once heater and cooler have different max + power. + Biggest risk once that clamp is removed: two separate paths start + issuing real negative-power commands that were previously harmless - + HOLD's `Outer.y_hold_min` floor (via `Inner.Hold`, see the steady-state + overshoot item above) and the pre-existing `pid_inner_cool` (via + `Inner.Cool`) - and neither has been tuned against actual active-cooling + dynamics (compressor lag, min-on-time). `Inner.Cool` in the demo + configs is literally a copy of `Inner.Heat`'s gains, never validated + independently. So adding a real cooler is a wiring task (new actuator + class, per-actuator power scaling) plus a re-tuning task (`Inner.Cool`, + and re-checking the `HoldCool`/`CoolHold` thresholds in + `temp_controller_fsm.py:14-17`), not just wiring. + +- [ ] **`Outer.y_hold_min` is a fixed heuristic constant, not grounded in + the pot's actual passive-cooling capacity.** Related to the active-cooler + item above. `Pot.process()` (`components/plant/pot.py:86-89`) already + computes `p_loss = L * M * (temp - amb)` unconditionally every tick, + independent of `y` - passive cooling isn't actually *driven by* the + negative half of `y` today, it's a physics term that happens regardless + of what the controller commands. `-0.1` (see the steady-state overshoot + fix above) was picked by estimating a typical passive loss rate by hand + for one plant/ambient combination; it doesn't adapt to a different pot, + water mass, or `theta_ist - theta_amb` delta. + `TempController` (Smith, `temp_controller_smith.py:12-16,22-32`) already + carries its own internal `Pot` model with `L`/`M`/`C` and ambient + temperature set (needed for the Smith prediction) - it could expose a + `heatrate_loss_max(theta_ist) = L * (theta_ist - theta_amb) / C * 60` + (K/min) from that model and use it to size `y_hold_min` (and bound + `pid_inner_cool`'s target) dynamically per-tick, instead of a fixed + config constant - the "cooling path" would always ask for exactly what + passive loss can actually deliver, no more and no less. Also a natural + stepping stone toward the active-cooler item above: same "ceiling" + concept, just a bigger number once real cooling power exists. + Caveat: `temp_controller.py` ("Normal", non-Smith) has no internal plant + model at all - `set_model_plant_params()`/`set_ambient_temperature()` + are no-ops in `TempControllerBase` and only overridden in the Smith + subclass - so this only works for Smith out of the box; Normal would + need either its own lightweight loss estimate (e.g. from observed + `heatrate_ist` decay while power is ~0) or keep the fixed fallback. - [ ] **`kalman.py` is now dead code in production.** Neither `temp_controller.py` nor `temp_controller_smith.py` uses `Kalman` diff --git a/components/pid/pid.py b/components/pid/pid.py index b82dfdd..0c40722 100644 --- a/components/pid/pid.py +++ b/components/pid/pid.py @@ -35,6 +35,9 @@ class Pid: kt = self.params['kt'] * scale self.yi = self.yi + ki*dt * err + kt*dt * self.awu + yi_max = self.params.get('yi_max') + if yi_max is not None: + self.yi = max(-yi_max, min(yi_max, self.yi)) yd = kd/dt*(d - self.d) yp = kp * err diff --git a/components/pid/temp_controller_base.py b/components/pid/temp_controller_base.py index 2a380b5..6d0cc83 100644 --- a/components/pid/temp_controller_base.py +++ b/components/pid/temp_controller_base.py @@ -19,6 +19,9 @@ class TempControllerBase(TempControllerFsm, APid): # params/set_params() (components/pid/pid.py), whose process() # already relies on the same "None means not configured yet". 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 @@ -29,9 +32,15 @@ class TempControllerBase(TempControllerFsm, APid): self.params = params self.thresholds = {**DEFAULT_THRESHOLDS, **params.get('Thresholds', {})} self.beta = params.get('beta', 0.05) - self.pid_hold.set_params(params['Hold']) - self.pid_heat.set_params(params['Heat']) - self.pid_cool.set_params(params['Cool']) + 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']) def _compute_heatrate(self, theta_ist): """Pre-filter theta_ist, then differentiate and low-pass to get heatrate_ist. @@ -131,31 +140,40 @@ class TempControllerBase(TempControllerFsm, APid): return self.heatrate_soll_set def process_pid(self, theta_err, hold_scale=1.0): - self.pid_hold.process(theta_err, -self.theta_ist, hold_scale) - # In HOLD state, clamp pid_hold's output to [0, 1]: a small temperature - # overshoot makes pid_hold.y go negative, which would invert heatrate_soll - # and drive pid_heat'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. - pid_hold_y = self.pid_hold.get_y() + self.pid_outer.process(theta_err, -self.theta_ist, hold_scale) + # 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_hold_y = max(0.0, pid_hold_y) - self.heatrate_soll = self.heatrate_soll_set * pid_hold_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 # Only the PID actually driving y is advanced - otherwise the - # inactive one (e.g. pid_heat while COOL has pid_cool driving) + # inactive one (e.g. pid_inner while COOL has pid_inner_cool driving) # would keep silently integrating against a heatrate_err that # isn't actually under its control, building a stale windup that # causes a discontinuity in y the moment it takes back over. if self.state == States.IDLE: self.y = 0 elif self.state == States.COOL: - self.pid_cool.process(heatrate_err, -self.heatrate_ist) - self.y = self.pid_cool.get_y() + self.pid_inner_cool.process(heatrate_err, -self.heatrate_ist) + self.y = self.pid_inner_cool.get_y() else: - self.pid_heat.process(heatrate_err, -self.heatrate_ist) - self.y = self.pid_heat.get_y() + inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params + self.pid_inner.set_params(inner_params) + self.pid_inner.process(heatrate_err, -self.heatrate_ist) + self.y = self.pid_inner.get_y() self.post_pid() diff --git a/components/pid/temp_controller_fsm.py b/components/pid/temp_controller_fsm.py index f641d43..3d9dc3f 100644 --- a/components/pid/temp_controller_fsm.py +++ b/components/pid/temp_controller_fsm.py @@ -8,9 +8,9 @@ DEFAULT_THRESHOLDS = { # when COOL meant nothing more than going idle - it's an active state # with its own PID now, so a threshold this tight relative to a real # heater's discrete power steps/sensor noise causes the system to - # chatter in and out of it every tick, resetting both pid_hold's and - # pid_cool's integrators each time and never letting either actually - # converge. Symmetric with the others instead. + # chatter in and out of it every tick, resetting both pid_outer's and + # pid_inner_cool's integrators each time and never letting either + # actually converge. Symmetric with the others instead. "HoldCool": 1.0, "HeatHold": 1.0, "HeatCool": 1.0, @@ -30,14 +30,14 @@ class States(enum.Enum): class TempControllerFsm: def __init__(self, dt): - self.pid_hold = Pid(dt) - self.pid_heat = Pid(dt) + self.pid_outer = Pid(dt) + self.pid_inner = Pid(dt) # Separate gains for ramping down (negative diff) - the actuator # (e.g. a heat-only Pot/heater) is responsible for clamping the # resulting negative power to whatever it's actually capable of; # the controller itself no longer assumes "can't cool" == "must go # idle". - self.pid_cool = Pid(dt) + self.pid_inner_cool = Pid(dt) self.thresholds = None self.state = States.INIT self.is_startup = True @@ -80,13 +80,13 @@ class TempControllerFsm: # which calls this method directly), so an unconditional HOLD # here gets mistaken for "ramp already reached" and can finish # a freshly (re-)started step instantly - see SudTask. - # on_process()'s is_holding() check. pid_heat/pid_cool were - # frozen (see process_pid()) and possibly stale for as long as - # we were disabled - start whichever one matters clean rather + # on_process()'s is_holding() check. pid_inner/pid_inner_cool + # were frozen (see process_pid()) and possibly stale for as long + # as we were disabled - start whichever one matters clean rather # than resuming wherever it last left off. - self.pid_hold.reset() - self.pid_heat.reset() - self.pid_cool.reset() + self.pid_outer.reset() + self.pid_inner.reset() + self.pid_inner_cool.reset() if diff >= self.thresholds['HoldHeat']: state_next = States.HEAT elif diff <= -self.thresholds['HoldCool']: @@ -96,31 +96,31 @@ class TempControllerFsm: elif self.state == States.HOLD: if diff >= self.thresholds['HoldHeat']: state_next = States.HEAT - # No pid_heat.reset() here — bumpless transfer: carry the + # No pid_inner.reset() here — bumpless transfer: carry the # hold-phase integral into the new ramp so power doesn't # drop to near-zero and crawl back up from scratch. elif diff <= -self.thresholds['HoldCool']: state_next = States.COOL - self.pid_cool.reset() + self.pid_inner_cool.reset() elif self.state == States.HEAT: if diff <= -self.thresholds['HeatCool']: state_next = States.COOL - self.pid_cool.reset() + self.pid_inner_cool.reset() elif diff <= self.thresholds['HeatHold']: state_next = States.HOLD - self.pid_hold.reset() + self.pid_outer.reset() elif self.state == States.COOL: if diff >= self.thresholds['CoolHeat']: state_next = States.HEAT - self.pid_heat.reset() + self.pid_inner.reset() elif diff >= -self.thresholds['CoolHold']: state_next = States.HOLD - self.pid_hold.reset() - # pid_heat was frozen during COOL (see process_pid()) - + self.pid_outer.reset() + # pid_inner was frozen during COOL (see process_pid()) - # resume it clean rather than from whatever it last held # before COOL took over, which by now may be a stale fit # for a completely different part of the curve. - self.pid_heat.reset() + self.pid_inner.reset() if state_next != self.state: self.state = state_next diff --git a/components/sud_forecast.py b/components/sud_forecast.py index f62a956..7f2dd87 100644 --- a/components/sud_forecast.py +++ b/components/sud_forecast.py @@ -46,7 +46,7 @@ class SudForecastEstimator: estimate() also duty-cycles the PID's continuous output across the heater's own discrete power steps (see its actuate() closure), exactly like the real run's HeaterTask/device chain does - feeding tc.get_power() - straight to the plant instead lets pid_heat's own oscillatory tendency + straight to the plant instead lets pid_inner's own oscillatory tendency reach the plant undamped, producing a forecast far more jagged than any real run actually is (the discrete steps end up duty-cycling it back down to something close to the commanded average).""" diff --git a/config-real.json.tpl b/config-real.json.tpl index 5db5933..b6ed3e9 100644 --- a/config-real.json.tpl +++ b/config-real.json.tpl @@ -4,23 +4,33 @@ "TempCtrl": { "pid_type": "Smith", "beta": 0.05, - "Hold": { + "Outer": { "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, - "Heat": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 - }, - "Cool": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 + "Inner": { + "Heat": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + }, + "Hold": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5, + "yi_max": 0.3 + }, + "Cool": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + } }, "Thresholds": { "HoldHeat": 1.0, diff --git a/config-sim.json.tpl b/config-sim.json.tpl index 7c30879..c911a7c 100644 --- a/config-sim.json.tpl +++ b/config-sim.json.tpl @@ -4,23 +4,33 @@ "TempCtrl": { "pid_type": "Smith", "beta": 0.05, - "Hold": { + "Outer": { "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, - "Heat": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 - }, - "Cool": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 + "Inner": { + "Heat": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + }, + "Hold": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5, + "yi_max": 0.3 + }, + "Cool": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + } }, "Thresholds": { "HoldHeat": 1.0, diff --git a/docs/fsm_states.png b/docs/fsm_states.png new file mode 100644 index 0000000..7271911 Binary files /dev/null and b/docs/fsm_states.png differ 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 ed04bbb..9dea9cf 100644 --- a/docs/overshoot_hold_windup.md +++ b/docs/overshoot_hold_windup.md @@ -1,10 +1,12 @@ -# HOLD-state overshoot from a disturbance: root cause and fix plan +# HOLD-state overshoot from a disturbance: root cause and fix Investigation of the overshoot visible in `docs/overshoot.png`: a cold-water disturbance during a steady HOLD at 30°C caused `theta_ist` to overshoot to ~30.5°C and stay there, rather than settling back at soll. This document -records the diagnosis and the (not yet implemented) fix plan. Numbers below -are pulled from `logs/log_latest.json`. +records the diagnosis, the fix that shipped for it, and the test coverage +added alongside it. Numbers in the incident summary below are pulled from +`logs/log_latest.json`; names in that section (`pid_hold`/`pid_heat`) are +the *pre-fix* names — see "Rename for honesty" below for what they became. ## Incident summary @@ -88,23 +90,24 @@ on which state is currently active. ### Rename for honesty -The current names don't match what actually runs when: -- `pid_hold` already runs unconditionally *every* tick regardless of state - (`process_pid()`'s first line, `temp_controller_base.py:134`) — it's the - outer loop, not "the HOLD-state PID". Rename to **`pid_outer`**. -- `pid_heat` already runs in *both* `HOLD` and `HEAT` today (only `IDLE`/ - `COOL` skip it, `temp_controller_base.py:151-158`) — it's the inner loop - for the heating direction. Rename to **`pid_inner`**. -- `pid_cool` only ever runs in `COOL` — no `HOLD`-time ambiguity, since a +The pre-fix names didn't match what actually ran when: +- `pid_hold` already ran unconditionally *every* tick regardless of state + (`process_pid()`'s first line, `temp_controller_base.py:137`) — it's the + outer loop, not "the HOLD-state PID". Renamed to **`pid_outer`**. +- `pid_heat` already ran in *both* `HOLD` and `HEAT` (only `IDLE`/`COOL` + skip it, `temp_controller_base.py:154-163`) — it's the inner loop for + the heating direction. Renamed to **`pid_inner`**. +- `pid_cool` only ever ran in `COOL` — no `HOLD`-time ambiguity, since a disturbance that pushes temp *above* soll during `HOLD` is still handled - by `pid_inner` (the outer loop's `max(0.0, pid_hold_y)` floor sends + by `pid_inner` (the outer loop's `max(0.0, pid_outer_y)` floor sends `heatrate_soll` to 0, and `pid_inner` reacts to the resulting negative `heatrate_err` — the state only escalates to real `COOL` past - `Thresholds.HoldCool`). Rename to **`pid_inner_cool`** for symmetry, kept - as its own instance — today's explicit `reset()` calls when crossing + `Thresholds.HoldCool`). Renamed to **`pid_inner_cool`** for symmetry, kept + as its own instance — the explicit `reset()` calls when crossing between heat-direction and cool-direction states - (`temp_controller_fsm.py:104,108,115,123`) stay exactly as they are; there - is no reason to share integrator state across a heater/chiller boundary. + (`temp_controller_fsm.py:104,108,115,123`) stay exactly as they were; + there is no reason to share integrator state across a heater/chiller + boundary. ### Config: `Hold`/`Heat`/`Cool` → `Outer` / `Inner.{Heat,Hold,Cool}` @@ -131,31 +134,31 @@ The current names don't match what actually runs when: cooling-side incident. `pid_inner_cool` never runs during `HOLD`, so it doesn't need its own `Hold` variant the way `Heat` does; one params block is enough. -- **This is a breaking config change** — no backward-compat shim for the +- **This was a breaking config change** — no backward-compat shim for the old flat `Hold`/`Heat`/`Cool` keys (per the "no compat hacks" convention). - Every deployed `config.json` needs migrating, not just the repo's + Every deployed `config.json` needed migrating, not just the repo's `config-real.json.tpl`/`config-sim.json.tpl` templates. -### Code changes (planned, not yet implemented) +### Code changes (implemented) -1. **`components/pid/pid.py`** — add the symmetric `yi_max` clamp inside - `process()` (already planned): `self.yi = max(-yi_max, min(yi_max, - self.yi))` when `self.params.get('yi_max')` is set, applied right after +1. **`components/pid/pid.py`** — the symmetric `yi_max` clamp lives inside + `process()` (line 40): `self.yi = max(-yi_max, min(yi_max, self.yi))` + when `self.params.get('yi_max')` is set, applied right after accumulating `yi` and before it's summed into `y`. -2. **`components/pid/temp_controller_fsm.py`** — rename `self.pid_hold` → +2. **`components/pid/temp_controller_fsm.py`** — `self.pid_hold` → `self.pid_outer`, `self.pid_heat` → `self.pid_inner`, `self.pid_cool` → - `self.pid_inner_cool` (constructor at lines 33-34/40, all `reset()` call + `self.pid_inner_cool` (constructor at lines 33/34/40, all `reset()` call sites and comments at lines 11-12, 83, 87-89, 99, 104, 108, 111, 115, - 118-119, 123). + 119, 123). 3. **`components/pid/temp_controller_base.py`**: - - `set_params()` (line 28-34): `self.pid_outer.set_params(params['Outer'])`; - store `self._inner_heat_params = params['Inner']['Heat']` and + - `set_params()` (lines 30-37): `self.pid_outer.set_params(params['Outer'])`; + stores `self._inner_heat_params = params['Inner']['Heat']` and `self._inner_hold_params = params['Inner']['Hold']` for the per-tick lookup below; `self.pid_inner_cool.set_params(params['Inner']['Cool'])`. - - `process_pid()` (lines 133-160): rename `pid_hold_y` → `pid_outer_y` - and the `self.pid_hold.process(...)` call. In the combined `HOLD`/`HEAT` - branch (today's `else`, lines 156-158), select the active param set - before processing: + - `process_pid()` (lines 136-163): `pid_hold_y` → `pid_outer_y`, and the + `self.pid_hold.process(...)` call. In the combined `HOLD`/`HEAT` + branch (lines 159-163), the active param set is selected before + processing: ```python else: inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params @@ -165,61 +168,144 @@ The current names don't match what actually runs when: ``` `set_params()` is a cheap dict-reference assignment (`pid.py:22-23`), so calling it every tick has no meaningful cost. Because `kp`/`ki`/`kd`/ - `kt` are identical between `Inner.Heat` and `Inner.Hold` in the starting - config, switching the active set at a `HOLD↔HEAT` transition changes - no term of `y` at that instant — only the `yi_max` ceiling going - forward. No bump at the transition, unlike the freeze/thaw approach. + `kt` are identical between `Inner.Heat` and `Inner.Hold` in the + shipped config, switching the active set at a `HOLD↔HEAT` transition + changes no term of `y` at that instant — only the `yi_max` ceiling + going forward, with one caveat noted in Status below. 4. **Config files** — `config.json`, `config-real.json.tpl`, - `config-sim.json.tpl`: restructure into `Outer`/`Inner.{Heat,Hold,Cool}` - as above. Inline `"Cool": {...}` dicts in `scripts/demos/pid/ - demo_temp_controller_smith.py:21`, `demo_temp_controller.py:21`, and - `scripts/demos/sud/demo_sud.py:32` need the same restructure. + `config-sim.json.tpl` restructured into `Outer`/`Inner.{Heat,Hold,Cool}` + as above. The inline `"Cool": {...}` dicts in `scripts/demos/pid/ + demo_temp_controller_smith.py`, `demo_temp_controller.py`, and + `scripts/demos/sud/demo_sud.py` got the same restructure. 5. **`utils/replay_sim.py`** — `_apply_gain_overrides()` and the CLI flag - loop (lines 50, 214-222) iterate `('Hold','hold'), ('Heat','heat'), - ('Cool','cool')`; becomes `('Outer','outer'), ('Inner.Heat','inner-heat'), - ('Inner.Hold','inner-hold'), ('Inner.Cool','inner-cool')` (nested dict - access needed since these aren't flat top-level keys any more). The - `for section in ('Hold','Heat','Cool')` print loop at line 284 and the - `_infer_heatrate_soll_set()` docstring's "hold PID" reference (line 66) - need the same rename. -6. **`components/pid/TODO.md`** — update the cross-link entry added for the - previous (superseded) plan to point at this rewritten section instead. + loop now iterate the shared `GAIN_SECTIONS = (('Outer','outer'), + ('Inner.Heat','inner-heat'), ('Inner.Hold','inner-hold'), + ('Inner.Cool','inner-cool'))`, with nested dict access for the + `Inner.*` entries. The params print loop and + `_infer_heatrate_soll_set()`'s docstring were updated to match + (`pid_outer.get_y()` instead of "the hold PID"). +6. **`components/pid/TODO.md`** — the windup entry is marked `[x]` and + points at this section. -## Test plan (not yet implemented) +## Tests -Stdlib `unittest`, no pytest — `tests/components/pid/test_pid.py`, -discoverable via `python -m unittest discover -t . -s tests/components/pid`. -Simpler than the FSM-gating plan's test plan: no engage/disengage hysteresis -or threshold-relationship behavior to cover, since the loop is never turned -off — only its `yi_max` ceiling changes with state. +Stdlib `unittest`, no pytest — `tests/components/pid/test_pid.py` and +`test_temp_controller_closed_loop.py`, discoverable via +`python -m unittest discover -t . -s tests/components/pid` (or `-s tests` +for the whole repo suite). Simpler than the FSM-gating plan's test plan +would have needed: no engage/disengage hysteresis or threshold-relationship +behavior to cover, since the loop is never turned off — only its `yi_max` +ceiling changes with state. -**A. Unit-level, isolated `Pid`** — no plant involved. Feed a synthetic -`err` sequence shaped like the incident (positive `heatrate_err ≈ 0.3` held -for ~130 ticks, then decaying/negative tail, matching the real -`rate_soll - rate_ist` pulled from the log) into two `Pid` instances with -identical gains, one with `yi_max` set (the `Inner.Hold` case) and one -without (`Inner.Heat`). Assert: recovery time (ticks after error goes -negative until `y` drops back under a small threshold) is measurably -shorter when clamped; `yi` never exceeds the configured bound. +**A. Unit-level, isolated `Pid`** (`test_pid.py`) — no plant involved. Feeds +a synthetic `err` sequence shaped like the incident (positive +`heatrate_err = 0.3` held for 130 ticks, matching the outer loop's real +demand during the disturbance, then a flat `-0.3` tail for 200 ticks, +matching the real `rate_soll - rate_ist` gap once the outer loop had +zeroed its target) into two `Pid` instances with identical gains, one with +`yi_max` set (the `Inner.Hold` case) and one without (`Inner.Heat`). +Asserts: recovery time (ticks after the error goes negative until `y` +drops back under a small threshold) is measurably shorter when clamped; +`yi` never exceeds the configured bound; the unclamped instance's `yi` +does exceed it (sanity-checks the test itself isn't vacuous). -**B. Closed-loop, self-contained synthetic scenario** — no dependency on -the multi-MB log file. Real numbers from `PlantParams`/`config.json`: -`Pot(dt)` with `M=27.96, C=3403.43, L=0.2, Td=17`, ambient from -`config.json`; `TempController` via `PidFactory.create('Smith', dt)` with -the real `Outer`/`Inner.*` gains. Run closed-loop (controller's own `y` -drives the plant, unlike `utils/replay_sim.py`'s open-loop observe-only -mode): +**B. Closed-loop** (`test_temp_controller_closed_loop.py`) — real `Pot(dt)` +plant with `M=27.96, C=3403.43, L=0.2, Td=17`, ambient `20`°C, driven by a +real `TempController(Smith)` with the shipped `Outer`/`Inner.*` gains. +Controller's own `y` feeds back into the plant each tick (unlike +`utils/replay_sim.py`'s open-loop observe-only mode): - **Disturbance case**: hold at 30°C until settled, knock `plant.temp` down - ~0.5°C to emulate the cold-water event, keep ticking for several minutes. - Assert peak overshoot above 30.0°C is measurably smaller with `Inner.Hold`'s - `yi_max` set than without. -- **Ramp case**: command a genuine 1.5 K/min ramp (state reaches `HEAT`) and - assert the sustained heat rate actually reaches ~1.5 K/min — guards against - reintroducing the flat-clamp regression from the rejected approach above. -- **Transition case**: drive a `HOLD→HEAT→HOLD` sequence and assert `y` has - no discontinuity at either transition beyond what the changing `heatrate_err` - itself would explain — confirms the per-state param swap is truly bumpless. + 0.5°C to emulate the cold-water event, keep ticking for 600 more ticks — + confirms the state stays in `HOLD` throughout, matching the incident. + Asserts peak overshoot above 30.0°C is measurably smaller with + `Inner.Hold`'s `yi_max` set than with an unclamped copy of `Inner.Heat`. +- **Ramp case**: commands a genuine 1.5 K/min ramp to 40°C (state reaches + `HEAT`) and asserts the sustained heat rate actually exceeds 1.4 K/min — + guards against reintroducing the flat-clamp regression from the rejected + approach above. +- **Transition case**: drives a `HOLD→HEAT→HOLD` sequence and asserts the + `y` step at either transition stays under `0.1` — see the retroactive-clamp + caveat in Status below for why this isn't a stricter "no discontinuity" + assertion. ## Status -Plan only — nothing in this document has been implemented yet. +Implemented: `pid.py`'s `yi_max` clamp, the `pid_outer`/`pid_inner`/ +`pid_inner_cool` rename, the `Outer`/`Inner.{Heat,Hold,Cool}` config +restructure (`config.json`, both `.tpl` templates, and the three demo +scripts), and `utils/replay_sim.py`'s matching CLI-flag/print-loop rename. +Tests added under `tests/components/pid/` (`test_pid.py` for the isolated +`Pid` clamp behavior, `test_temp_controller_closed_loop.py` for the +closed-loop disturbance/ramp/transition cases) — all passing. + +One subtlety found while writing the closed-loop transition test that +this plan didn't anticipate: `Inner.Hold`'s `yi_max` clamp applies +retroactively. If a sustained `HEAT` ramp pushes `yi` above the `Hold` +ceiling before the `HeatHold` threshold fires, the very next tick after +the `HEAT→HOLD` transition clamps `yi` back down immediately, producing a +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 +state gating, Smith-predictor feedback) exists as a Claude Artifact: +https://claude.ai/code/artifact/a32e4752-b4a6-4153-b344-eb2423eb6512 — this +is a session-scoped link, not a durable one, so it may not resolve for +everyone with repo access. `docs/fsm_states.png` is a static screenshot of +just the diagram's FSM-states panel, checked in as the durable copy. diff --git a/docs/temp_control_calibration.md b/docs/temp_control_calibration.md index c25d070..e1d8f53 100644 --- a/docs/temp_control_calibration.md +++ b/docs/temp_control_calibration.md @@ -143,7 +143,7 @@ Two problems compound: differentiating is the least effective arrangement: noise has already been amplified before the filter sees it. -The noisy `heatrate_err` feeds into `pid_heat.process()` as the proportional +The noisy `heatrate_err` feeds into `pid_inner.process()` as the proportional error. With `kp=0.08`, ±1 K/min rate ripple produces ±8% power output noise. ### Options diff --git a/scripts/demos/pid/demo_temp_controller.py b/scripts/demos/pid/demo_temp_controller.py index c3af445..f055828 100644 --- a/scripts/demos/pid/demo_temp_controller.py +++ b/scripts/demos/pid/demo_temp_controller.py @@ -6,23 +6,33 @@ from components.pid.temp_controller import TempController if __name__ == '__main__': ctrl_params = { - "Hold": { + "Outer": { "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, - "Heat": { - "kp": 0.08, - "ki": 0.008, - "kd": 0.0, - "kt": 1.5 - }, - "Cool": { - "kp": 0.08, - "ki": 0.008, - "kd": 0.0, - "kt": 1.5 + "Inner": { + "Heat": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5 + }, + "Hold": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5, + "yi_max": 0.3 + }, + "Cool": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5 + } } } diff --git a/scripts/demos/pid/demo_temp_controller_smith.py b/scripts/demos/pid/demo_temp_controller_smith.py index d03069f..0813ddf 100644 --- a/scripts/demos/pid/demo_temp_controller_smith.py +++ b/scripts/demos/pid/demo_temp_controller_smith.py @@ -6,23 +6,33 @@ from components.pid.temp_controller_smith import TempController if __name__ == '__main__': ctrl_params = { - "Hold": { + "Outer": { "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, - "Heat": { - "kp": 0.08, - "ki": 0.008, - "kd": 0.0, - "kt": 1.5 - }, - "Cool": { - "kp": 0.08, - "ki": 0.008, - "kd": 0.0, - "kt": 1.5 + "Inner": { + "Heat": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5 + }, + "Hold": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5, + "yi_max": 0.3 + }, + "Cool": { + "kp": 0.08, + "ki": 0.008, + "kd": 0.0, + "kt": 1.5 + } } } diff --git a/scripts/demos/sud/demo_sud.py b/scripts/demos/sud/demo_sud.py index 77a8189..444cc78 100644 --- a/scripts/demos/sud/demo_sud.py +++ b/scripts/demos/sud/demo_sud.py @@ -17,23 +17,33 @@ if __name__ == '__main__': theta_amb = 20 ctrl_params = { - "Hold": { + "Outer": { "kp": 0.4, "ki": 0.0, "kd": 0.0, - "kt": 0.0 + "kt": 0.0, + "y_hold_min": -0.1 }, - "Heat": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 - }, - "Cool": { - "kp": 0.08, - "ki": 0.02, - "kd": 0.0, - "kt": 1.5 + "Inner": { + "Heat": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + }, + "Hold": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5, + "yi_max": 0.3 + }, + "Cool": { + "kp": 0.08, + "ki": 0.02, + "kd": 0.0, + "kt": 1.5 + } } } 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/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/components/__init__.py b/tests/components/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/components/pid/__init__.py b/tests/components/pid/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/components/pid/test_pid.py b/tests/components/pid/test_pid.py new file mode 100644 index 0000000..e5718cf --- /dev/null +++ b/tests/components/pid/test_pid.py @@ -0,0 +1,69 @@ +import unittest + +from components.pid.pid import Pid + + +GAINS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5} + + +def _feed(pid, err_sequence): + """Runs pid.process(err, 0) over err_sequence, returns the y trace.""" + ys = [] + for err in err_sequence: + pid.process(err, 0) + ys.append(pid.get_y()) + return ys + + +def _incident_err_sequence(): + """Shaped like the HOLD-disturbance incident: a positive heatrate_err + held for ~130 ticks (outer loop asking for real heat), then a negative + tail (outer loop has zeroed its target while heatrate_ist is still + high, matching the real rate_soll - rate_ist gap from the log).""" + return [0.3] * 130 + [-0.3] * 200 + + +class TestPidYiMax(unittest.TestCase): + def test_yi_never_exceeds_configured_bound(self): + pid = Pid(dt=1.0) + pid.set_params({**GAINS, "yi_max": 0.3}) + for err in _incident_err_sequence(): + pid.process(err, 0) + self.assertLessEqual(abs(pid.yi), 0.3 + 1e-9) + + def test_clamped_recovers_faster_than_unclamped(self): + err_sequence = _incident_err_sequence() + + clamped = Pid(dt=1.0) + clamped.set_params({**GAINS, "yi_max": 0.3}) + ys_clamped = _feed(clamped, err_sequence) + + unclamped = Pid(dt=1.0) + unclamped.set_params(dict(GAINS)) + ys_unclamped = _feed(unclamped, err_sequence) + + # Ticks after the error goes negative (index 130) until y drops + # back under a small threshold. + threshold = 0.05 + negative_start = 130 + + def recovery_time(ys): + for i in range(negative_start, len(ys)): + if ys[i] < threshold: + return i - negative_start + return len(ys) - negative_start + + self.assertLess(recovery_time(ys_clamped), recovery_time(ys_unclamped)) + + def test_no_yi_max_configured_is_unbounded(self): + pid = Pid(dt=1.0) + pid.set_params(dict(GAINS)) + peak_yi = 0.0 + for err in _incident_err_sequence(): + pid.process(err, 0) + peak_yi = max(peak_yi, pid.yi) + self.assertGreater(peak_yi, 0.3) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/components/pid/test_temp_controller_closed_loop.py b/tests/components/pid/test_temp_controller_closed_loop.py new file mode 100644 index 0000000..35c58ba --- /dev/null +++ b/tests/components/pid/test_temp_controller_closed_loop.py @@ -0,0 +1,199 @@ +import unittest + +from components.pid.temp_controller_smith import TempController +from components.pid.temp_controller_fsm import States +from components.plant.pot import Pot + + +DT = 1.0 +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, outer_params=None): + ctrl = TempController(DT) + ctrl.set_params({ + "beta": 0.9, + "Outer": dict(outer_params if outer_params is not None else OUTER_PARAMS), + "Inner": { + "Heat": dict(INNER_HEAT_PARAMS), + "Hold": dict(inner_hold_params), + "Cool": dict(INNER_HEAT_PARAMS), + }, + }) + ctrl.set_model_plant_params(PLANT_PARAMS) + ctrl.set_ambient_temperature(AMBIENT) + ctrl.set_enabled(True) + return ctrl + + +def make_plant(initial_temp): + plant = Pot(DT) + plant.set_plant_params(PLANT_PARAMS) + plant.set_ambient_temperature(AMBIENT) + plant.initial(initial_temp) + return plant + + +def tick(ctrl, plant, temp_soll, heatrate_soll): + plant.process() + ctrl.set_theta_ist(plant.get_temperature()) + ctrl.set_theta_soll(temp_soll) + ctrl.set_heatrate_soll(heatrate_soll) + ctrl.process() + power = 3500.0 * max(0.0, ctrl.get_power()) + plant.set_power(power) + ctrl.set_model_power(power) + + +class TestHoldDisturbanceOvershoot(unittest.TestCase): + """Reproduces the cold-water-during-HOLD incident from + docs/overshoot_hold_windup.md and checks that Inner.Hold's yi_max + measurably reduces the resulting overshoot.""" + + def _run_disturbance(self, inner_hold_params): + ctrl = make_controller(inner_hold_params) + plant = make_plant(30.0) + + # Settle at HOLD, 30 degC. + for _ in range(60): + tick(ctrl, plant, 30.0, 1.0) + self.assertEqual(ctrl.state, States.HOLD) + + # Cold-water disturbance: knock plant.temp down ~0.5 degC. + plant.temp -= 0.5 + + peak = plant.get_temperature() + for _ in range(600): + tick(ctrl, plant, 30.0, 1.0) + peak = max(peak, plant.get_temperature()) + # Disturbance must stay within HOLD the whole time, matching + # the incident (diff never reached HoldHeat). + self.assertEqual(ctrl.state, States.HOLD) + + return peak + + def test_clamped_overshoot_smaller_than_unclamped(self): + peak_clamped = self._run_disturbance(INNER_HOLD_CLAMPED) + peak_unclamped = self._run_disturbance(INNER_HOLD_UNCLAMPED) + + overshoot_clamped = peak_clamped - 30.0 + overshoot_unclamped = peak_unclamped - 30.0 + + 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 + reach its commanded heat rate.""" + + def test_ramp_reaches_commanded_heatrate(self): + ctrl = make_controller(INNER_HOLD_CLAMPED) + plant = make_plant(20.0) + + heatrate_soll = 1.5 + temp_soll = 40.0 + + max_heatrate_ist = 0.0 + reached_heat = False + for _ in range(600): + tick(ctrl, plant, temp_soll, heatrate_soll) + if ctrl.state == States.HEAT: + reached_heat = True + max_heatrate_ist = max(max_heatrate_ist, ctrl.get_heatrate_ist()) + + self.assertTrue(reached_heat) + self.assertGreater(max_heatrate_ist, 1.4) + + +class TestHoldHeatHoldTransitionIsBumpless(unittest.TestCase): + """Per-state yi_max swap must not itself introduce a discontinuity in y + at a HOLD<->HEAT transition - the same Pid instance/state carries over, + only the yi_max ceiling changes going forward.""" + + def test_no_bump_at_transitions(self): + ctrl = make_controller(INNER_HOLD_CLAMPED) + plant = make_plant(20.0) + + y_prev = None + state_prev = None + max_bump = 0.0 + for _ in range(1000): + tick(ctrl, plant, 40.0, 1.5) + y = ctrl.get_power() + if state_prev is not None and ctrl.state != state_prev and \ + {state_prev, ctrl.state} <= {States.HOLD, States.HEAT}: + max_bump = max(max_bump, abs(y - y_prev)) + y_prev = y + state_prev = ctrl.state + + # A "bump" here would be a y-step far larger than what one tick of + # normal PID evolution produces elsewhere in the same run - not + # literally zero, since heatrate_err itself keeps evolving tick to + # tick regardless of the state transition. Note: a HEAT->HOLD + # transition can retroactively clamp yi if a sustained ramp pushed + # it past Inner.Hold's yi_max before HeatHold's threshold fired, + # producing a small (not literally bumpless) step - bounded well + # below the full-freeze/thaw magnitude this replaces. + self.assertLess(max_bump, 0.1) + + +if __name__ == '__main__': + unittest.main() diff --git a/utils/replay_sim.py b/utils/replay_sim.py index 158940a..4232d36 100644 --- a/utils/replay_sim.py +++ b/utils/replay_sim.py @@ -45,13 +45,21 @@ from components.pid import PidFactory from components.plant.pot import Pot +GAIN_SECTIONS = (('Outer', 'outer'), ('Inner.Heat', 'inner-heat'), + ('Inner.Hold', 'inner-hold'), ('Inner.Cool', 'inner-cool')) + + def _apply_gain_overrides(params, args): p = copy.deepcopy(params) - for section, prefix in (('Hold', 'hold'), ('Heat', 'heat'), ('Cool', 'cool')): + for section, prefix in GAIN_SECTIONS: for gain in ('kp', 'ki', 'kd', 'kt'): - val = getattr(args, '{}_{}'.format(prefix, gain)) + val = getattr(args, '{}_{}'.format(prefix.replace('-', '_'), gain)) if val is not None: - p[section][gain] = val + if '.' in section: + outer, inner = section.split('.') + p[outer][inner][gain] = val + else: + p[section][gain] = val return p @@ -63,7 +71,7 @@ def _infer_max_power(samples): def _infer_heatrate_soll_set(samples): """Estimate heatrate_soll_set from the log. - rate_soll = heatrate_soll_set * pid_hold.get_y(); when the hold PID is + rate_soll = heatrate_soll_set * pid_outer.get_y(); when the outer PID is saturated at 1.0 during active heating the two are equal, so the max rate_soll seen while the heater is on is a good upper bound.""" candidates = [s['rate_soll'] for s in samples if s['power_set'] > 0] @@ -211,12 +219,12 @@ def main(): parser.add_argument('--plant-L', type=float, default=None, metavar='W/kgK', help='Heat loss coeff [W/(kg·K)]') parser.add_argument('--plant-Td', type=float, default=None, metavar='s', help='Transport delay [s]') - for section, prefix in (('Hold', 'hold'), ('Heat', 'heat'), ('Cool', 'cool')): + for section, prefix in GAIN_SECTIONS: for gain in ('kp', 'ki', 'kd', 'kt'): parser.add_argument( '--{}-{}'.format(prefix, gain), type=float, default=None, - dest='{}_{}'.format(prefix, gain), + dest='{}_{}'.format(prefix.replace('-', '_'), gain), metavar='VAL', help='Override TempCtrl.{}.{}'.format(section, gain), ) @@ -265,8 +273,8 @@ def main(): ambient = args.ambient if args.ambient is not None else config.get('ambient_temperature', 20.0) any_override = any( - getattr(args, '{}_{}'.format(p, g)) is not None - for p in ('hold', 'heat', 'cool') + getattr(args, '{}_{}'.format(prefix.replace('-', '_'), g)) is not None + for _, prefix in GAIN_SECTIONS for g in ('kp', 'ki', 'kd', 'kt') ) config_source = 'log' if log.get('Config') else 'file' @@ -281,8 +289,12 @@ def main(): print('Params: {} ({})'.format('overridden' if any_override else 'from {}'.format(config_source), 'log' if log_plant else 'defaults')) print() - for section in ('Hold', 'Heat', 'Cool'): - p = pid_params[section] + for section, _ in GAIN_SECTIONS: + if '.' in section: + outer, inner = section.split('.') + p = pid_params[outer][inner] + else: + p = pid_params[section] print(' {}: kp={kp} ki={ki} kd={kd} kt={kt}'.format(section, **p)) replay = run_replay(samples, pid_type, pid_params, rate_soll, plant_params, param_events,