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

- [ ] **`Pid.scale()` is a gain-scheduling hack, not anti-windup.**
  `pid.py`'s `scale(k)` multiplies every gain by `1/heatrate_soll_set`
  (called from `temp_controller*.py`'s `process()`), but it's framed
  as overshoot compensation, conflated with the real anti-windup logic
  a few lines below. `self.k` also never resets in `reset()`, so a
  stale scale factor can survive a state-transition reset. Consider
  moving this scheduling logic out of the generic `Pid` class and into
  the temp controller, and resetting `k` in `reset()`. Still present.

- [ ] **No automated tests.** `tests/components/pid/` was added once
  (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.

- [ ] **`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']`, `model_params['gain']`, 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 still
  sitting in `config.json.templ` and `config.json.sim` (harmless today
  since extra keys are just ignored, but nothing reads them anymore
  after the Kalman/`kn` removal — see `components/plant/TODO.md`).

- [ ] **`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.