docs: note grounding y_hold_min in passive-cooling capacity in pid TODO

Captures the idea of computing the HOLD-state negative floor from the Smith controller's internal Pot model (L*(theta_ist-theta_amb)/C) instead of the fixed -0.1 heuristic, plus the caveat that the Normal controller has no internal plant model to draw the same estimate from. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE
docs: note active-cooler architecture assessment in pid TODO
2026-07-06 08:30:40 +02:00 · 2026-07-06 08:26:25 +02:00 · 2026-07-06 08:19:10 +02:00 · 2026-07-05 22:08:19 +02:00 · 2026-07-05 21:59:20 +02:00 · 2026-07-05 21:23:55 +02:00
38 changed files with 1407 additions and 241 deletions
@@ -23,39 +23,58 @@ class HeaterHendi(AHeater):
 	def connect(self):
 		ok = self.hendi.connect()
 		self.connected = ok
-		self.firmware_version = self.hendi.sw_ver if ok else None
 		return ok

 	def disconnect(self):
 		self.hendi.disconnect()
 		self.connected = False
-		self.firmware_version = None

 	def activate(self, enable):
 		if not self.connected:
 			return
-		if enable:
-			self.hendi.remoteEnable(1)
-		else:
-			self.hendi.setSwitch(0)
+		try:
+			if enable:
+				self.hendi.remoteEnable(1)
+			else:
+				self.hendi.setSwitch(0)
+		except Exception as e:
+			# A device that was connected can still fail a live command (the
+			# hendi's own idle-timeout auto-suspend, a cable pulled mid-run,
+			# ...) - treat that the same as a disconnect rather than letting
+			# it escape uncaught, since this is called both from HeaterTask's
+			# tick loop (with self.device.open(): activate(True)/(False)) and
+			# from a client's Connect/Disconnect command, neither of which
+			# expect activate() itself to ever raise.
+			print(f"HeaterHendi: comm error in activate({enable}): {e}")
+			self.disconnect()

 	def is_activated(self):
 		if not self.connected:
 			return False
-		s = self.hendi.isRemoteEnable()
-		return '1' in s
+		try:
+			s = self.hendi.isRemoteEnable()
+			return '1' in s
+		except Exception as e:
+			print(f"HeaterHendi: comm error in is_activated: {e}")
+			self.disconnect()
+			return False

 	def process(self):
 		if not self.connected:
 			self.power_eff = 0
 			return
-		power = self.power_set
-		if power == 0:
-			self.hendi.setSwitch(0)
-		else:
-			self.hendi.setSwitch(1)
-		self.hendi.setPowerWatts(power)
-		self.power_eff = self.hendi.getPowerWatts() if power > 0 else 0
+		try:
+			power = self.power_set
+			if power == 0:
+				self.hendi.setSwitch(0)
+			else:
+				self.hendi.setSwitch(1)
+			self.hendi.setPowerWatts(power)
+			self.power_eff = self.hendi.getPowerWatts() if power > 0 else 0
+		except Exception as e:
+			print(f"HeaterHendi: comm error in process: {e}")
+			self.disconnect()
+			self.power_eff = 0

 	def set_power(self, power):
 		self.power_set = min(self.get_power_max(), power)
@@ -213,9 +213,23 @@ class HendiCtrl:
 		if self.ser.is_open:
 			try:
 				self.remoteEnable(False)
-			except HendiException:
+			except Exception:
+				# Not just HendiException (a protocol-level NAK) - a genuine
+				# unplug fails deep inside pyserial itself (termios.error/
+				# OSError from flushInput()/readline() on a dead fd), and
+				# disconnect() must never let that escape: every caller here
+				# (HeaterHendi.process()/activate()'s own except handlers,
+				# AHeater.open()'s context-manager exit) calls disconnect()
+				# specifically to recover from a comm failure, so if this
+				# raises too it propagates all the way out of HeaterTask's
+				# on_process() and permanently kills that task's loop - the
+				# device then never gets process()'d again even after a
+				# later successful reconnect.
+				pass
+			try:
+				self.ser.close()
+			except Exception:
 				pass
-			self.ser.close()
 		self.sw_id = None
 		self.sw_ver = None

@@ -23,13 +23,11 @@ class StirrerPololu1376(AStirrer):
 	def connect(self):
 		ok = self.drv.connect()
 		self.connected = ok
-		self.firmware_version = self.drv.firmware_version if ok else None
 		return ok

 	def disconnect(self):
 		self.drv.disconnect()
 		self.connected = False
-		self.firmware_version = None

 	@contextmanager
 	def remote_open(self):
@@ -47,27 +45,45 @@ class StirrerPololu1376(AStirrer):
 	def activate(self, enable):
 		if not self.connected:
 			return
-		print("activate {}".format(enable))
-		if enable:
-			self.drv.go()
-		else:
-			self.drv.stop()
+		try:
+			print("activate {}".format(enable))
+			if enable:
+				self.drv.go()
+			else:
+				self.drv.stop()
+		except Exception as e:
+			# See HeaterHendi.activate()'s own comment: a device that was
+			# connected can still fail a live command (cable pulled
+			# mid-run, ...) - this is called both from StirrerTask's tick
+			# loop (with self.device.open(): activate(True)/(False)) and
+			# from a client's Connect/Disconnect command, neither of which
+			# expect activate() itself to ever raise.
+			print(f"StirrerPololu1376: comm error in activate({enable}): {e}")
+			self.disconnect()

 	def _on_set_speed(self, speed):
 		if not self.connected:
 			return
-		self.drv.motor_forward(speed)
-		print("Set speed to {} %".format(speed))
+		try:
+			self.drv.motor_forward(speed)
+			print("Set speed to {} %".format(speed))
+		except Exception as e:
+			print(f"StirrerPololu1376: comm error in _on_set_speed: {e}")
+			self.disconnect()

 	def _on_process(self):
 		if not self.connected:
 			return
-		status = self.drv.get_variable(Varid.STATUS)
-		limit_status = self.drv.get_variable(Varid.STATUS_LIMIT_STATUS)
-		if status & 0x01 == 0x01:
-			if limit_status & 0x01 == 0x01:
-				print("Recover after motor error!")
-				self.drv.go()
+		try:
+			status = self.drv.get_variable(Varid.STATUS)
+			limit_status = self.drv.get_variable(Varid.STATUS_LIMIT_STATUS)
+			if status & 0x01 == 0x01:
+				if limit_status & 0x01 == 0x01:
+					print("Recover after motor error!")
+					self.drv.go()
+		except Exception as e:
+			print(f"StirrerPololu1376: comm error in _on_process: {e}")
+			self.disconnect()
 		

 if __name__ == '__main__':
@@ -4,16 +4,15 @@ from utils.value import AttributeChange
 class Connectable(AttributeChange):
 	"""Observable hardware-connection state shared by AHeater/AStirrer.

-	Simulated devices are always connected and report no firmware version;
-	real (serial) devices start disconnected and only flip once connect()
-	actually reaches the hardware.
+	Simulated devices are always connected; real (serial) devices start
+	disconnected and only flip once connect() actually reaches the
+	hardware.
 	"""

 	def __init__(self, simulated):
 		AttributeChange.__init__(self)
 		self.simulated = simulated
 		self.connected = simulated
-		self.firmware_version = None

 	def connect(self):
 		return self.connected
@@ -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
@@ -63,6 +67,121 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
  before that file was removed entirely — `Pot` dropped `gain`
  entirely, see `components/plant/TODO.md`.

+- [x] **`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, 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.
+
+- [x] **HOLD overshoot was a permanent steady-state offset, not just a
+  transient windup.** Found testing the rework against a real Sud run
+  (`sude/sud_0030.json`): a grain-fill-in disturbance during a `HOLD`
+  overshot by ~0.4°C (under the `HoldCool` threshold, so the FSM stayed in
+  `HOLD`) and then never converged back — `temp_ist` sat in a 55.3-55.4°C
+  band for the rest of the 20-minute hold instead of returning to 55.0.
+  Cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` clamped to
+  exactly `0.0`, so once overshot, `heatrate_soll` = 0 ("hold flat") and
+  `pid_inner` actively fought the pot's own ambient loss to keep the
+  overshot temperature flat instead of declining back to setpoint. Fixed:
+  the floor is now a configurable `Outer.y_hold_min` (default `0.0`,
+  backward compatible), set to `-0.1` in `config.json`/both `.tpl`
+  templates, letting HOLD request a gentle decline that roughly matches
+  passive ambient cooling without reopening the `bb5af3c` limit cycle
+  (fully unclamped negative). See the "Follow-up" section in
+  `docs/overshoot_hold_windup.md` and
+  `TestHoldOvershootRecoversToSetpoint` in
+  `tests/components/pid/test_temp_controller_closed_loop.py`.
+
+- [ ] **Architecture isn't ready for a real active cooler yet, only a
+  heat-only actuator.** Asked (not yet built): would the cascade/FSM
+  design still hold up if an active cooler (compressor/chiller, some
+  finite cooling power) replaced "heater off + passive ambient loss"?
+  Mostly yes at the PID/plant-model level: `Pid.y_min`/`y_max` are
+  already symmetric `-1.0`/`1.0` (`pid.py:12-13`), `Pot.set_power()`/
+  `process()` are sign-agnostic so the simulator already models negative
+  power correctly, and the FSM already has a dedicated `COOL` state with
+  its own PID instance (`pid_inner_cool`, `temp_controller_fsm.py:35-39`)
+  whose comment explicitly anticipates this ("the actuator ... is
+  responsible for clamping the resulting negative power to whatever it's
+  actually capable of").
+  The gap is at the actuator boundary: today `y` only ever reaches one
+  consumer, `tasks/heater.py:59`'s `self.power_actor = max(0, ...  * y)`,
+  which silently discards every negative command — that's what's made
+  tuning mismatches free so far. A real cooler needs (a) a second
+  actuator/device class consuming the negative half instead of that
+  `max(0, ...)` clamp — there's no `Cooler`/chiller abstraction anywhere
+  yet (`grep -rniE "cooler|chiller|compressor"` across all `.py` is
+  empty, fully greenfield), and `heater_hendi.py:80`'s `set_power` only
+  clamps to a max, not a min, so feeding it a negative value today would
+  call `setPowerWatts(negative)` on real hardware with undefined result;
+  and (b) its own power ceiling, since one normalized `y` scaled by one
+  `get_power_max()` breaks once heater and cooler have different max
+  power.
+  Biggest risk once that clamp is removed: two separate paths start
+  issuing real negative-power commands that were previously harmless -
+  HOLD's `Outer.y_hold_min` floor (via `Inner.Hold`, see the steady-state
+  overshoot item above) and the pre-existing `pid_inner_cool` (via
+  `Inner.Cool`) - and neither has been tuned against actual active-cooling
+  dynamics (compressor lag, min-on-time). `Inner.Cool` in the demo
+  configs is literally a copy of `Inner.Heat`'s gains, never validated
+  independently. So adding a real cooler is a wiring task (new actuator
+  class, per-actuator power scaling) plus a re-tuning task (`Inner.Cool`,
+  and re-checking the `HoldCool`/`CoolHold` thresholds in
+  `temp_controller_fsm.py:14-17`), not just wiring.
+
+- [ ] **`Outer.y_hold_min` is a fixed heuristic constant, not grounded in
+  the pot's actual passive-cooling capacity.** Related to the active-cooler
+  item above. `Pot.process()` (`components/plant/pot.py:86-89`) already
+  computes `p_loss = L * M * (temp - amb)` unconditionally every tick,
+  independent of `y` - passive cooling isn't actually *driven by* the
+  negative half of `y` today, it's a physics term that happens regardless
+  of what the controller commands. `-0.1` (see the steady-state overshoot
+  fix above) was picked by estimating a typical passive loss rate by hand
+  for one plant/ambient combination; it doesn't adapt to a different pot,
+  water mass, or `theta_ist - theta_amb` delta.
+  `TempController` (Smith, `temp_controller_smith.py:12-16,22-32`) already
+  carries its own internal `Pot` model with `L`/`M`/`C` and ambient
+  temperature set (needed for the Smith prediction) - it could expose a
+  `heatrate_loss_max(theta_ist) = L * (theta_ist - theta_amb) / C * 60`
+  (K/min) from that model and use it to size `y_hold_min` (and bound
+  `pid_inner_cool`'s target) dynamically per-tick, instead of a fixed
+  config constant - the "cooling path" would always ask for exactly what
+  passive loss can actually deliver, no more and no less. Also a natural
+  stepping stone toward the active-cooler item above: same "ceiling"
+  concept, just a bigger number once real cooling power exists.
+  Caveat: `temp_controller.py` ("Normal", non-Smith) has no internal plant
+  model at all - `set_model_plant_params()`/`set_ambient_temperature()`
+  are no-ops in `TempControllerBase` and only overridden in the Smith
+  subclass - so this only works for Smith out of the box; Normal would
+  need either its own lightweight loss estimate (e.g. from observed
+  `heatrate_ist` decay while power is ~0) or keep the fixed fallback.
+
 - [ ] **`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
@@ -35,6 +35,9 @@ class Pid:
        kt = self.params['kt'] * scale

        self.yi = self.yi + ki*dt * err + kt*dt * self.awu
+        yi_max = self.params.get('yi_max')
+        if yi_max is not None:
+            self.yi = max(-yi_max, min(yi_max, self.yi))
        yd = kd/dt*(d - self.d)
        yp = kp * err

@@ -19,6 +19,9 @@ class TempControllerBase(TempControllerFsm, APid):
        # params/set_params() (components/pid/pid.py), whose process()
        # already relies on the same "None means not configured yet".
        self.params = None
+        self._inner_heat_params = None
+        self._inner_hold_params = None
+        self._outer_hold_y_min = 0.0
        self.y = -1
        # Heat-rate pre-filter state (option A) - None until first process() tick
        self.last_theta_ist = None
@@ -29,9 +32,15 @@ class TempControllerBase(TempControllerFsm, APid):
        self.params = params
        self.thresholds = {**DEFAULT_THRESHOLDS, **params.get('Thresholds', {})}
        self.beta = params.get('beta', 0.05)
-        self.pid_hold.set_params(params['Hold'])
-        self.pid_heat.set_params(params['Heat'])
-        self.pid_cool.set_params(params['Cool'])
+        self.pid_outer.set_params(params['Outer'])
+        # How far into "actively cool" pid_outer is allowed to reach during
+        # HOLD - see the floor in process_pid() below. Defaults to 0.0
+        # (old flat-clamp behaviour) so configs that don't set it are
+        # unaffected.
+        self._outer_hold_y_min = params['Outer'].get('y_hold_min', 0.0)
+        self._inner_heat_params = params['Inner']['Heat']
+        self._inner_hold_params = params['Inner']['Hold']
+        self.pid_inner_cool.set_params(params['Inner']['Cool'])

    def _compute_heatrate(self, theta_ist):
        """Pre-filter theta_ist, then differentiate and low-pass to get heatrate_ist.
@@ -131,31 +140,40 @@ class TempControllerBase(TempControllerFsm, APid):
        return self.heatrate_soll_set

    def process_pid(self, theta_err, hold_scale=1.0):
-        self.pid_hold.process(theta_err, -self.theta_ist, hold_scale)
-        # In HOLD state, clamp pid_hold's output to [0, 1]: a small temperature
-        # overshoot makes pid_hold.y go negative, which would invert heatrate_soll
-        # and drive pid_heat's power to 0, causing a limit cycle (power on →
-        # overshoot → power off → coast down → repeat). Clamping to 0 lets the
-        # outer loop reduce the inner setpoint to zero but no further.
-        pid_hold_y = self.pid_hold.get_y()
+        self.pid_outer.process(theta_err, -self.theta_ist, hold_scale)
+        # In HOLD state, floor pid_outer's output at Outer.y_hold_min (<= 0)
+        # rather than letting it swing all the way to -1: a large negative
+        # heatrate_soll asks for a decline far steeper than the pot's own
+        # ambient heat loss can deliver, so pid_inner pins power at 0 for an
+        # extended stretch, undershoots, and swings back hard - a ~110s
+        # limit cycle (see git history for this clamp). A small negative
+        # floor instead lets HOLD ask for a gentle decline that roughly
+        # matches passive cooling, so a small overshoot coasts back down to
+        # setpoint instead of sitting in a permanent flat-clamp dead zone
+        # (the outer loop otherwise commands "stay flat" forever, and
+        # pid_inner actively fights the pot's own ambient loss to hold that
+        # flat line - see docs/overshoot_hold_windup.md).
+        pid_outer_y = self.pid_outer.get_y()
        if self.state == States.HOLD:
-            pid_hold_y = max(0.0, pid_hold_y)
-        self.heatrate_soll = self.heatrate_soll_set * pid_hold_y
+            pid_outer_y = max(self._outer_hold_y_min, pid_outer_y)
+        self.heatrate_soll = self.heatrate_soll_set * pid_outer_y
        heatrate_err = self.heatrate_soll - self.heatrate_ist

        # Only the PID actually driving y is advanced - otherwise the
-        # inactive one (e.g. pid_heat while COOL has pid_cool driving)
+        # inactive one (e.g. pid_inner while COOL has pid_inner_cool driving)
        # would keep silently integrating against a heatrate_err that
        # isn't actually under its control, building a stale windup that
        # causes a discontinuity in y the moment it takes back over.
        if self.state == States.IDLE:
            self.y = 0
        elif self.state == States.COOL:
-            self.pid_cool.process(heatrate_err, -self.heatrate_ist)
-            self.y = self.pid_cool.get_y()
+            self.pid_inner_cool.process(heatrate_err, -self.heatrate_ist)
+            self.y = self.pid_inner_cool.get_y()
        else:
-            self.pid_heat.process(heatrate_err, -self.heatrate_ist)
-            self.y = self.pid_heat.get_y()
+            inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params
+            self.pid_inner.set_params(inner_params)
+            self.pid_inner.process(heatrate_err, -self.heatrate_ist)
+            self.y = self.pid_inner.get_y()

        self.post_pid()

@@ -8,9 +8,9 @@ DEFAULT_THRESHOLDS = {
    # when COOL meant nothing more than going idle - it's an active state
    # with its own PID now, so a threshold this tight relative to a real
    # heater's discrete power steps/sensor noise causes the system to
-    # chatter in and out of it every tick, resetting both pid_hold's and
-    # pid_cool's integrators each time and never letting either actually
-    # converge. Symmetric with the others instead.
+    # chatter in and out of it every tick, resetting both pid_outer's and
+    # pid_inner_cool's integrators each time and never letting either
+    # actually converge. Symmetric with the others instead.
    "HoldCool": 1.0,
    "HeatHold": 1.0,
    "HeatCool": 1.0,
@@ -30,14 +30,14 @@ class States(enum.Enum):
 class TempControllerFsm:

    def __init__(self, dt):
-        self.pid_hold = Pid(dt)
-        self.pid_heat = Pid(dt)
+        self.pid_outer = Pid(dt)
+        self.pid_inner = Pid(dt)
        # Separate gains for ramping down (negative diff) - the actuator
        # (e.g. a heat-only Pot/heater) is responsible for clamping the
        # resulting negative power to whatever it's actually capable of;
        # the controller itself no longer assumes "can't cool" == "must go
        # idle".
-        self.pid_cool = Pid(dt)
+        self.pid_inner_cool = Pid(dt)
        self.thresholds = None
        self.state = States.INIT
        self.is_startup = True
@@ -80,13 +80,13 @@ class TempControllerFsm:
            # which calls this method directly), so an unconditional HOLD
            # here gets mistaken for "ramp already reached" and can finish
            # a freshly (re-)started step instantly - see SudTask.
-            # on_process()'s is_holding() check. pid_heat/pid_cool were
-            # frozen (see process_pid()) and possibly stale for as long as
-            # we were disabled - start whichever one matters clean rather
+            # on_process()'s is_holding() check. pid_inner/pid_inner_cool
+            # were frozen (see process_pid()) and possibly stale for as long
+            # as we were disabled - start whichever one matters clean rather
            # than resuming wherever it last left off.
-            self.pid_hold.reset()
-            self.pid_heat.reset()
-            self.pid_cool.reset()
+            self.pid_outer.reset()
+            self.pid_inner.reset()
+            self.pid_inner_cool.reset()
            if diff >= self.thresholds['HoldHeat']:
                state_next = States.HEAT
            elif diff <= -self.thresholds['HoldCool']:
@@ -96,31 +96,31 @@ class TempControllerFsm:
        elif self.state == States.HOLD:
            if diff >= self.thresholds['HoldHeat']:
                state_next = States.HEAT
-                # No pid_heat.reset() here — bumpless transfer: carry the
+                # No pid_inner.reset() here — bumpless transfer: carry the
                # hold-phase integral into the new ramp so power doesn't
                # drop to near-zero and crawl back up from scratch.
            elif diff <= -self.thresholds['HoldCool']:
                state_next = States.COOL
-                self.pid_cool.reset()
+                self.pid_inner_cool.reset()
        elif self.state == States.HEAT:
            if diff <= -self.thresholds['HeatCool']:
                state_next = States.COOL
-                self.pid_cool.reset()
+                self.pid_inner_cool.reset()
            elif diff <= self.thresholds['HeatHold']:
                state_next = States.HOLD
-                self.pid_hold.reset()
+                self.pid_outer.reset()
        elif self.state == States.COOL:
            if diff >= self.thresholds['CoolHeat']:
                state_next = States.HEAT
-                self.pid_heat.reset()
+                self.pid_inner.reset()
            elif diff >= -self.thresholds['CoolHold']:
                state_next = States.HOLD
-                self.pid_hold.reset()
-                # pid_heat was frozen during COOL (see process_pid()) -
+                self.pid_outer.reset()
+                # pid_inner was frozen during COOL (see process_pid()) -
                # resume it clean rather than from whatever it last held
                # before COOL took over, which by now may be a stale fit
                # for a completely different part of the curve.
-                self.pid_heat.reset()
+                self.pid_inner.reset()

        if state_next != self.state:
            self.state = state_next
@@ -10,14 +10,54 @@ class TempSensor_max31865(ATemperatureSensor):

 	def __init__(self, temp_offset=0, **kwargs):
 		ATemperatureSensor.__init__(self)
-		# Open SPI bus
+		self.temp_offset = temp_offset
+		# Set before the first read attempt so a read failure even on the
+		# very first call (temperature() below, or TempSensorTask's own
+		# priming call) has a well-defined last-known-good value to fall
+		# back to rather than raising AttributeError.
+		self.temp = None
 		self.spi = spidev.SpiDev()
+		try:
+			self._open_spi()
+		except Exception as e:
+			# Don't crash the whole server if the SPI bus is momentarily
+			# busy/unavailable at startup (e.g. right after a Pi reboot) -
+			# temperature()'s own except handler already retries via
+			# reopen() on the next tick, same recovery path as a later
+			# runtime failure.
+			print(f"TempSensor_max31865: could not open SPI at startup: {e}")
+
+	def _open_spi(self):
 		self.spi.open(0, 0)
 		self.spi.max_speed_hz = 50000
 		self.spi.mode = 0b01
-		self.temp_offset = temp_offset
 		self.write_reg(0x00, 0xA3)

+	def reopen(self):
+		"""Closes and reopens the SPI handle - recovers from a wedged bus/
+		chip-select state (e.g. left mid-transaction by some other fault)
+		without needing a full process or Pi reboot. There's no serial-
+		style connect/disconnect concept for a bus peripheral like this
+		one, so this is the closest equivalent - called automatically by
+		temperature() below on a read failure.
+
+		Never raises - it's itself the recovery path called from
+		temperature()'s except handler, and if it raised too that would
+		escape temperature() entirely (same class of bug as HendiCtrl.
+		disconnect() previously letting a comm failure escape and
+		permanently kill HeaterTask's coroutine - see tasks/heater.py's
+		on_process()). A genuinely still-broken bus just leaves self.temp
+		at its last-known-good value, logged, and gets retried again next
+		tick."""
+		try:
+			self.spi.close()
+		except Exception:
+			pass
+		try:
+			self._open_spi()
+		except Exception as e:
+			print(f"TempSensor_max31865: reopen failed: {e}")
+
 	def read_reg(self, addr):
 		reg = self.spi.xfer([addr, 0xFF])

@@ -46,8 +86,20 @@ class TempSensor_max31865(ATemperatureSensor):


 	def temperature(self):
-		digits = self.read_digits()
-		self.temp = self.temp_offset + self.to_temperature(digits)
+		try:
+			digits = self.read_digits()
+			self.temp = self.temp_offset + self.to_temperature(digits)
+		except Exception as e:
+			# Holds the last-known-good self.temp rather than raising -
+			# TempSensorTask.on_process() calls this every tick with no
+			# guard of its own (mirrors HeaterHendi.process()'s pattern of
+			# self-healing at the actor level rather than letting a comm
+			# failure escape and permanently kill the task's coroutine -
+			# nothing restarts a dead ATask). reopen() gives the SPI bus a
+			# chance to recover from a wedged state without a full process/
+			# Pi reboot - see docs/pi_deployment_notes.md.
+			print(f"TempSensor_max31865: comm error reading sensor, reopening SPI: {e}")
+			self.reopen()
 		return self.temp

 	@staticmethod
@@ -46,7 +46,7 @@ class SudForecastEstimator:
 	estimate() also duty-cycles the PID's continuous output across the
 	heater's own discrete power steps (see its actuate() closure), exactly
 	like the real run's HeaterTask/device chain does - feeding tc.get_power()
-	straight to the plant instead lets pid_heat's own oscillatory tendency
+	straight to the plant instead lets pid_inner's own oscillatory tendency
 	reach the plant undamped, producing a forecast far more jagged than any
 	real run actually is (the discrete steps end up duty-cycling it back
 	down to something close to the commanded average)."""
@@ -4,23 +4,33 @@
 	"TempCtrl": {
 		"pid_type": "Smith",
 		"beta": 0.05,
-		"Hold": {
+		"Outer": {
 			"kp": 0.4,
 			"ki": 0.0,
 			"kd": 0.0,
-			"kt": 0.0
+			"kt": 0.0,
+			"y_hold_min": -0.1
 		},
-		"Heat": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
-		},
-		"Cool": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
+		"Inner": {
+			"Heat": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			},
+			"Hold": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5,
+				"yi_max": 0.3
+			},
+			"Cool": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			}
 		},
 		"Thresholds": {
 			"HoldHeat": 1.0,
@@ -4,23 +4,33 @@
 	"TempCtrl": {
 		"pid_type": "Smith",
 		"beta": 0.05,
-		"Hold": {
+		"Outer": {
 			"kp": 0.4,
 			"ki": 0.0,
 			"kd": 0.0,
-			"kt": 0.0
+			"kt": 0.0,
+			"y_hold_min": -0.1
 		},
-		"Heat": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
-		},
-		"Cool": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
+		"Inner": {
+			"Heat": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			},
+			"Hold": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5,
+				"yi_max": 0.3
+			},
+			"Cool": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			}
 		},
 		"Thresholds": {
 			"HoldHeat": 1.0,
@@ -0,0 +1,35 @@
+# Claude FAQ
+
+Notes on how Claude Code works with this repo, for later reference.
+
+## Does Claude store a separate memory/context for planning work like the overshoot investigation, or is the TODO enough?
+
+**Q:** After a long session diagnosing the HOLD-state overshoot and writing
+up the fix plan in `docs/overshoot_hold_windup.md` / `components/pid/TODO.md`,
+I asked: did you also store a context and/or memory for easier retrieval
+later, or is the TODO enough?
+
+**A:** No separate memory was created for the plan's *content* — that lives
+in the committed docs (`docs/overshoot_hold_windup.md` +
+`components/pid/TODO.md`), which is strictly better than Claude's own memory
+system for this: it's version-controlled, visible to anyone with repo
+access, and gets updated in place as the real work happens. Claude's memory
+system explicitly excludes storing "architecture/file paths/design content
+already derivable by reading the project" — duplicating a plan into memory
+would just be a second, decaying copy of the same facts.
+
+What Claude *does* keep in memory is a **pointer**, not the content: an
+existing `pid_design_backlog.md` memory says (in effect) "check
+`components/pid/TODO.md` before starting PID work." That pointer was 15
+days stale at the time and had a couple of now-wrong claims (e.g. it said
+the three Kalman filters sharing one tuning was "still open", but
+`temp_controller_smith.py` had since dropped Kalman filtering entirely) —
+so it was refreshed in the same turn, with one line added noting this
+session's windup investigation exists and where to find it, without
+copying the investigation's content into memory.
+
+**Takeaway:** for durable project knowledge that belongs in the repo
+(design decisions, investigation writeups, TODOs), the repo docs *are* the
+memory — Claude's own memory system is reserved for cross-session pointers,
+preferences, and facts that aren't otherwise written down anywhere in the
+project.
@@ -0,0 +1,311 @@
+# 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, 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
+
+- Steady HOLD at 30.0°C. Cold water added → `theta_ist` (Smith-corrected)
+  dipped to ~29.49°C (`diff ≈ 0.51°C`).
+- `diff` never reached `Thresholds.HoldHeat` (1.0°C, `temp_controller_fsm.py`),
+  so **the FSM never left `HOLD`** — the entire episode happened inside what
+  the system still considered "holding".
+- The outer loop (`pid_hold`, `Hold: {kp:0.6, ki:0, kd:0, kt:0}` — pure
+  proportional) tracked `0.6 * max(0, diff)` exactly, tick for tick, peaking
+  at `y ≈ 0.31` — nowhere near its `y_max=1.0` ceiling. Confirmed directly
+  against logged samples: `rate_soll` == `0.6*diff` to the last digit
+  throughout.
+- The inner loop (`pid_heat`, `Heat: {kp:0.08, ki:0.02, kd:0, kt:1.5}`) then
+  chased that `heatrate_soll` target, driving commanded power from a
+  baseline ~500W burst-cycle up to **~2600W** (`y ≈ 0.74` of the 3500W max,
+  see `tasks/heater.py:58-59`: `power = get_power_max() * y`).
+- Plant dead time (`Td=17s`) and thermal mass (`M=27.96 kg`,
+  `C≈3403 J/(kg·K)`, from `PlantParams` in the log) meant heat from that
+  burst kept arriving after `theta_ist` had already crossed back over soll.
+  By the time `rate_soll` had fallen back to exactly `0` (outer loop asking
+  for zero heat rate), `power_set` was still **1442 W**, decaying only
+  gradually over the next ~35s (1193 → 979 → 817 → 632 → 487 → 337 → 142 W)
+  rather than snapping to 0.
+- No active cooling actuator exists; the passive loss coefficient
+  (`L=0.2 W/(kg·K)`) gives a time constant `C/L ≈ 17,000s (~4.7h)`, so the
+  resulting ~0.4°C plateau above soll does not visibly decay within any
+  reasonable observation window.
+
+**Root cause:** the inner loop's integral term (`yi`) accumulated during
+the ~130s the outer loop demanded a real (if modest) heat rate, and nothing
+bounded that accumulation — the existing anti-windup in `Pid.process()`
+(`components/pid/pid.py:45-48`) is back-calculation that only corrects `yi`
+when the combined output `y` is actually clamped at `y_min`/`y_max`. Since
+`y` peaked at ~0.74, well under the `1.0` ceiling, `awu` was `0` the entire
+time and the anti-windup mechanism never engaged. The integrator just had to
+unwind naturally against real (delayed) negative error, which took ~35+
+seconds after the outer loop had already zeroed its target.
+
+## Rejected approach: a flat `yi_max` clamp on the inner loop
+
+The first fix considered was bounding `yi` directly, with one constant for
+the inner loop regardless of state. Numeric check against the real plant
+model kills this as a standalone fix:
+
+| scenario | P needed | y needed |
+|---|---|---|
+| 1.5 K/min ramp @ 30°C | 2435 W | 0.696 |
+| 1.5 K/min ramp @ 50°C | 2547 W | 0.728 |
+| 1.5 K/min ramp @ 66°C | 2636 W | 0.753 |
+| the actual HOLD disturbance (peak) | 2600 W | 0.74 |
+| steady HOLD loss compensation @ 66°C | ~257 W | ~0.073 |
+
+At steady state (`err=0`), `kp*err` contributes nothing — the entire
+`y≈0.7-0.75` needed to sustain a genuine 1.5 K/min ramp has to come from
+`yi` alone, for as long as the ramp lasts. That's the *same* range the
+disturbance transient itself peaked at, but far above what real steady-HOLD
+loss compensation ever needs (~0.07-0.15 at realistic brew temperatures). A
+single constant can't serve both: tight enough to matter for the disturbance
+(well under ~0.74) permanently starves a real ramp of the ~0.7 it needs;
+loose enough not to interfere with ramps (~0.75+) never engages during the
+disturbance at all. **Rejected as a single global constant.**
+
+An FSM-gating alternative (freeze the inner loop's output to 0 while in
+`HOLD` unless explicitly engaged by command or a new threshold) was also
+sketched, but left an open question about how a new engage threshold should
+relate to the existing `HoldHeat` FSM threshold, and needed extra
+engage/disengage hysteresis to avoid chatter. **Superseded** by the plan
+below, which reaches the same effect with less new machinery.
+
+## Refined plan: split the clamp by *which state is driving the same loop*,
+## not by freezing the loop
+
+The insight above — real HOLD-time demand (~0.07-0.15) and real ramp demand
+(~0.7-0.75) occupy clearly different ranges — means a **per-state parameter
+set** on the *same* PID instance solves this more cleanly than gating the
+loop on/off: same accumulated `yi`/`d` state carried across `HOLD↔HEAT`
+transitions (bumpless transfer for free, no explicit reset, no engage/
+disengage hysteresis to tune), just a different `yi_max` ceiling depending
+on which state is currently active.
+
+### Rename for honesty
+
+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_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`). 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 were;
+  there is no reason to share integrator state across a heater/chiller
+  boundary.
+
+### Config: `Hold`/`Heat`/`Cool` → `Outer` / `Inner.{Heat,Hold,Cool}`
+
+```json
+"TempCtrl": {
+  "pid_type": "Smith",
+  "beta": 0.9,
+  "Outer": { "kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0 },
+  "Inner": {
+    "Heat": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5 },
+    "Hold": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5, "yi_max": 0.3 },
+    "Cool": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5 }
+  },
+  "Thresholds": { "...": "unchanged" }
+}
+```
+- `Inner.Heat` keeps today's `Heat` gains, no `yi_max` (or a very loose one)
+  — a real ramp must be able to reach `y≈0.75`.
+- `Inner.Hold` starts as a copy of the same gains, with `yi_max≈0.2-0.3`
+  added — comfortably above realistic steady-loss compensation (~0.07-0.15)
+  but well below what turned a 0.5°C dip into a 2600W burst.
+- `Inner.Cool` is `Cool`'s existing gains, moved under `Inner` purely for
+  structural consistency — introduced now, not because we've observed a
+  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 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` needed migrating, not just the repo's
+  `config-real.json.tpl`/`config-sim.json.tpl` templates.
+
+### Code changes (implemented)
+
+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`** — `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
+   sites and comments at lines 11-12, 83, 87-89, 99, 104, 108, 111, 115,
+   119, 123).
+3. **`components/pid/temp_controller_base.py`**:
+   - `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 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
+         self.pid_inner.set_params(inner_params)
+         self.pid_inner.process(heatrate_err, -self.heatrate_ist)
+         self.y = self.pid_inner.get_y()
+     ```
+     `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
+     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` 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 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.
+
+## Tests
+
+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`** (`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** (`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 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
+
+Implemented: `pid.py`'s `yi_max` clamp, the `pid_outer`/`pid_inner`/
+`pid_inner_cool` rename, the `Outer`/`Inner.{Heat,Hold,Cool}` config
+restructure (`config.json`, both `.tpl` templates, and the three demo
+scripts), and `utils/replay_sim.py`'s matching CLI-flag/print-loop rename.
+Tests added under `tests/components/pid/` (`test_pid.py` for the isolated
+`Pid` clamp behavior, `test_temp_controller_closed_loop.py` for the
+closed-loop disturbance/ramp/transition cases) — all passing.
+
+One subtlety found while writing the closed-loop transition test that
+this plan didn't anticipate: `Inner.Hold`'s `yi_max` clamp applies
+retroactively. If a sustained `HEAT` ramp pushes `yi` above the `Hold`
+ceiling before the `HeatHold` threshold fires, the very next tick after
+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.
+
+## Follow-up: steady-state overshoot in HOLD (`Outer.y_hold_min`)
+
+Found while testing the rework against a real Sud run (`sude/sud_0030.json`,
+`logs/log_20260706T074658_Sud-0030.json`). Visible in `docs/overshoot2.png`
+(`theta_ist` vs `theta_soll` across the run): at both the 55°C and 63°C
+rests, `theta_ist` overshoots the step and then plateaus above `theta_soll`
+for the rest of the hold instead of converging back down — most clearly at
+the first rest, where it settles at ~55.5-55.6°C against a 55.0°C target.
+
+A grain-fill-in disturbance during "1. Rast" (mash-in rest, `HOLD` at 55°C)
+pushed `temp_ist` to a ~0.4°C overshoot — well under `HoldCool`'s 1.0
+threshold, so the FSM stayed in `HOLD` throughout, same as the transient
+windup case above. But this overshoot never decayed: `temp_ist` sat in a
+55.30-55.44 band for the rest of the 20-minute hold instead of converging
+back to 55.0. Distinct failure mode from the transient windup fixed above
+(that one unwound over ~35s; this one was flat/permanent for as long as the
+hold lasted).
+
+Root cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` (added in
+`bb5af3c` to break a limit cycle - see the History section) clamped to
+exactly `0.0`. Once `temp_ist > temp_soll`, that floor forces
+`heatrate_soll = 0`, i.e. "hold flat" - and `pid_inner` then actively fights
+the pot's own ambient heat loss to keep the *overshot* temperature flat,
+rather than being allowed to request a genuine decline back toward
+setpoint. With `Outer.ki = 0`, there's no integral action to null the
+resulting steady-state error any other way, so the offset persists for the
+rest of the hold.
+
+Fix: replaced the hardcoded `0.0` floor with a configurable
+`Outer.y_hold_min` (default `0.0`, so configs that don't set it keep the old
+behavior), set to `-0.1` in `config.json`, both `.tpl` templates, and the
+three demo scripts. A small negative floor lets HOLD ask for a gentle
+decline that roughly matches
+passive ambient cooling, rather than the fully unclamped `[-1, 1]` range
+that caused the original limit cycle (a large negative `heatrate_soll` asks
+for a decline steeper than passive loss can deliver, pinning power at 0 for
+an extended stretch and producing a hard undershoot/rebound). Test coverage
+added: `TestHoldOvershootRecoversToSetpoint` in
+`tests/components/pid/test_temp_controller_closed_loop.py` reproduces the
+sud_0030 disturbance, asserts the old flat-clamp behavior still fails to
+recover (regression guard) and the new floor converges close to setpoint,
+plus a guard that the recovery doesn't undershoot by more than the injected
+disturbance itself (i.e. doesn't reintroduce the `bb5af3c` limit cycle).
+
+`-0.1` was chosen from the real Sud's plant params (`Pot.mass=5.96` +
+`water_mass=22` ≈ the test harness's `M=27.96`, `L=0.2`): passive ambient
+loss at a ~35°C delta works out to roughly 0.1-0.15 K/min, so
+`heatrate_soll_set * -0.1` lands in that same ballpark for a typical
+`heatrate_soll_set` of ~1.0-1.5 K/min. Not derived from first-principles
+tuning - may need adjustment per installation, same as the other PID gains.
+
+Confirmed against a live re-run of `sud_0030` with the fix applied, not just
+the unit test above.
+
+## 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.
@@ -0,0 +1,25 @@
+# brewpi Pi deployment notes
+
+`config.json` is gitignored (`.gitignore:6`) - it's generated per-deployment
+from `config-real.json.tpl`/`config-sim.json.tpl`, not tracked in git, so
+notes about its actual on-Pi content have to live here instead.
+
+## Heater/Stirrer `port` should use `/dev/serial/by-id/...`, not `/dev/ttyUSB0`/`/dev/ttyACM0`
+
+Found 2026-07-03: unplugging the Hendi mid-brew (to test the connect/
+disconnect feature, and the crash-recovery fix that followed) caused both
+USB-serial adapters to renumber on replug - `/dev/ttyUSB0` became
+`/dev/ttyUSB1`, `/dev/ttyACM0` became `/dev/ttyACM1`. `config.json` still
+pointed at the old raw device paths, so a plain reconnect kept failing
+(wrong port, not a code bug) until `config.json` was updated to the stable
+`/dev/serial/by-id/...` symlinks udev already provides on this Pi:
+
+```
+Heater.port:  /dev/serial/by-id/usb-JDI_Hendi-Control_AM3VLPVE-if00-port0
+Stirrer.port: /dev/serial/by-id/usb-Pololu_Corporation_Pololu_Simple_High-Power_Motor_Controller_18v15_53FF-6F06-7277-5049-3950-0767-if00
+```
+
+Since raw `/dev/ttyUSB*`/`/dev/ttyACM*` numbering isn't stable across
+replugs (and a real unplug is exactly the scenario the connect/disconnect
+feature exists to handle gracefully), `config.json` should keep using the
+by-id paths above rather than reverting to `/dev/ttyUSB0`/`/dev/ttyACM0`.
@@ -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
@@ -6,23 +6,33 @@ from components.pid.temp_controller import TempController

 if __name__ == '__main__':
 	ctrl_params = {
-		"Hold": {
+		"Outer": {
 			"kp": 0.4,
 			"ki": 0.0,
 			"kd": 0.0,
-			"kt": 0.0
+			"kt": 0.0,
+			"y_hold_min": -0.1
 		},
-		"Heat": {
-			"kp": 0.08,
-			"ki": 0.008,
-			"kd": 0.0,
-			"kt": 1.5
-		},
-		"Cool": {
-			"kp": 0.08,
-			"ki": 0.008,
-			"kd": 0.0,
-			"kt": 1.5
+		"Inner": {
+			"Heat": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5
+			},
+			"Hold": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5,
+				"yi_max": 0.3
+			},
+			"Cool": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5
+			}
 		}
 	}

@@ -6,23 +6,33 @@ from components.pid.temp_controller_smith import TempController

 if __name__ == '__main__':
 	ctrl_params = {
-		"Hold": {
+		"Outer": {
 			"kp": 0.4,
 			"ki": 0.0,
 			"kd": 0.0,
-			"kt": 0.0
+			"kt": 0.0,
+			"y_hold_min": -0.1
 		},
-		"Heat": {
-			"kp": 0.08,
-			"ki": 0.008,
-			"kd": 0.0,
-			"kt": 1.5
-		},
-		"Cool": {
-			"kp": 0.08,
-			"ki": 0.008,
-			"kd": 0.0,
-			"kt": 1.5
+		"Inner": {
+			"Heat": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5
+			},
+			"Hold": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5,
+				"yi_max": 0.3
+			},
+			"Cool": {
+				"kp": 0.08,
+				"ki": 0.008,
+				"kd": 0.0,
+				"kt": 1.5
+			}
 		}
 	}

@@ -17,23 +17,33 @@ if __name__ == '__main__':
 	theta_amb = 20

 	ctrl_params = {
-		"Hold": {
+		"Outer": {
 			"kp": 0.4,
 			"ki": 0.0,
 			"kd": 0.0,
-			"kt": 0.0
+			"kt": 0.0,
+			"y_hold_min": -0.1
 		},
-		"Heat": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
-		},
-		"Cool": {
-			"kp": 0.08,
-			"ki": 0.02,
-			"kd": 0.0,
-			"kt": 1.5
+		"Inner": {
+			"Heat": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			},
+			"Hold": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5,
+				"yi_max": 0.3
+			},
+			"Cool": {
+				"kp": 0.08,
+				"ki": 0.02,
+				"kd": 0.0,
+				"kt": 1.5
+			}
 		}
 	}

@@ -0,0 +1,119 @@
+{
+	"Name": "Sud-0030",
+	"Description": "Münchner Hell",
+	"pot": {
+		"grain_mass": 0,
+		"water_mass": 22,
+		"volumen": 30
+	},
+	"default": {
+		"step": {
+			"descr": "Put description here",
+			"user_message": "Put user message here",
+			"user_wait_for_continue": false,
+			"pot": {
+				"grain_mass": 5.21,
+				"water_mass": 22
+			},
+			"temperature": 0,
+			"ramp": {
+				"rate": 1.0,
+				"stirrer": {
+					"speed": 50,
+					"interval_time": 0,
+					"on_ratio": 1.0
+				}
+			},
+			"hold": {
+				"duration": 0,
+				"stirrer": {
+					"speed": 30,
+					"interval_time": 60,
+					"on_ratio": 0.5
+				}
+			}
+		}
+	},
+	"steps": [
+		{
+			"pot": {
+				"grain_mass": 0
+			},
+			"descr": "Aufheizen",
+			"temperature": 57,
+			"ramp": {
+				"rate": 1.5,
+				"stirrer": {
+					"speed": 75
+				}
+			}
+		},
+		{
+			"pot": {
+				"grain_mass": 0
+			},
+			"descr": "Einmaischen",
+			"user_message": "Bitte Malz einfüllen und bestätigen",
+			"user_wait_for_continue": true,
+			"hold": {
+				"stirrer": {
+					"speed": 0
+				}
+			}
+		},
+		{
+			"descr": "1. Rast",
+			"temperature": 55,
+			"hold": {
+				"duration": 20
+			}
+		},
+		{
+			"descr": "Glucose-Rast",
+			"temperature": 63,
+			"ramp": {
+				"rate": 1.0
+			},
+			"hold": {
+				"duration": 40,
+				"stirrer": {
+					"speed": 20,
+					"interval_time": 90,
+					"on_ratio": 0.8
+				}
+			}
+		},
+		{
+			"descr": "Verzuckerungs-Rast",
+			"temperature": 72,
+			"ramp": {
+				"rate": 1.0
+			},
+			"hold": {
+				"duration": 30,
+				"stirrer": {
+					"speed": 35,
+					"interval_time": 120
+				}
+			}
+		},
+		{
+			"pot": {
+				"water_mass": 20
+			},
+			"descr": "Abmaischen",
+			"user_message": "Quittieren zum Abmaischen",
+			"user_wait_for_continue": true,
+			"temperature": 76,
+			"ramp": {
+				"rate": 1.0
+			},
+			"hold": {
+				"duration": 30,
+				"stirrer": {
+					"speed": 50
+				}
+			}
+		}
+	]
+}
@@ -1,4 +1,4 @@
-from tasks.task import ATask, TaskManager
+from tasks.task import ATask, TaskManager, fire_and_forget
 from tasks.tempsensor import TempSensorTask
 from tasks.heater import HeaterTask
 from tasks.stirrer import StirrerTask
@@ -1,5 +1,5 @@
 import asyncio
-from tasks import ATask
+from tasks import ATask, fire_and_forget
 from components import AHeater
 from ws.message import MsgIo
 from utils.value import ChangedInteger
@@ -21,7 +21,6 @@ class HeaterTask(ATask):
 		self._on_connected_changed = None
 		device.set_on_changed('power_eff', ChangedInteger(self.on_changed_power).set)
 		device.set_on_changed('connected', self.on_connected_changed)
-		device.set_on_changed('firmware_version', self.on_firmware_version_changed)
 		self.power_set_changed = ChangedInteger(self.on_changed_power_set).set

 	def set_on_closed_loop_changed(self, callback):
@@ -34,13 +33,10 @@ class HeaterTask(ATask):
 		self._on_connected_changed = callback

 	def on_connected_changed(self, value):
-		asyncio.create_task(self.send({'Connected': value}))
+		fire_and_forget(self.send({'Connected': value}))
 		if self._on_connected_changed:
 			self._on_connected_changed(value)

-	def on_firmware_version_changed(self, value):
-		asyncio.create_task(self.send({'FirmwareVersion': value}))
-
 	def shutdown(self):
 		"""Called when the brew ends (DONE/IDLE). Switches to open-loop at
 		power 0 and disables TC, mirroring what a Stop press should do."""
@@ -83,6 +79,8 @@ class HeaterTask(ATask):
 			elif pair[0] == 'Disconnect':
 				self.device.activate(False)
 				self.device.disconnect()
+				self.power_soll = 0
+				self.power_set_changed(0)
 			elif 'Power' in pair[0]:
 				self.power_soll = pair[1]

@@ -125,37 +123,58 @@ class HeaterTask(ATask):
 		# fire and a freshly-subscribed client would never learn it's
 		# connected.
 		await self.send({'Connected': self.device.connected})
-		await self.send({'FirmwareVersion': self.device.firmware_version})

-		with self.device.open():
-			while True:
-				# Closed-loop: TC has full control. Open-loop: direct manual power only.
-				power_soll = self.power_actor if self.closed_loop else self.power_soll
-				self.power_set_changed(power_soll)
+		# Outer retry loop - belt-and-suspenders around the tick loop's own
+		# per-call guards below. A comm failure a device-level guard didn't
+		# anticipate (e.g. a raw pyserial/termios exception escaping from
+		# somewhere other than device.process(), the one call already
+		# wrapped below) must never be allowed to fall out of this
+		# coroutine entirely: nothing restarts a dead ATask, so a single
+		# uncaught exception here would permanently stop this device from
+		# ever being process()'d again - even after a later, otherwise-
+		# successful reconnect. See the incident this was added for: a
+		# mid-brew unplug raised termios.error from deep inside
+		# HendiCtrl.disconnect() itself (fixed there too), which escaped
+		# all the way out of this loop and killed it for the rest of the
+		# process's life.
+		while True:
+			try:
+				with self.device.open():
+					while True:
+						# Closed-loop: TC has full control. Open-loop: direct manual power only.
+						power_soll = self.power_actor if self.closed_loop else self.power_soll
+						self.power_set_changed(power_soll)

-				# Calculate duty cycle
-				power_low = self.get_next_smaller_power(power_soll)
-				power_high = self.get_next_greater_power(power_soll)
-				power_step = power_high - power_low
-				power_diff = power_soll - power_low
-				duty = power_diff / power_step
+						# Calculate duty cycle
+						power_low = self.get_next_smaller_power(power_soll)
+						power_high = self.get_next_greater_power(power_soll)
+						power_step = power_high - power_low
+						power_diff = power_soll - power_low
+						duty = power_diff / power_step

-				on_count = pulse_period_count*duty
+						on_count = pulse_period_count*duty

-				self.pulse_counter += 1
-				if self.pulse_counter >= pulse_period_count:
-					self.pulse_counter = 0
+						self.pulse_counter += 1
+						if self.pulse_counter >= pulse_period_count:
+							self.pulse_counter = 0

-				if power_soll == 0 or self.pulse_counter >= on_count:
-					self.device.set_power(power_low)
-				elif self.pulse_counter < on_count:
-					self.device.set_power(power_high)
+						if power_soll == 0 or self.pulse_counter >= on_count:
+							self.device.set_power(power_low)
+						elif self.pulse_counter < on_count:
+							self.device.set_power(power_high)

+						try:
+							self.device.process()
+						except Exception as e:
+							print(f"HeaterTask: comm error, marking disconnected: {e}")
+							self.device.disconnect()
+						await asyncio.sleep(self.interval)
+			except Exception as e:
+				print(f"HeaterTask: unexpected error, recovering: {e}")
 				try:
-					self.device.process()
-				except Exception as e:
-					print(f"HeaterTask: comm error, marking disconnected: {e}")
 					self.device.disconnect()
+				except Exception:
+					pass
 				await asyncio.sleep(self.interval)


@@ -1,5 +1,5 @@
 import asyncio
-from tasks import ATask
+from tasks import ATask, fire_and_forget
 from ws.message import MsgIo
 from components import AStirrer
 from utils.value import ChangedInteger
@@ -16,7 +16,6 @@ class StirrerTask(ATask):
 		stirrer_device.set_on_changed("dutyCycle", self.on_dutycycle_changed)
 		stirrer_device.set_on_changed("cycleTime", self.on_cycletime_changed)
 		stirrer_device.set_on_changed("connected", self.on_connected_changed)
-		stirrer_device.set_on_changed("firmware_version", self.on_firmware_version_changed)

 	def on_speed_changed(self, value):
 		asyncio.create_task(self.send({'Speed': value}))
@@ -34,13 +33,10 @@ class StirrerTask(ATask):
 		self._on_connected_changed = callback

 	def on_connected_changed(self, value):
-		asyncio.create_task(self.send({'Connected': value}))
+		fire_and_forget(self.send({'Connected': value}))
 		if self._on_connected_changed:
 			self._on_connected_changed(value)

-	def on_firmware_version_changed(self, value):
-		asyncio.create_task(self.send({'FirmwareVersion': value}))
-
 	async def recv(self, data):
 		for pair in data.items():
 			if pair[0] == 'Connect':
@@ -49,6 +45,7 @@ class StirrerTask(ATask):
 			elif pair[0] == 'Disconnect':
 				self.device.activate(False)
 				self.device.disconnect()
+				self.device.set_speed(0.0)
 			elif 'Speed' in pair[0]:
 				self.device.set_speed(pair[1])

@@ -69,13 +66,25 @@ class StirrerTask(ATask):
 		# for why a simulated device's connect() alone doesn't fire the
 		# observer.
 		await self.send({'Connected': self.device.connected})
-		await self.send({'FirmwareVersion': self.device.firmware_version})

-		with self.device.open():
-			while True:
+		# Outer retry loop - see HeaterTask.on_process()'s own comment for
+		# why: nothing restarts a dead ATask, so an exception escaping this
+		# coroutine entirely would permanently stop this device from ever
+		# being process()'d again, even after a later successful reconnect.
+		while True:
+			try:
+				with self.device.open():
+					while True:
+						try:
+							self.device.process()
+						except Exception as e:
+							print(f"StirrerTask: comm error, marking disconnected: {e}")
+							self.device.disconnect()
+						await asyncio.sleep(self.interval)
+			except Exception as e:
+				print(f"StirrerTask: unexpected error, recovering: {e}")
 				try:
-					self.device.process()
-				except Exception as e:
-					print(f"StirrerTask: comm error, marking disconnected: {e}")
 					self.device.disconnect()
+				except Exception:
+					pass
 				await asyncio.sleep(self.interval)
@@ -1,6 +1,6 @@
 import asyncio
 import bisect
-from tasks import ATask
+from tasks import ATask, fire_and_forget
 from ws.message import MsgIo
 from utils.value import ChangedFloat
 from components import APid, AStirrer, AHeater
@@ -265,14 +265,28 @@ class SudTask(ATask):
 		outside RAMPING/HOLDING/WAIT_USER/PAUSED), the same path a manual
 		Stop press takes, so all the usual shutdown plumbing (heater
 		shutdown, sud log stop_run - see SudTask.set_on_end()'s wiring in
-		server/brewpi.py) fires exactly as it would for Stop."""
+		server/brewpi.py) fires exactly as it would for Stop.
+
+		No-op if there's no running event loop - happens when a
+		Connectable's connected attribute changes as a side effect of the
+		server's own final best-effort hardware-release cleanup
+		(server/brewpi.py's `finally:` block), which runs after the loop
+		has already stopped. self.sud.stop() below cascades into Sud's own
+		'state' observable (on_state_changed -> heater_task.shutdown()/
+		sud_log_task.stop_run()), which isn't itself guarded against a
+		stopped loop - the whole server is already tearing down at that
+		point regardless, so there's nothing useful to stop/notify."""
+		try:
+			asyncio.get_running_loop()
+		except RuntimeError:
+			return
 		if self.sud.state in (SudState.IDLE, SudState.DONE):
 			return
 		if self.heater.connected and self.stirrer.connected:
 			return
 		self.sud.stop()
-		asyncio.create_task(self.send({'Error': 'Heater/Stirrer disconnected - brew stopped.'}))
-		asyncio.create_task(self.send({'Error': None}))
+		fire_and_forget(self.send({'Error': 'Heater/Stirrer disconnected - brew stopped.'}))
+		fire_and_forget(self.send({'Error': None}))

 	def on_state_changed(self, value):
 		asyncio.create_task(self.send({'State': str(value)}))
@@ -3,6 +3,21 @@ import asyncio
 from utils.value import AttributeChange


+def fire_and_forget(coro):
+	"""asyncio.create_task(), tolerant of there being no running event loop.
+	An actor's observable attribute (e.g. Connectable.connected) can change
+	as a side effect of the server's own final best-effort hardware-release
+	cleanup (server/brewpi.py's `finally:` block - heater.close()/
+	stirrer.activate(False)), which runs after the loop has already
+	stopped. There's nobody left to notify at that point anyway, so this
+	just drops the message instead of crashing the shutdown sequence."""
+	try:
+		return asyncio.create_task(coro)
+	except RuntimeError:
+		coro.close()
+		return None
+
+
 class ATask(AttributeChange):
 	def __init__(self, interval):
 		AttributeChange.__init__(self)
@@ -16,7 +16,15 @@ class TempSensorTask(ATask):
 		# which would otherwise fire on_temp_changed() (and its
 		# asyncio.create_task()) before the event loop is running, since
 		# all tasks are built synchronously at module level in brewpi.py.
-		self.sensor.temperature()
+		# Guarded even though TempSensor_max31865.temperature() already
+		# catches its own read/reopen failures - belt-and-suspenders
+		# against a still-unanticipated exception crashing server startup
+		# entirely (same lesson as HeaterTask/StirrerTask's own outer
+		# guards - see tasks/heater.py's on_process()).
+		try:
+			self.sensor.temperature()
+		except Exception as e:
+			print(f"TempSensorTask: comm error priming sensor: {e}")
 		self.sensor.set_on_changed("temp", ChangedFloat(self.on_temp_changed, prec=1).set)

 	def on_temp_changed(self, value):
@@ -29,6 +37,12 @@ class TempSensorTask(ATask):
 		await self.msg_handler.send(data)

 	async def on_process(self):
+		# Guarded for the same reason as the priming call above - nothing
+		# restarts a dead ATask, so an exception escaping this loop would
+		# permanently stop this sensor from ever being read again.
 		while True:
-			self.sensor.temperature()
+			try:
+				self.sensor.temperature()
+			except Exception as e:
+				print(f"TempSensorTask: comm error reading sensor: {e}")
 			await asyncio.sleep(self.interval)
@@ -0,0 +1,69 @@
+import unittest
+
+from components.pid.pid import Pid
+
+
+GAINS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5}
+
+
+def _feed(pid, err_sequence):
+    """Runs pid.process(err, 0) over err_sequence, returns the y trace."""
+    ys = []
+    for err in err_sequence:
+        pid.process(err, 0)
+        ys.append(pid.get_y())
+    return ys
+
+
+def _incident_err_sequence():
+    """Shaped like the HOLD-disturbance incident: a positive heatrate_err
+    held for ~130 ticks (outer loop asking for real heat), then a negative
+    tail (outer loop has zeroed its target while heatrate_ist is still
+    high, matching the real rate_soll - rate_ist gap from the log)."""
+    return [0.3] * 130 + [-0.3] * 200
+
+
+class TestPidYiMax(unittest.TestCase):
+    def test_yi_never_exceeds_configured_bound(self):
+        pid = Pid(dt=1.0)
+        pid.set_params({**GAINS, "yi_max": 0.3})
+        for err in _incident_err_sequence():
+            pid.process(err, 0)
+            self.assertLessEqual(abs(pid.yi), 0.3 + 1e-9)
+
+    def test_clamped_recovers_faster_than_unclamped(self):
+        err_sequence = _incident_err_sequence()
+
+        clamped = Pid(dt=1.0)
+        clamped.set_params({**GAINS, "yi_max": 0.3})
+        ys_clamped = _feed(clamped, err_sequence)
+
+        unclamped = Pid(dt=1.0)
+        unclamped.set_params(dict(GAINS))
+        ys_unclamped = _feed(unclamped, err_sequence)
+
+        # Ticks after the error goes negative (index 130) until y drops
+        # back under a small threshold.
+        threshold = 0.05
+        negative_start = 130
+
+        def recovery_time(ys):
+            for i in range(negative_start, len(ys)):
+                if ys[i] < threshold:
+                    return i - negative_start
+            return len(ys) - negative_start
+
+        self.assertLess(recovery_time(ys_clamped), recovery_time(ys_unclamped))
+
+    def test_no_yi_max_configured_is_unbounded(self):
+        pid = Pid(dt=1.0)
+        pid.set_params(dict(GAINS))
+        peak_yi = 0.0
+        for err in _incident_err_sequence():
+            pid.process(err, 0)
+            peak_yi = max(peak_yi, pid.yi)
+        self.assertGreater(peak_yi, 0.3)
+
+
+if __name__ == '__main__':
+    unittest.main()
@@ -0,0 +1,199 @@
+import unittest
+
+from components.pid.temp_controller_smith import TempController
+from components.pid.temp_controller_fsm import States
+from components.plant.pot import Pot
+
+
+DT = 1.0
+AMBIENT = 20.0
+PLANT_PARAMS = {"M": 27.96, "C": 3403.43, "L": 0.2, "Td": 17}
+
+OUTER_PARAMS = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0}
+OUTER_PARAMS_HOLD_COOL = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0, "y_hold_min": -0.1}
+INNER_HEAT_PARAMS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5}
+INNER_HOLD_CLAMPED = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5, "yi_max": 0.3}
+INNER_HOLD_UNCLAMPED = dict(INNER_HEAT_PARAMS)
+
+
+def make_controller(inner_hold_params, outer_params=None):
+    ctrl = TempController(DT)
+    ctrl.set_params({
+        "beta": 0.9,
+        "Outer": dict(outer_params if outer_params is not None else OUTER_PARAMS),
+        "Inner": {
+            "Heat": dict(INNER_HEAT_PARAMS),
+            "Hold": dict(inner_hold_params),
+            "Cool": dict(INNER_HEAT_PARAMS),
+        },
+    })
+    ctrl.set_model_plant_params(PLANT_PARAMS)
+    ctrl.set_ambient_temperature(AMBIENT)
+    ctrl.set_enabled(True)
+    return ctrl
+
+
+def make_plant(initial_temp):
+    plant = Pot(DT)
+    plant.set_plant_params(PLANT_PARAMS)
+    plant.set_ambient_temperature(AMBIENT)
+    plant.initial(initial_temp)
+    return plant
+
+
+def tick(ctrl, plant, temp_soll, heatrate_soll):
+    plant.process()
+    ctrl.set_theta_ist(plant.get_temperature())
+    ctrl.set_theta_soll(temp_soll)
+    ctrl.set_heatrate_soll(heatrate_soll)
+    ctrl.process()
+    power = 3500.0 * max(0.0, ctrl.get_power())
+    plant.set_power(power)
+    ctrl.set_model_power(power)
+
+
+class TestHoldDisturbanceOvershoot(unittest.TestCase):
+    """Reproduces the cold-water-during-HOLD incident from
+    docs/overshoot_hold_windup.md and checks that Inner.Hold's yi_max
+    measurably reduces the resulting overshoot."""
+
+    def _run_disturbance(self, inner_hold_params):
+        ctrl = make_controller(inner_hold_params)
+        plant = make_plant(30.0)
+
+        # Settle at HOLD, 30 degC.
+        for _ in range(60):
+            tick(ctrl, plant, 30.0, 1.0)
+        self.assertEqual(ctrl.state, States.HOLD)
+
+        # Cold-water disturbance: knock plant.temp down ~0.5 degC.
+        plant.temp -= 0.5
+
+        peak = plant.get_temperature()
+        for _ in range(600):
+            tick(ctrl, plant, 30.0, 1.0)
+            peak = max(peak, plant.get_temperature())
+            # Disturbance must stay within HOLD the whole time, matching
+            # the incident (diff never reached HoldHeat).
+            self.assertEqual(ctrl.state, States.HOLD)
+
+        return peak
+
+    def test_clamped_overshoot_smaller_than_unclamped(self):
+        peak_clamped = self._run_disturbance(INNER_HOLD_CLAMPED)
+        peak_unclamped = self._run_disturbance(INNER_HOLD_UNCLAMPED)
+
+        overshoot_clamped = peak_clamped - 30.0
+        overshoot_unclamped = peak_unclamped - 30.0
+
+        self.assertLess(overshoot_clamped, overshoot_unclamped)
+
+
+class TestHoldOvershootRecoversToSetpoint(unittest.TestCase):
+    """Reproduces the sud_0030 grain-fill-in incident: a HOLD overshoot
+    (post-recovery temp_ist above temp_soll, well under HoldCool's
+    threshold so the FSM never leaves HOLD) must coast back down to
+    setpoint rather than sitting in a permanent flat-clamp dead zone -
+    see docs/overshoot_hold_windup.md and the Outer.y_hold_min floor in
+    temp_controller_base.py's process_pid()."""
+
+    def _run_overshoot(self, outer_params):
+        ctrl = make_controller(INNER_HOLD_CLAMPED, outer_params)
+        plant = make_plant(30.0)
+
+        for _ in range(60):
+            tick(ctrl, plant, 30.0, 1.0)
+        self.assertEqual(ctrl.state, States.HOLD)
+
+        # Overshoot above setpoint, small enough to stay inside HOLD
+        # (matches the ~0.3-0.4 degC band observed in the real trace).
+        plant.temp += 0.4
+
+        min_temp = plant.get_temperature()
+        for _ in range(900):
+            tick(ctrl, plant, 30.0, 1.0)
+            self.assertEqual(ctrl.state, States.HOLD)
+            min_temp = min(min_temp, plant.get_temperature())
+
+        return plant.get_temperature(), min_temp
+
+    def test_negative_floor_recovers_flat_clamp_does_not(self):
+        final_flat, _ = self._run_overshoot(OUTER_PARAMS)
+        final_floored, min_floored = self._run_overshoot(OUTER_PARAMS_HOLD_COOL)
+
+        # Old behaviour (y clamped to exactly 0.0): pid_inner fights the
+        # pot's own ambient loss to hold the overshot temperature flat -
+        # it stays measurably above setpoint within this window.
+        self.assertGreater(final_flat - 30.0, 0.05)
+
+        # New behaviour (small negative floor): the outer loop can ask
+        # for a gentle decline, so the overshoot recovers much closer to
+        # setpoint than the flat clamp manages in the same window.
+        self.assertLess(abs(final_floored - 30.0), 0.15)
+
+        # Guard against reintroducing the violent limit cycle the original
+        # [0, 1] clamp was added to break (bb5af3c): recovery should coast
+        # down smoothly, undershooting by no more than the injected
+        # disturbance itself (0.4 degC) - not the ~1 degC+ swings of a
+        # runaway power on/off cycle.
+        self.assertGreater(min_floored, 30.0 - 0.4)
+
+
+class TestRealRampStillReachesTarget(unittest.TestCase):
+    """Guards against reintroducing the rejected flat-clamp regression:
+    Inner.Heat has no yi_max, so a genuine ramp must still be able to
+    reach its commanded heat rate."""
+
+    def test_ramp_reaches_commanded_heatrate(self):
+        ctrl = make_controller(INNER_HOLD_CLAMPED)
+        plant = make_plant(20.0)
+
+        heatrate_soll = 1.5
+        temp_soll = 40.0
+
+        max_heatrate_ist = 0.0
+        reached_heat = False
+        for _ in range(600):
+            tick(ctrl, plant, temp_soll, heatrate_soll)
+            if ctrl.state == States.HEAT:
+                reached_heat = True
+                max_heatrate_ist = max(max_heatrate_ist, ctrl.get_heatrate_ist())
+
+        self.assertTrue(reached_heat)
+        self.assertGreater(max_heatrate_ist, 1.4)
+
+
+class TestHoldHeatHoldTransitionIsBumpless(unittest.TestCase):
+    """Per-state yi_max swap must not itself introduce a discontinuity in y
+    at a HOLD<->HEAT transition - the same Pid instance/state carries over,
+    only the yi_max ceiling changes going forward."""
+
+    def test_no_bump_at_transitions(self):
+        ctrl = make_controller(INNER_HOLD_CLAMPED)
+        plant = make_plant(20.0)
+
+        y_prev = None
+        state_prev = None
+        max_bump = 0.0
+        for _ in range(1000):
+            tick(ctrl, plant, 40.0, 1.5)
+            y = ctrl.get_power()
+            if state_prev is not None and ctrl.state != state_prev and \
+                    {state_prev, ctrl.state} <= {States.HOLD, States.HEAT}:
+                max_bump = max(max_bump, abs(y - y_prev))
+            y_prev = y
+            state_prev = ctrl.state
+
+        # A "bump" here would be a y-step far larger than what one tick of
+        # normal PID evolution produces elsewhere in the same run - not
+        # literally zero, since heatrate_err itself keeps evolving tick to
+        # tick regardless of the state transition. Note: a HEAT->HOLD
+        # transition can retroactively clamp yi if a sustained ramp pushed
+        # it past Inner.Hold's yi_max before HeatHold's threshold fired,
+        # producing a small (not literally bumpless) step - bounded well
+        # below the full-freeze/thaw magnitude this replaces.
+        self.assertLess(max_bump, 0.1)
+
+
+if __name__ == '__main__':
+    unittest.main()
@@ -45,13 +45,21 @@ from components.pid import PidFactory
 from components.plant.pot import Pot


+GAIN_SECTIONS = (('Outer', 'outer'), ('Inner.Heat', 'inner-heat'),
+                 ('Inner.Hold', 'inner-hold'), ('Inner.Cool', 'inner-cool'))
+
+
 def _apply_gain_overrides(params, args):
    p = copy.deepcopy(params)
-    for section, prefix in (('Hold', 'hold'), ('Heat', 'heat'), ('Cool', 'cool')):
+    for section, prefix in GAIN_SECTIONS:
        for gain in ('kp', 'ki', 'kd', 'kt'):
-            val = getattr(args, '{}_{}'.format(prefix, gain))
+            val = getattr(args, '{}_{}'.format(prefix.replace('-', '_'), gain))
            if val is not None:
-                p[section][gain] = val
+                if '.' in section:
+                    outer, inner = section.split('.')
+                    p[outer][inner][gain] = val
+                else:
+                    p[section][gain] = val
    return p


@@ -63,7 +71,7 @@ def _infer_max_power(samples):
 def _infer_heatrate_soll_set(samples):
    """Estimate heatrate_soll_set from the log.

-    rate_soll = heatrate_soll_set * pid_hold.get_y(); when the hold PID is
+    rate_soll = heatrate_soll_set * pid_outer.get_y(); when the outer PID is
    saturated at 1.0 during active heating the two are equal, so the max
    rate_soll seen while the heater is on is a good upper bound."""
    candidates = [s['rate_soll'] for s in samples if s['power_set'] > 0]
@@ -211,12 +219,12 @@ def main():
    parser.add_argument('--plant-L',  type=float, default=None, metavar='W/kgK', help='Heat loss coeff [W/(kg·K)]')
    parser.add_argument('--plant-Td', type=float, default=None, metavar='s',     help='Transport delay [s]')

-    for section, prefix in (('Hold', 'hold'), ('Heat', 'heat'), ('Cool', 'cool')):
+    for section, prefix in GAIN_SECTIONS:
        for gain in ('kp', 'ki', 'kd', 'kt'):
            parser.add_argument(
                '--{}-{}'.format(prefix, gain),
                type=float, default=None,
-                dest='{}_{}'.format(prefix, gain),
+                dest='{}_{}'.format(prefix.replace('-', '_'), gain),
                metavar='VAL',
                help='Override TempCtrl.{}.{}'.format(section, gain),
            )
@@ -265,8 +273,8 @@ def main():
    ambient      = args.ambient   if args.ambient   is not None else config.get('ambient_temperature', 20.0)

    any_override = any(
-        getattr(args, '{}_{}'.format(p, g)) is not None
-        for p in ('hold', 'heat', 'cool')
+        getattr(args, '{}_{}'.format(prefix.replace('-', '_'), g)) is not None
+        for _, prefix in GAIN_SECTIONS
        for g in ('kp', 'ki', 'kd', 'kt')
    )
    config_source = 'log' if log.get('Config') else 'file'
@@ -281,8 +289,12 @@ def main():
    print('Params: {} ({})'.format('overridden' if any_override else 'from {}'.format(config_source),
                                   'log' if log_plant else 'defaults'))
    print()
-    for section in ('Hold', 'Heat', 'Cool'):
-        p = pid_params[section]
+    for section, _ in GAIN_SECTIONS:
+        if '.' in section:
+            outer, inner = section.split('.')
+            p = pid_params[outer][inner]
+        else:
+            p = pid_params[section]
        print('  {}: kp={kp}  ki={ki}  kd={kd}  kt={kt}'.format(section, **p))

    replay = run_replay(samples, pid_type, pid_params, rate_soll, plant_params, param_events,
@@ -45,10 +45,8 @@ let closedLoop = true;
 // are always connected and never expose Connect/Disconnect (see
 // updateDeviceStatus()).
 let heaterConnected = false;
-let heaterFirmware = null;
 let heaterSimulated = null;
 let stirrerConnected = false;
-let stirrerFirmware = null;
 let stirrerSimulated = null;

 // Pot hardware config from server's Pot section - used for display before
@@ -536,21 +534,41 @@ function updateSudActions() {
 	document.getElementById('btn-sud-stop').disabled = !(running || paused);
 }

-// Shared by onHeaterChanged/onStirrerChanged - updates the status badge,
-// firmware label, and Connect/Disconnect buttons for one device. Simulated
-// devices are always connected and have nothing to connect/disconnect, so
-// their buttons are hidden entirely rather than just disabled.
-function updateDeviceStatus(prefix, isConnected, firmware, simulated) {
+// Shared by onHeaterChanged/onStirrerChanged - updates the status badge
+// and Connect/Disconnect buttons for one device. Simulated devices are
+// always connected and have nothing to connect/disconnect, so their
+// buttons are hidden entirely rather than just disabled.
+//
+// isConnected is a tri-state: true/false once the server has actually
+// reported it, or null for "unknown" - shown as a blank '---' badge with
+// both buttons disabled, used while the browser itself has no connection
+// to the server at all (see setConnected()) since a stale Connected/
+// Disconnected from before that disconnect would otherwise misleadingly
+// look current.
+function updateDeviceStatus(prefix, isConnected, simulated) {
 	const statusEl = document.getElementById(`${prefix}-status`);
-	statusEl.textContent = isConnected ? 'Connected' : 'Disconnected';
-	statusEl.className = isConnected ? 'status-connected' : 'status-disconnected';
-	document.getElementById(`${prefix}-firmware`).textContent = firmware ? `F/W ${firmware}` : '';
+	if (isConnected === null) {
+		statusEl.textContent = '---';
+		statusEl.className = 'panel-status status-disconnected';
+	} else {
+		statusEl.textContent = isConnected ? 'Connected' : 'Disconnected';
+		statusEl.className = `panel-status ${isConnected ? 'status-connected' : 'status-disconnected'}`;
+	}
 	const connectBtn = document.getElementById(`btn-${prefix}-connect`);
 	const disconnectBtn = document.getElementById(`btn-${prefix}-disconnect`);
 	connectBtn.classList.toggle('hidden', !!simulated);
 	disconnectBtn.classList.toggle('hidden', !!simulated);
-	connectBtn.disabled = isConnected;
-	disconnectBtn.disabled = !isConnected;
+	connectBtn.disabled = isConnected !== false;
+	disconnectBtn.disabled = isConnected !== true;
+}
+
+// Reflects closedLoop (the Controller panel's Enable toggle) in the
+// Controller heading's status badge - Enabled/green when the TC is
+// driving the heater, Disabled/gray in open-loop.
+function updateClosedLoopStatus() {
+	const statusEl = document.getElementById('closed-loop-status');
+	statusEl.textContent = closedLoop ? 'Enabled' : 'Disabled';
+	statusEl.className = `panel-status ${closedLoop ? 'status-connected' : 'status-disconnected'}`;
 }

 // Mirrors client/brewpi_gui.py's show_user_message(): the Sud is already
@@ -632,6 +650,7 @@ function onHeaterChanged(msg) {
 		} else if (key === 'ClosedLoop') {
 			closedLoop = msg.ClosedLoop;
 			document.getElementById('closed-loop').checked = closedLoop;
+			updateClosedLoopStatus();
 			updateControlsEnabled();
 		} else if (key === 'Capabilities') {
 			const power = msg.Capabilities.Power;
@@ -641,15 +660,12 @@ function onHeaterChanged(msg) {
 			slider.max = power.Max;
 		} else if (key === 'Connected') {
 			heaterConnected = msg.Connected;
-			updateDeviceStatus('heater', heaterConnected, heaterFirmware, heaterSimulated);
+			updateDeviceStatus('heater', heaterConnected, heaterSimulated);
 			updateControlsEnabled();
 			updateSudActions();
-		} else if (key === 'FirmwareVersion') {
-			heaterFirmware = msg.FirmwareVersion;
-			updateDeviceStatus('heater', heaterConnected, heaterFirmware, heaterSimulated);
 		} else if (key === 'Simulated') {
 			heaterSimulated = msg.Simulated;
-			updateDeviceStatus('heater', heaterConnected, heaterFirmware, heaterSimulated);
+			updateDeviceStatus('heater', heaterConnected, heaterSimulated);
 		}
 	}
 }
@@ -669,15 +685,12 @@ function onStirrerChanged(msg) {
 			slider.max = power.Max;
 		} else if (key === 'Connected') {
 			stirrerConnected = msg.Connected;
-			updateDeviceStatus('stirrer', stirrerConnected, stirrerFirmware, stirrerSimulated);
+			updateDeviceStatus('stirrer', stirrerConnected, stirrerSimulated);
 			updateControlsEnabled();
 			updateSudActions();
-		} else if (key === 'FirmwareVersion') {
-			stirrerFirmware = msg.FirmwareVersion;
-			updateDeviceStatus('stirrer', stirrerConnected, stirrerFirmware, stirrerSimulated);
 		} else if (key === 'Simulated') {
 			stirrerSimulated = msg.Simulated;
-			updateDeviceStatus('stirrer', stirrerConnected, stirrerFirmware, stirrerSimulated);
+			updateDeviceStatus('stirrer', stirrerConnected, stirrerSimulated);
 		}
 	}
 }
@@ -751,6 +764,7 @@ function onSudChanged(msg) {
 			if (isRunningNow && !wasRunning) {
 				closedLoop = true;
 				document.getElementById('closed-loop').checked = true;
+				updateClosedLoopStatus();
 				sendMsg('Heater', {ClosedLoop: true});
 				// Accept the next Soll push from the server even if it
 				// arrives before sudRunning has gone true at this client
@@ -865,6 +879,20 @@ function setConnected(isConnected) {
 	document.getElementById('lcd-pot-row').style.visibility = isConnected ? 'visible' : 'hidden';
 	document.getElementById('sud-status-line').style.visibility = isConnected ? 'visible' : 'hidden';
 	document.getElementById('env-status-line').style.visibility = isConnected ? 'visible' : 'hidden';
+	if (!isConnected) {
+		// The browser's own link to the server is gone, so whatever
+		// Heater/Stirrer connection state was last reported is now stale -
+		// blank it out rather than leave a misleading Connected/
+		// Disconnected badge showing. Simulated stays unknown too (null,
+		// not false) since updateDeviceStatus() only hides the buttons
+		// once the server says so again.
+		heaterConnected = null;
+		heaterSimulated = null;
+		stirrerConnected = null;
+		stirrerSimulated = null;
+		updateDeviceStatus('heater', heaterConnected, heaterSimulated);
+		updateDeviceStatus('stirrer', stirrerConnected, stirrerSimulated);
+	}
 	updateSudActions();
 }

@@ -886,6 +914,7 @@ function connect() {
 		initialTempSollSync = true;
 		initialPotTempSync = true;
 		closedLoop = true;
+		updateClosedLoopStatus();
 		potConfig = {};
 		setConnected(true);
 		for (const channel of CHANNELS) {
@@ -937,6 +966,8 @@ document.getElementById('btn-connect').addEventListener('click', () => {
 });

 document.getElementById('closed-loop').addEventListener('change', (e) => {
+	closedLoop = e.target.checked;
+	updateClosedLoopStatus();
 	sendMsg('Heater', {ClosedLoop: e.target.checked});
 });
 document.getElementById('btn-heater-connect').addEventListener('click', () => {
@@ -38,7 +38,10 @@
 	<button id="btn-manual-toggle" title="Collapse manual panel">&#9664;</button>
 	<div id="manual-panels">
 		<div class="panel">
-			<h3>Controller</h3>
+			<h3>Controller <span id="closed-loop-status" class="panel-status status-connected">Enabled</span></h3>
+			<label class="checkbox-label">
+				<input type="checkbox" id="closed-loop" checked> Enable
+			</label>
 			<label>Temperature [&deg;C]
 				<div class="slider-row">
 					<input type="range" id="temp-soll" min="0" max="100" step="1">
@@ -50,16 +53,11 @@
 			</label>
 		</div>
 		<div class="panel">
-			<h3>Heater</h3>
+			<h3>Heater <span id="heater-status" class="panel-status status-disconnected">Disconnected</span></h3>
 			<div class="device-status-row">
-				<span id="heater-status" class="status-disconnected">Disconnected</span>
-				<span id="heater-firmware" class="device-firmware"></span>
 				<button id="btn-heater-connect">Connect</button>
 				<button id="btn-heater-disconnect" disabled>Disconnect</button>
 			</div>
-			<label class="checkbox-label">
-				<input type="checkbox" id="closed-loop" checked> Closed-loop
-			</label>
 			<label>Power [W]
 				<div class="slider-row">
 					<input type="range" id="heater-power" min="0" max="100" step="1">
@@ -68,10 +66,8 @@
 			</label>
 		</div>
 		<div class="panel">
-			<h3>Stirrer</h3>
+			<h3>Stirrer <span id="stirrer-status" class="panel-status status-disconnected">Disconnected</span></h3>
 			<div class="device-status-row">
-				<span id="stirrer-status" class="status-disconnected">Disconnected</span>
-				<span id="stirrer-firmware" class="device-firmware"></span>
 				<button id="btn-stirrer-connect">Connect</button>
 				<button id="btn-stirrer-disconnect" disabled>Disconnect</button>
 			</div>
@@ -307,6 +307,9 @@ header#connection-bar {
    box-shadow: 0 2px 8px rgba(0,0,0,0.45);
 }
 .panel h3 {
+    display: flex;
+    justify-content: space-between;
+    align-items: baseline;
    margin: 0 0 0.5em 0;
    font-size: 0.85em;
    text-transform: uppercase;
@@ -315,6 +318,11 @@ header#connection-bar {
    border-bottom: 1px solid var(--border);
    padding-bottom: 0.35em;
 }
+.panel-status {
+    text-transform: none;
+    letter-spacing: normal;
+    font-size: 1rem;
+}
 .panel label {
    display: flex;
    flex-direction: column;
@@ -336,12 +344,12 @@ header#connection-bar {
 .device-status-row {
    display: flex;
    align-items: center;
+    flex-wrap: wrap;
    gap: 0.6em;
    margin: 0.4em 0;
    font-size: 0.9em;
 }
 .device-status-row button { padding: 0.15em 0.6em; font-size: 0.9em; }
-.device-firmware { color: var(--text-muted); }

 /* --- Hidden --- */
 .hidden { display: none !important; }
Author	SHA1	Message	Date
jensandClaude Sonnet 5	5a75d15416	docs: note grounding y_hold_min in passive-cooling capacity in pid TODO Captures the idea of computing the HOLD-state negative floor from the Smith controller's internal Pot model (L*(theta_ist-theta_amb)/C) instead of the fixed -0.1 heuristic, plus the caveat that the Normal controller has no internal plant model to draw the same estimate from. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE	2026-07-06 08:30:40 +02:00
jensandClaude Sonnet 5	6d17419067	docs: note active-cooler architecture assessment in pid TODO Captures the answer to "would the cascade/FSM design still hold up with a real active cooler instead of heat-only + passive loss": PID/plant model already generalize (symmetric y range, sign-agnostic plant math, existing COOL state/pid_inner_cool), but the actor-level max(0, y) clamp in tasks/heater.py is the one thing making today's tuning mismatches free - removing it exposes both Outer.y_hold_min and pid_inner_cool as live, untuned negative-power paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE	2026-07-06 08:26:25 +02:00
jensandClaude Sonnet 5	2d032bf9b2	fix: allow gentle negative HOLD floor to fix steady-state overshoot Testing the windup-fix rework live against sude/sud_0030.json surfaced a distinct bug: a HOLD overshoot from grain-fill-in cooling never decayed - process_pid()'s pid_outer_y floor clamped to exactly 0.0, so pid_inner fought the pot's own ambient loss to hold the overshot temperature flat instead of declining back to setpoint (see docs/overshoot2.png). Replace the hardcoded 0.0 floor with a configurable Outer.y_hold_min (default 0.0, backward compatible), set to -0.1 in config.json, both .tpl templates, and the demo scripts - small enough to avoid reintroducing the `bb5af3c` limit cycle while letting HOLD request a gentle decline matching passive ambient cooling. Adds TestHoldOvershootRecoversToSetpoint and documents the finding in docs/overshoot_hold_windup.md's Follow-up section and components/pid/TODO.md. Confirmed against a live sud_0030 re-run, not just the unit test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE	2026-07-06 08:19:10 +02:00
jensandClaude Sonnet 5	11c3d92f25	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	2026-07-05 22:08:19 +02:00
jensandClaude Sonnet 5	acfb98e186	docs: add FSM states diagram screenshot Reference screenshot of the FSM-states panel from the temp controller architecture diagram (states, thresholds, and the HOLD->HEAT no-reset bumpless-transfer note). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DGQhVQ2Y3yXAQTXhrxVd5u	2026-07-05 21:59:20 +02:00
jensandClaude Sonnet 5	f69d63c31b	fix: split inner-loop yi_max clamp to fix HOLD-state windup overshoot Renames pid_hold/pid_heat/pid_cool to pid_outer/pid_inner/pid_inner_cool to match what actually runs when, and splits inner-loop config into Inner.Heat/Inner.Hold/Inner.Cool so the same PID instance gets a tight yi_max ceiling only while HOLD drives it, without capping legitimate 1.5 K/min ramps. Fixes the overshoot from docs/overshoot_hold_windup.md where a cold-water disturbance during HOLD wound up pid_heat's integral term with no anti-windup engagement, taking ~35s+ to unwind naturally. Breaking config change: Hold/Heat/Cool -> Outer/Inner.{Heat,Hold,Cool} in config.json, both .tpl templates, the pid/sud demo scripts, and replay_sim.py's CLI flags. Adds tests/components/pid/ (stdlib unittest) covering the Pid clamp/recovery behavior and closed-loop disturbance, ramp, and HOLD<->HEAT transition cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DGQhVQ2Y3yXAQTXhrxVd5u	2026-07-05 21:23:55 +02:00
jensandClaude Sonnet 5	da832b961e	docs: add claude-faq.md, starting with memory-vs-docs Q&A Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KefnwbGDM8CGrhb4sFhVq9	2026-07-04 12:57:21 +02:00
jensandClaude Sonnet 5	de11b849d8	docs: revise pid_heat windup plan to pid_outer/pid_inner rename + Inner.Hold split Supersedes the earlier flat yi_max clamp / FSM-gating ideas: same inner PID instance switches its active param set (Inner.Heat vs Inner.Hold) by state instead of freezing, giving bumpless transfer for free and a tight yi_max that only applies while HOLD is driving it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KefnwbGDM8CGrhb4sFhVq9	2026-07-04 12:48:59 +02:00
jensandClaude Sonnet 5	13bb011099	docs: plan HOLD-state pid_heat windup fix from overshoot investigation Records the root cause of the cold-water overshoot in docs/overshoot.png (integral windup in pid_heat with no anti-windup engagement, since y never saturated) and the refined fix plan: gate pid_heat by FSM state instead of a flat yi_max clamp, which was rejected after the numbers showed it would also cripple legitimate high-rate ramps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KefnwbGDM8CGrhb4sFhVq9	2026-07-04 12:31:58 +02:00
jensandClaude Sonnet 5	8689494be0	fix: zero heater power / stirrer rpm on manual disconnect A manual Disconnect command left the last commanded power/speed in place server-side, so a reconnect (or the UI's PowerSet/Speed echo) could show a stale nonzero setpoint for a device that's no longer attached. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5WEDCc4v473DKqSaQhTQt	2026-07-03 23:29:29 +02:00
jensandClaude Sonnet 5	b526711595	fix: blank Heater/Stirrer status on browser disconnect instead of stale text updateDeviceStatus() gains a tri-state: true/false once the server has actually reported it, or null for "unknown" - rendered as a blank '---' badge with both Connect/Disconnect buttons disabled. setConnected() now resets Heater/Stirrer to null whenever the browser's own websocket connection drops, so a stale Connected/Disconnected badge from before the disconnect doesn't keep misleadingly showing as current. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 21:59:15 +02:00
jensandClaude Sonnet 5	4b9a144b62	fix: temp sensor read failures could crash startup or permanently kill the task Same bug class as the earlier heater fix: TempSensorTask.on_process() called self.sensor.temperature() every tick with no exception handling at all, and the constructor even called it once synchronously at startup to prime the value. A read failure there would either crash the whole server before the event loop started, or permanently kill TempSensorTask's coroutine mid-run - nothing restarts a dead ATask, so the sensor would never be read again for the rest of the process's life. TempSensor_max31865.temperature() now catches read failures and calls a new reopen() (closes/reopens the spidev handle - the closest equivalent to unplug/replug for a bus peripheral) instead of raising, holding the last-known-good reading meanwhile. reopen() itself never raises either, learned from HendiCtrl.disconnect() previously letting a failure escape the same way. Belt-and-suspenders guards added at the TempSensorTask level too, matching HeaterTask/StirrerTask. Verified against a fake spidev: a read failure no longer raises, reopen() is attempted automatically, and readings resume once the bus responds again - even when reopen() itself also fails. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 21:45:21 +02:00
jensandClaude Sonnet 5	b6b79adff8	docs: drop stale config.json.bak cleanup reminder The backup has already been deleted from the Pi, so the note is done. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 21:37:44 +02:00
jensandClaude Sonnet 5	9cd386a22e	docs: note config.json's by-id serial paths and the .bak cleanup config.json isn't tracked in git, so this records what changed on the Pi during today's connect/disconnect testing: Heater/Stirrer ports switched from /dev/ttyUSB0//dev/ttyACM0 to stable /dev/serial/by-id/... paths after a replug renumbered both devices, plus a reminder to remove the pre-edit config.json.bak-20260703212821 left on the Pi. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 21:35:30 +02:00
jensandClaude Sonnet 5	671e0c2474	fix: heater task permanently died after a mid-brew unplug, never recovered Root cause (from the brewpi Pi's log): HendiCtrl.disconnect() only caught HendiException (a protocol-level NAK), but a genuine unplug fails deep inside pyserial itself - flushInput()/readline() raise termios.error/OSError on the now-dead fd. That escaped disconnect() uncaught (skipping self.ser.close() too), then kept propagating up through every caller that invoked disconnect() from its own except handler (HeaterHendi.process()/activate(), AHeater.open()'s context-manager exit), all the way out of HeaterTask.on_process()'s coroutine entirely. Nothing restarts a dead ATask, so the heater's process()/duty-cycle loop was gone for the rest of the process's life - reconnecting afterward changed `connected` back to True but nothing was left running to ever call process() again. Fixes: - HendiCtrl.disconnect() now catches broadly and always closes the port, matching Pololu1376.disconnect()'s already-correct pattern. - HeaterTask/StirrerTask.on_process() gain an outer retry loop as a safety net: even an unanticipated exception now logs, marks disconnected, and re-enters rather than permanently killing the task. Verified by reproducing the exact failure (comm error raising a raw OSError, not HendiException) against a fake device - the task now survives and resumes process()ing once reconnected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 21:21:44 +02:00
jensandClaude Sonnet 5	7b162cf1f5	refactor: drop firmware-version display, move Enable toggle to Controller Firmware version is already printed to the log at connect() time (HendiCtrl/Pololu1376) - remove the live GUI/websocket firmware reporting entirely (Connectable, HeaterHendi/StirrerPololu1376, Heater/StirrerTask, web GUI) as redundant. Also move the Heater panel's Closed-loop checkbox into the Controller panel and rename it "Enable" - the Controller panel now shows a live Enabled (green) / Disabled (gray) status badge next to its heading, matching the Connected/Disconnected badges on the Heater/ Stirrer panels. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 20:23:55 +02:00
jensandClaude Sonnet 5	7377fe3b6b	fix: move heater/stirrer connection status next to the panel heading The Connected/Disconnected badge, F/W version text, and both Connect/ Disconnect buttons were crammed into one row, clipping the Disconnect button off-panel. Move the status badge into the h3 heading itself (right-aligned) so the row below has room for the F/W text and both buttons. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 20:03:52 +02:00
jensandClaude Sonnet 5	069fc352bc	fix: guard connection-state callbacks against no running event loop The previous hotfix's disconnect()-on-comm-error path can now fire during server/brewpi.py's own final best-effort hardware-release cleanup (finally: block's heater.close()/stirrer.activate(False)), which runs after the event loop has already stopped - previously nothing about hardware teardown touched an AttributeChange-tracked field, so this path was dormant. asyncio.create_task() there raises RuntimeError: no running event loop, crashing the shutdown sequence. Adds fire_and_forget() (tasks/task.py) - asyncio.create_task() that silently drops the message instead of raising when there's no loop to schedule on - and uses it for the Connected/FirmwareVersion broadcasts. SudTask.check_connections() also now no-ops entirely without a running loop, since self.sud.stop() cascades into further unguarded create_task() calls (on_state_changed, heater_task.shutdown(), ...) that a stopped loop can't help with anyway. Reproduced and verified fixed directly against HeaterTask/HeaterHendi with no event loop running (mirrors the exact server/brewpi.py shutdown-time call path that crashed on the brewpi Pi). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 19:47:17 +02:00
jensandClaude Sonnet 5	0877e7754b	fix: guard HeaterHendi/StirrerPololu1376 activate() against comm errors Live-testing on the brewpi Pi found that a connected-but-unresponsive device (e.g. the hendi sitting in its ungraceful-disconnect lockout, see docs/hendi_lockout_findings.md) crashed the whole server: activate() - called both from HeaterTask's `with device.open():` and from a client's Connect/Disconnect command - raised uncaught, escaping the gathered task and killing the entire asyncio.gather in TaskManager.start(). activate()/is_activated()/process() on both real actors now catch comm exceptions and self-heal via disconnect() (the same path a genuine unplug already takes), instead of letting the exception propagate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YaPLuRPpyjWcwhMvCvpHCL	2026-07-03 19:39:07 +02:00
jens	1db0e66ab7	Merge pull request 'Add connect/disconnect status for real heater/stirrer hardware' (#3 ) from observer_refactor into master Reviewed-on: http://192.168.22.90:3001/jayfield/brewpi/pulls/3	2026-07-03 19:26:15 +02:00