# components/pid design backlog Findings from a design review, refreshed against the current state of `master` after the Kalman-filter removal and Smith-predictor rewrite (see git history for `temp_controller.py`/`temp_controller_smith.py`). - [x] **FSM thresholds aren't configurable.** Moved into an optional `TempCtrl.Thresholds` config section (`HoldIdle`, `HoldHeat`, `IdleHeat`, `IdleHold`, `HeatHold`, `HeatIdle`), merged over `DEFAULT_THRESHOLDS` (now in `temp_controller_base.py`, formerly `tc_constants.py`) so existing configs without the section keep working unchanged. - [x] **matplotlib imported at module level in production code.** `temp_controller.py`, `temp_controller_smith.py`, and `kalman.py` used to `from matplotlib.pyplot import ...` just to support their `__main__` self-test plots. The plotting demos (and `kalman_eval.py`) moved to `scripts/demos/pid/demo_*.py`; production modules no longer import matplotlib. - [x] **Three Kalman filters share one tuning.** No longer applicable: `temp_controller_smith.py` was rewritten to drop Kalman filtering entirely (see below), so there's no shared tuning to split anymore. - [x] **`Pid.scale()` is a gain-scheduling hack, not anti-windup.** Fixed: removed `Pid.scale()`/`self.k` entirely. `Pid.process(err, d, scale=1.0)` now takes the gain multiplier as a plain per-call argument instead of mutable state, so there's nothing to go stale across a `reset()`. `temp_controller*.py` compute `hold_scale = 1.0/heatrate_soll_set if heatrate_soll_set > 0 else 1.0` themselves and pass it through `process_pid(theta_err, heatrate_err, hold_scale)` — the overshoot-compensation scheduling logic now lives entirely in the temp controller, not the generic `Pid` block. - [ ] **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 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. - [x] **`set_model_power` isn't defined on every controller, but `brewpi.py` wires it unconditionally.** Fixed: `TempControllerBase` now has a no-op `set_model_power`/`set_ambient_temperature`/ `set_model_plant_params` (`temp_controller_base.py:83-90`), so `brewpi.py`'s unconditional `heater.set_on_changed("power_set", tc.set_model_power)` no longer crashes for `"pid_type": "Normal"` - `temp_controller_smith.py` still overrides `set_model_power` to actually feed its internal model. The redundant `hasattr()` guards at both call sites (`brewpi.py`, `components/sud_forecast.py`) were removed along with it. - [x] **Config access is unchecked `dict[key]` everywhere.** Partially fixed, scoped to the section that actually caused an incident (`config.json.sim`'s `gain`/`Model` nesting mismatch): `utils/ config_validate.py`'s `require()` is a small stdlib-only dotted-path checker (no schema library added - matches this project's no-pytest/ stdlib-first preference), and `validate_temp_ctrl()` requires `TempCtrl.pid_type` plus every `kp`/`ki`/`kd`/`kt` gain under `TempCtrl.Outer` and `TempCtrl.Inner.{Heat,Hold,Cool}` - the ones every `pid_type` reads unconditionally. Called once in `server/brewpi.py` right after `json.load()`, before any factory runs, so a missing/ misnamed gain now fails fast with `ConfigError: missing required config key: TempCtrl.Inner.Hold.kp` instead of a bare `KeyError` from inside some component's `__init__` or first `process()` tick. Covered by `tests/utils/test_config_validate.py`. Deliberately left for later, since neither has actually crashed anything yet: `Heater`/`Stirrer`/`TempSensor`/`Pot` config sections (all already default gracefully via `.get(..., default)` in `PlantFactory` etc.), and erroring (vs. just `log.warning()`) on unknown/stale keys like the old `TempCtrl.Kalman`/`Model.kn`/ `Plant.kn`/`Model.gain` leftovers - see `components/plant/TODO.md`. - [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 anti-windup (`Pid.process()`, `pid.py:45-48`) never triggered — the FSM never even left `HOLD` (`diff` stayed under `HoldHeat=1.0`). The resulting overshoot took ~35s+ to unwind naturally. A flat `yi_max` clamp was considered and rejected: sustaining a genuine 1.5 K/min ramp needs `y≈0.7-0.75` from `yi` alone at steady state, the same range the 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. 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 `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` anymore; the only remaining references are `scripts/demos/pid/demo_kalman.py` and `demo_kalman_eval.py`. Decide whether to keep it as a documented standalone filtering example or delete it along with the demos.