jensandClaude Sonnet 4.6
4256622e9d
THRESH_HOLD_IDLE/HOLD_HEAT/IDLE_HEAT/IDLE_HOLD/HEAT_HOLD/HEAT_IDLE in tc_constants.py were hardcoded module-level constants shared by every controller instance, so tuning the IDLE/HOLD/HEAT hysteresis required a code change. Replace them with DEFAULT_THRESHOLDS plus an optional TempCtrl.Thresholds config section, merged at construction time in TempControllerBase.__init__ so configs that omit the section keep behaving exactly as before. Documented the new section in config.json.templ; left config.json.sim without it to exercise the default-fallback path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
48 lines
2.4 KiB
Markdown
48 lines
2.4 KiB
Markdown
# 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`).
|
|
|
|
- [x] **FSM thresholds aren't configurable.** Moved into an optional
|
|
`TempCtrl.Thresholds` config section (`HoldIdle`, `HoldHeat`,
|
|
`IdleHeat`, `IdleHold`, `HeatHold`, `HeatIdle`), merged over
|
|
`DEFAULT_THRESHOLDS` in `tc_constants.py` so existing configs without
|
|
the section keep working unchanged.
|
|
|
|
- [ ] **`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__`.
|