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

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

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

• 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. 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. 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'], 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.

• 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. 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 for this (closed-loop disturbance/ramp/transition cases) is still outstanding — see the "No automated tests" item above.

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