docs: refresh windup writeup to past tense, link architecture diagram
overshoot_hold_windup.md read like a live plan ("not yet implemented")
even though the fix had already shipped; reworks it into a past-tense
record with corrected line numbers and test descriptions matching what
was actually built. Fixes a stale pid_heat reference in
temp_control_calibration.md.
TODO.md's windup item still claimed test coverage was "outstanding"
from before the test suite was added; corrects that and the "No
automated tests" item above it to reflect current coverage. Both docs
now link docs/fsm_states.png (durable) and the Claude Artifact URL
(session-scoped) for the cascade architecture diagram.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DGQhVQ2Y3yXAQTXhrxVd5u
This commit is contained in:
+23
-11
@@ -32,13 +32,17 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
|
||||
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
|
||||
- [ ] **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, 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.
|
||||
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
|
||||
@@ -81,11 +85,19 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
|
||||
`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.
|
||||
`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.
|
||||
|
||||
- [ ] **`kalman.py` is now dead code in production.** Neither
|
||||
`temp_controller.py` nor `temp_controller_smith.py` uses `Kalman`
|
||||
|
||||
@@ -1,10 +1,12 @@
|
||||
# HOLD-state overshoot from a disturbance: root cause and fix plan
|
||||
# HOLD-state overshoot from a disturbance: root cause and fix
|
||||
|
||||
Investigation of the overshoot visible in `docs/overshoot.png`: a cold-water
|
||||
disturbance during a steady HOLD at 30°C caused `theta_ist` to overshoot to
|
||||
~30.5°C and stay there, rather than settling back at soll. This document
|
||||
records the diagnosis and the (not yet implemented) fix plan. Numbers below
|
||||
are pulled from `logs/log_latest.json`.
|
||||
records the diagnosis, the fix that shipped for it, and the test coverage
|
||||
added alongside it. Numbers in the incident summary below are pulled from
|
||||
`logs/log_latest.json`; names in that section (`pid_hold`/`pid_heat`) are
|
||||
the *pre-fix* names — see "Rename for honesty" below for what they became.
|
||||
|
||||
## Incident summary
|
||||
|
||||
@@ -88,23 +90,24 @@ on which state is currently active.
|
||||
|
||||
### Rename for honesty
|
||||
|
||||
The current names don't match what actually runs when:
|
||||
- `pid_hold` already runs unconditionally *every* tick regardless of state
|
||||
(`process_pid()`'s first line, `temp_controller_base.py:134`) — it's the
|
||||
outer loop, not "the HOLD-state PID". Rename to **`pid_outer`**.
|
||||
- `pid_heat` already runs in *both* `HOLD` and `HEAT` today (only `IDLE`/
|
||||
`COOL` skip it, `temp_controller_base.py:151-158`) — it's the inner loop
|
||||
for the heating direction. Rename to **`pid_inner`**.
|
||||
- `pid_cool` only ever runs in `COOL` — no `HOLD`-time ambiguity, since a
|
||||
The pre-fix names didn't match what actually ran when:
|
||||
- `pid_hold` already ran unconditionally *every* tick regardless of state
|
||||
(`process_pid()`'s first line, `temp_controller_base.py:137`) — it's the
|
||||
outer loop, not "the HOLD-state PID". Renamed to **`pid_outer`**.
|
||||
- `pid_heat` already ran in *both* `HOLD` and `HEAT` (only `IDLE`/`COOL`
|
||||
skip it, `temp_controller_base.py:154-163`) — it's the inner loop for
|
||||
the heating direction. Renamed to **`pid_inner`**.
|
||||
- `pid_cool` only ever ran in `COOL` — no `HOLD`-time ambiguity, since a
|
||||
disturbance that pushes temp *above* soll during `HOLD` is still handled
|
||||
by `pid_inner` (the outer loop's `max(0.0, pid_hold_y)` floor sends
|
||||
by `pid_inner` (the outer loop's `max(0.0, pid_outer_y)` floor sends
|
||||
`heatrate_soll` to 0, and `pid_inner` reacts to the resulting negative
|
||||
`heatrate_err` — the state only escalates to real `COOL` past
|
||||
`Thresholds.HoldCool`). Rename to **`pid_inner_cool`** for symmetry, kept
|
||||
as its own instance — today's explicit `reset()` calls when crossing
|
||||
`Thresholds.HoldCool`). Renamed to **`pid_inner_cool`** for symmetry, kept
|
||||
as its own instance — the explicit `reset()` calls when crossing
|
||||
between heat-direction and cool-direction states
|
||||
(`temp_controller_fsm.py:104,108,115,123`) stay exactly as they are; there
|
||||
is no reason to share integrator state across a heater/chiller boundary.
|
||||
(`temp_controller_fsm.py:104,108,115,123`) stay exactly as they were;
|
||||
there is no reason to share integrator state across a heater/chiller
|
||||
boundary.
|
||||
|
||||
### Config: `Hold`/`Heat`/`Cool` → `Outer` / `Inner.{Heat,Hold,Cool}`
|
||||
|
||||
@@ -131,31 +134,31 @@ The current names don't match what actually runs when:
|
||||
cooling-side incident. `pid_inner_cool` never runs during `HOLD`, so it
|
||||
doesn't need its own `Hold` variant the way `Heat` does; one params block
|
||||
is enough.
|
||||
- **This is a breaking config change** — no backward-compat shim for the
|
||||
- **This was a breaking config change** — no backward-compat shim for the
|
||||
old flat `Hold`/`Heat`/`Cool` keys (per the "no compat hacks" convention).
|
||||
Every deployed `config.json` needs migrating, not just the repo's
|
||||
Every deployed `config.json` needed migrating, not just the repo's
|
||||
`config-real.json.tpl`/`config-sim.json.tpl` templates.
|
||||
|
||||
### Code changes (planned, not yet implemented)
|
||||
### Code changes (implemented)
|
||||
|
||||
1. **`components/pid/pid.py`** — add the symmetric `yi_max` clamp inside
|
||||
`process()` (already planned): `self.yi = max(-yi_max, min(yi_max,
|
||||
self.yi))` when `self.params.get('yi_max')` is set, applied right after
|
||||
1. **`components/pid/pid.py`** — the symmetric `yi_max` clamp lives inside
|
||||
`process()` (line 40): `self.yi = max(-yi_max, min(yi_max, self.yi))`
|
||||
when `self.params.get('yi_max')` is set, applied right after
|
||||
accumulating `yi` and before it's summed into `y`.
|
||||
2. **`components/pid/temp_controller_fsm.py`** — rename `self.pid_hold` →
|
||||
2. **`components/pid/temp_controller_fsm.py`** — `self.pid_hold` →
|
||||
`self.pid_outer`, `self.pid_heat` → `self.pid_inner`, `self.pid_cool` →
|
||||
`self.pid_inner_cool` (constructor at lines 33-34/40, all `reset()` call
|
||||
`self.pid_inner_cool` (constructor at lines 33/34/40, all `reset()` call
|
||||
sites and comments at lines 11-12, 83, 87-89, 99, 104, 108, 111, 115,
|
||||
118-119, 123).
|
||||
119, 123).
|
||||
3. **`components/pid/temp_controller_base.py`**:
|
||||
- `set_params()` (line 28-34): `self.pid_outer.set_params(params['Outer'])`;
|
||||
store `self._inner_heat_params = params['Inner']['Heat']` and
|
||||
- `set_params()` (lines 30-37): `self.pid_outer.set_params(params['Outer'])`;
|
||||
stores `self._inner_heat_params = params['Inner']['Heat']` and
|
||||
`self._inner_hold_params = params['Inner']['Hold']` for the per-tick
|
||||
lookup below; `self.pid_inner_cool.set_params(params['Inner']['Cool'])`.
|
||||
- `process_pid()` (lines 133-160): rename `pid_hold_y` → `pid_outer_y`
|
||||
and the `self.pid_hold.process(...)` call. In the combined `HOLD`/`HEAT`
|
||||
branch (today's `else`, lines 156-158), select the active param set
|
||||
before processing:
|
||||
- `process_pid()` (lines 136-163): `pid_hold_y` → `pid_outer_y`, and the
|
||||
`self.pid_hold.process(...)` call. In the combined `HOLD`/`HEAT`
|
||||
branch (lines 159-163), the active param set is selected before
|
||||
processing:
|
||||
```python
|
||||
else:
|
||||
inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params
|
||||
@@ -165,60 +168,65 @@ The current names don't match what actually runs when:
|
||||
```
|
||||
`set_params()` is a cheap dict-reference assignment (`pid.py:22-23`),
|
||||
so calling it every tick has no meaningful cost. Because `kp`/`ki`/`kd`/
|
||||
`kt` are identical between `Inner.Heat` and `Inner.Hold` in the starting
|
||||
config, switching the active set at a `HOLD↔HEAT` transition changes
|
||||
no term of `y` at that instant — only the `yi_max` ceiling going
|
||||
forward. No bump at the transition, unlike the freeze/thaw approach.
|
||||
`kt` are identical between `Inner.Heat` and `Inner.Hold` in the
|
||||
shipped config, switching the active set at a `HOLD↔HEAT` transition
|
||||
changes no term of `y` at that instant — only the `yi_max` ceiling
|
||||
going forward, with one caveat noted in Status below.
|
||||
4. **Config files** — `config.json`, `config-real.json.tpl`,
|
||||
`config-sim.json.tpl`: restructure into `Outer`/`Inner.{Heat,Hold,Cool}`
|
||||
as above. Inline `"Cool": {...}` dicts in `scripts/demos/pid/
|
||||
demo_temp_controller_smith.py:21`, `demo_temp_controller.py:21`, and
|
||||
`scripts/demos/sud/demo_sud.py:32` need the same restructure.
|
||||
`config-sim.json.tpl` restructured into `Outer`/`Inner.{Heat,Hold,Cool}`
|
||||
as above. The inline `"Cool": {...}` dicts in `scripts/demos/pid/
|
||||
demo_temp_controller_smith.py`, `demo_temp_controller.py`, and
|
||||
`scripts/demos/sud/demo_sud.py` got the same restructure.
|
||||
5. **`utils/replay_sim.py`** — `_apply_gain_overrides()` and the CLI flag
|
||||
loop (lines 50, 214-222) iterate `('Hold','hold'), ('Heat','heat'),
|
||||
('Cool','cool')`; becomes `('Outer','outer'), ('Inner.Heat','inner-heat'),
|
||||
('Inner.Hold','inner-hold'), ('Inner.Cool','inner-cool')` (nested dict
|
||||
access needed since these aren't flat top-level keys any more). The
|
||||
`for section in ('Hold','Heat','Cool')` print loop at line 284 and the
|
||||
`_infer_heatrate_soll_set()` docstring's "hold PID" reference (line 66)
|
||||
need the same rename.
|
||||
6. **`components/pid/TODO.md`** — update the cross-link entry added for the
|
||||
previous (superseded) plan to point at this rewritten section instead.
|
||||
loop now iterate the shared `GAIN_SECTIONS = (('Outer','outer'),
|
||||
('Inner.Heat','inner-heat'), ('Inner.Hold','inner-hold'),
|
||||
('Inner.Cool','inner-cool'))`, with nested dict access for the
|
||||
`Inner.*` entries. The params print loop and
|
||||
`_infer_heatrate_soll_set()`'s docstring were updated to match
|
||||
(`pid_outer.get_y()` instead of "the hold PID").
|
||||
6. **`components/pid/TODO.md`** — the windup entry is marked `[x]` and
|
||||
points at this section.
|
||||
|
||||
## Test plan (not yet implemented)
|
||||
## Tests
|
||||
|
||||
Stdlib `unittest`, no pytest — `tests/components/pid/test_pid.py`,
|
||||
discoverable via `python -m unittest discover -t . -s tests/components/pid`.
|
||||
Simpler than the FSM-gating plan's test plan: no engage/disengage hysteresis
|
||||
or threshold-relationship behavior to cover, since the loop is never turned
|
||||
off — only its `yi_max` ceiling changes with state.
|
||||
Stdlib `unittest`, no pytest — `tests/components/pid/test_pid.py` and
|
||||
`test_temp_controller_closed_loop.py`, discoverable via
|
||||
`python -m unittest discover -t . -s tests/components/pid` (or `-s tests`
|
||||
for the whole repo suite). Simpler than the FSM-gating plan's test plan
|
||||
would have needed: no engage/disengage hysteresis or threshold-relationship
|
||||
behavior to cover, since the loop is never turned off — only its `yi_max`
|
||||
ceiling changes with state.
|
||||
|
||||
**A. Unit-level, isolated `Pid`** — no plant involved. Feed a synthetic
|
||||
`err` sequence shaped like the incident (positive `heatrate_err ≈ 0.3` held
|
||||
for ~130 ticks, then decaying/negative tail, matching the real
|
||||
`rate_soll - rate_ist` pulled from the log) into two `Pid` instances with
|
||||
identical gains, one with `yi_max` set (the `Inner.Hold` case) and one
|
||||
without (`Inner.Heat`). Assert: recovery time (ticks after error goes
|
||||
negative until `y` drops back under a small threshold) is measurably
|
||||
shorter when clamped; `yi` never exceeds the configured bound.
|
||||
**A. Unit-level, isolated `Pid`** (`test_pid.py`) — no plant involved. Feeds
|
||||
a synthetic `err` sequence shaped like the incident (positive
|
||||
`heatrate_err = 0.3` held for 130 ticks, matching the outer loop's real
|
||||
demand during the disturbance, then a flat `-0.3` tail for 200 ticks,
|
||||
matching the real `rate_soll - rate_ist` gap once the outer loop had
|
||||
zeroed its target) into two `Pid` instances with identical gains, one with
|
||||
`yi_max` set (the `Inner.Hold` case) and one without (`Inner.Heat`).
|
||||
Asserts: recovery time (ticks after the error goes negative until `y`
|
||||
drops back under a small threshold) is measurably shorter when clamped;
|
||||
`yi` never exceeds the configured bound; the unclamped instance's `yi`
|
||||
does exceed it (sanity-checks the test itself isn't vacuous).
|
||||
|
||||
**B. Closed-loop, self-contained synthetic scenario** — no dependency on
|
||||
the multi-MB log file. Real numbers from `PlantParams`/`config.json`:
|
||||
`Pot(dt)` with `M=27.96, C=3403.43, L=0.2, Td=17`, ambient from
|
||||
`config.json`; `TempController` via `PidFactory.create('Smith', dt)` with
|
||||
the real `Outer`/`Inner.*` gains. Run closed-loop (controller's own `y`
|
||||
drives the plant, unlike `utils/replay_sim.py`'s open-loop observe-only
|
||||
mode):
|
||||
**B. Closed-loop** (`test_temp_controller_closed_loop.py`) — real `Pot(dt)`
|
||||
plant with `M=27.96, C=3403.43, L=0.2, Td=17`, ambient `20`°C, driven by a
|
||||
real `TempController(Smith)` with the shipped `Outer`/`Inner.*` gains.
|
||||
Controller's own `y` feeds back into the plant each tick (unlike
|
||||
`utils/replay_sim.py`'s open-loop observe-only mode):
|
||||
- **Disturbance case**: hold at 30°C until settled, knock `plant.temp` down
|
||||
~0.5°C to emulate the cold-water event, keep ticking for several minutes.
|
||||
Assert peak overshoot above 30.0°C is measurably smaller with `Inner.Hold`'s
|
||||
`yi_max` set than without.
|
||||
- **Ramp case**: command a genuine 1.5 K/min ramp (state reaches `HEAT`) and
|
||||
assert the sustained heat rate actually reaches ~1.5 K/min — guards against
|
||||
reintroducing the flat-clamp regression from the rejected approach above.
|
||||
- **Transition case**: drive a `HOLD→HEAT→HOLD` sequence and assert `y` has
|
||||
no discontinuity at either transition beyond what the changing `heatrate_err`
|
||||
itself would explain — confirms the per-state param swap is truly bumpless.
|
||||
0.5°C to emulate the cold-water event, keep ticking for 600 more ticks —
|
||||
confirms the state stays in `HOLD` throughout, matching the incident.
|
||||
Asserts peak overshoot above 30.0°C is measurably smaller with
|
||||
`Inner.Hold`'s `yi_max` set than with an unclamped copy of `Inner.Heat`.
|
||||
- **Ramp case**: commands a genuine 1.5 K/min ramp to 40°C (state reaches
|
||||
`HEAT`) and asserts the sustained heat rate actually exceeds 1.4 K/min —
|
||||
guards against reintroducing the flat-clamp regression from the rejected
|
||||
approach above.
|
||||
- **Transition case**: drives a `HOLD→HEAT→HOLD` sequence and asserts the
|
||||
`y` step at either transition stays under `0.1` — see the retroactive-clamp
|
||||
caveat in Status below for why this isn't a stricter "no discontinuity"
|
||||
assertion.
|
||||
|
||||
## Status
|
||||
|
||||
@@ -238,3 +246,12 @@ the `HEAT→HOLD` transition clamps `yi` back down immediately, producing a
|
||||
small (~0.07 in testing, well below the pre-fix disturbance's ~0.74 peak)
|
||||
step in `y` rather than the fully bumpless transfer described above. Not
|
||||
addressed here — flagged for awareness, not a blocker.
|
||||
|
||||
## Architecture diagram
|
||||
|
||||
A full signal-flow diagram of the cascade (`Outer`/`Inner.*` PIDs, FSM
|
||||
state gating, Smith-predictor feedback) exists as a Claude Artifact:
|
||||
https://claude.ai/code/artifact/a32e4752-b4a6-4153-b344-eb2423eb6512 — this
|
||||
is a session-scoped link, not a durable one, so it may not resolve for
|
||||
everyone with repo access. `docs/fsm_states.png` is a static screenshot of
|
||||
just the diagram's FSM-states panel, checked in as the durable copy.
|
||||
|
||||
@@ -143,7 +143,7 @@ Two problems compound:
|
||||
differentiating is the least effective arrangement: noise has already been
|
||||
amplified before the filter sees it.
|
||||
|
||||
The noisy `heatrate_err` feeds into `pid_heat.process()` as the proportional
|
||||
The noisy `heatrate_err` feeds into `pid_inner.process()` as the proportional
|
||||
error. With `kp=0.08`, ±1 K/min rate ripple produces ±8% power output noise.
|
||||
|
||||
### Options
|
||||
|
||||
Reference in New Issue
Block a user