brewpi/components/pid/TODO.md

# 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.

- [ ] **`set_model_power` isn't defined on every controller, but
  `brewpi.py` wires it unconditionally.** `brewpi.py` always does
  `heater.set_on_changed("power_set", tc.set_model_power)` regardless
  of `Controller.pid_type`. Only `temp_controller_smith.py` (the Smith
  predictor) defines `set_model_power`; `temp_controller.py` (`"Normal"`)
  does not, so starting the server with `"pid_type": "Normal"` crashes
  at wiring time with `AttributeError: 'TempController' object has no
  attribute 'set_model_power'`. Either add a no-op `set_model_power` to
  `TempControllerBase`, or only wire it when the configured controller
  actually exposes a model to feed.

- [ ] **Config access is unchecked `dict[key]` everywhere.**
  `params['Hold']`, `params['Td']`, etc., throughout, with no schema
  validation at load time. We already hit this bug class once
  (`config.json.sim`'s `gain`/`Model` nesting mismatch). A small
  schema/dataclass validation layer at config-load would surface
  errors immediately instead of mid-`__init__` — and would have caught
  the dead `TempCtrl.Kalman` / `Model.kn` / `Plant.kn` keys that used
  to sit unused in `config-sim.json.tpl`/`.sim` (since deleted), and the
  `Model.gain`/`Plant.gain` keys that did the same in `config.json.sim`
  before that file was removed entirely — `Pot` dropped `gain`
  entirely, 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.