Add design-flaw backlog for components/pid
Tracks open issues from a review that weren't fixed as part of this branch's Smith-predictor and dedup work: non-configurable FSM thresholds, Pid.scale()'s gain-scheduling hack, shared Kalman tuning across the Smith controller's 3 filters, matplotlib imported at module level in production code, no automated tests, and unchecked config access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,48 @@
|
||||
# components/pid design backlog
|
||||
|
||||
Findings from a design review, not yet acted on. See git history on
|
||||
`claude_refactor` for items already fixed (Smith predictor's delay
|
||||
correction enabled, the two TempController classes deduplicated into
|
||||
`temp_controller_base.py`).
|
||||
|
||||
- [ ] **FSM thresholds aren't configurable.** `tc_constants.py`'s
|
||||
`THRESH_HOLD_IDLE`, `THRESH_HOLD_HEAT`, `THRESH_IDLE_HEAT`,
|
||||
`THRESH_IDLE_HOLD`, `THRESH_HEAT_HOLD`, `THRESH_HEAT_IDLE` are
|
||||
hardcoded module-level constants shared by every controller
|
||||
instance. Move them into `config.json`'s `TempCtrl` section like the
|
||||
PID gains, so mash schedules can tune hysteresis without code edits.
|
||||
|
||||
- [ ] **`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()`.
|
||||
|
||||
- [ ] **Three Kalman filters share one tuning.** In
|
||||
`temp_controller_smith.py`, `kalman_model`, `kalman_model_delay`,
|
||||
and `kalman_plant` are all constructed with the same
|
||||
`params['Kalman']` despite tracking signals with different noise
|
||||
characteristics. Allow independent `var_P`/`var_Q`/`var_R` per
|
||||
filter in config.
|
||||
|
||||
- [ ] **matplotlib imported at module level in production code.**
|
||||
`temp_controller.py`, `temp_controller_smith.py`, and `kalman.py`
|
||||
all `from matplotlib.pyplot import ...` just to support their
|
||||
`__main__` self-test plots, which couples the live server's import
|
||||
graph (and its dependency list) to a GUI plotting library it never
|
||||
uses at runtime. Move the plotting demos into separate scripts.
|
||||
|
||||
- [ ] **No automated tests.** The `__main__` blocks (and
|
||||
`kalman_eval.py`) are manual, eyeballed-plot harnesses. Add
|
||||
regression tests for FSM transitions, anti-windup clamping, and
|
||||
Kalman convergence — this code drives a physical heater.
|
||||
|
||||
- [ ] **Config access is unchecked `dict[key]` everywhere.**
|
||||
`params['Kalman']`, `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__`.
|
||||
Reference in New Issue
Block a user