Make Sud steps ramp based on the actual temperature gap, not a 'ramp' key

Previously, whether a schedule step ramped at all was decided purely by
the presence of a 'ramp' key (components/sud.py's _advance()) - a
hold-only step jumped straight into its hold countdown, assuming the
plant was already at temperature. Now every step ramps toward
'temperature' first, for as long as the controller's own gap-tracking
FSM (TempControllerBase, already distinguishing HEAT/COOL/HOLD) says
the gap actually warrants it - via the new is_holding() (on APid and
TempControllerBase), which replaces a separate, redundant
TEMP_REACHED_TOLERANCE constant duplicated across tasks/sud.py,
components/sud_forecast.py, and scripts/demos/sud/demo_sud.py.

set_theta_soll() now recomputes the FSM eagerly so is_holding() can't
read stale HOLD for a tick after a much-further-away target is pushed.

components/sud.py's _build_step() always synthesizes a 'ramp' block
from default.step.ramp (so 'rate' is always available), but leaves
'temperature' undefaulted - every real sude/*.json's
default.step.temperature is inert template filler, and defaulting it
would send hold-only steps chasing 0 degrees. SudTask/
SudForecastEstimator/the demo only push a new theta_soll/heatrate_soll
when a step actually specifies its own temperature; otherwise the
controller keeps whatever the previous step left running.

Also fixes a related crash this surfaced: SudTask.remaining_schedule()'s
synthetic mid-hold step dropped 'ramp' to signal "don't re-ramp" - now
that every step always carries a 'ramp' dict, dropping it left 'rate'
missing the moment the synthetic step was re-resolved by
SudForecastEstimator. Drops 'temperature' instead, which is what
actually signals "no new target" under the new model.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhiQe64F74uHV8jzuoSa5K

README.md

@@ -155,14 +155,20 @@ environment (`$WORKON_HOME`/`$BREWPI_HOME`) on a Raspberry Pi-style deployment.
 ## Mash schedules
 
 Files under `sude/` describe a brew's mash schedule ("Sud"): `pot_mass`,
-`pot_material` (fixed for the whole brew), and a `steps` list, each a ramp, a
-hold, or both, plus a step-level `temperature` (the target a ramp step ramps
-to, and what a hold step holds at):
+`pot_material` (fixed for the whole brew), and a `steps` list. Every step
+ramps toward its `temperature`, then optionally holds:
 
-- a ramp — `"ramp": {"rate": ...}` — ramp to `temperature` at `rate`
-  (°C/min), and/or
+- ramping toward `temperature` at `"ramp": {"rate": ...}` (°C/min) is not
+  opt-in - it happens for every step, for as long as the temperature
+  controller's own gap-tracking FSM (`components/pid/temp_controller_base.py`)
+  says the gap actually warrants it; a step omitting `temperature` simply
+  has no new target of its own and keeps whatever the previous step left
+  running, so the ramp resolves immediately. `ramp.rate` itself still comes
+  from `default.step.ramp` unless a step overrides it.
 - a hold — `"hold": {"duration": ...}` — hold the current target for
-  `duration` minutes (omit/`0` for an immediate step).
+  `duration` minutes (omit/`0` for an immediate step) - this part stays
+  opt-in: only a step that specifies `hold` gets a hold-duration phase
+  after the ramp.
 
 A step with both ramps to `temperature` and then holds there for `duration`
 - useful for a mash rest ("ramp to 63°C, then hold 40 min") without needing
@@ -191,8 +197,11 @@ time the current step changes, instead of deriving them once at startup.
 The top-level `default.step` object gives every field above (including the
 nested `ramp`/`hold`/`stirrer` blocks) a default value; a step in `steps`
 only needs to specify the fields it overrides — anything it omits is filled
-in from `default.step`, recursively. A step is a ramp or a hold depending on
-which of those two keys it specifies; the other is not defaulted in.
+in from `default.step`, recursively. `ramp` is always defaulted in this way
+(every step ramps); `hold` stays opt-in - only present if the step specifies
+it. `temperature` is the one exception: it's never defaulted from
+`default.step` (that's just inert template filler in every `sude/*.json`) -
+a step either specifies its own, or has none and doesn't change the target.
 
 The server always runs a `Sud` (`components/sud.py`), starting out empty (no
 schedule) - `sude/` can hold several schedule files, and a client loads one
@@ -202,9 +211,11 @@ restarts; nothing is ever read from or written to `sude/*.json` by the
 server itself, that's on the client (e.g. the GUI's File menu). `Sud`
 resolves each raw step against `default.step` and tracks the current
 (resolved) step, while `tasks/sud.py`'s `SudTask` drives the temperature
-controller's `theta_soll`/`heatrate_soll` from ramp steps (advancing once
-`theta_ist` settles close to the target), counts down hold steps'
-`duration`, and applies each step's `stirrer` block (`interval_time`/
+controller's `theta_soll`/`heatrate_soll` toward each step's target
+(advancing once the controller's own FSM reports the gap closed -
+`TempControllerBase.is_holding()` - rather than a separate ad hoc
+tolerance), counts down hold steps' `duration`, and applies each step's
+`stirrer` block (`interval_time`/
 `on_ratio` map directly onto the stirrer's cycle time/duty cycle). It
 exposes progress (current step, remaining hold time, state, any
 `user_message`) on the `"Sud"` WebSocket channel and accepts
@@ -245,9 +256,10 @@ The naive estimate is structurally optimistic - it assumes the plant
 instantly tracks the declared rate and that "reached" is instant once the
 math says so, when the real PID cascade (`theta_err -> pid_hold ->
 heatrate_soll -> pid_heat -> heater power -> actual heat rate`) needs real
-time to spin up and settle within the tight 0.2°C "reached" tolerance
-(`tasks/sud.py`'s `TEMP_REACHED_TOLERANCE`). The simulated estimate accounts
-for that (it's driven by the real controller dynamics), so it's normally
+time to spin up and settle into the controller's own `HOLD` state
+(`TempControllerBase.is_holding()`, gated by the `HoldHeat`/`HoldCool`
+thresholds). The simulated estimate accounts for that (it's driven by the
+real controller dynamics), so it's normally
 within a few percent of how the dynamic one settles once a run actually
 finishes - if the two diverge by far more than that, suspect a real control
 issue rather than a forecasting one (see the `HoldCool` threshold note in

client/brewpi_gui.py

@@ -151,9 +151,9 @@ class SudForecastPlot(FigureCanvasQTAgg):
 		for step in schedule:
 			ramp = step.get('ramp')
 			hold = step.get('hold')
-			if ramp is not None:
+			target = step.get('temperature')
+			if ramp is not None and target is not None:
 				rate = ramp.get('rate', 0)
-				target = step['temperature']
 				if rate > 0:
 					t.append(t[-1] + abs(target - theta[-1]) / rate * 60.0)
 					theta.append(target)

components/TODO.md

@@ -2,21 +2,31 @@
 
 Confirmed spec for a `Sud` schedule step's internal phase order
 (`components/sud.py`), from a design discussion clarifying how `ramp`/
-`hold`/user-confirm interact.
+`hold`/user-confirm interact, since implemented (gap-based ramping).
 
 ## Phase order
 
-`ramp` -> `hold` -> user-confirm. Each step independently opts into
-`ramp` and/or `hold` by including that key (`_build_step()`,
-`sud.py:35-45`); the other phase is skipped entirely if its key is
-absent. The `'ramp'` key's presence is the *only* trigger for ramping
-(`_advance()`, `sud.py:192-210`) - there is no diff-based decision
-(`T_actual` vs. `T_target`) anywhere that decides whether to ramp, and
-no internal setpoint-trajectory limiter in the temperature controller
-(`set_theta_soll()` is an instant, unconditional assignment,
-`components/pid/temp_controller_base.py:81-82`). The `ramp` key both
-gates ramping *and* carries its settings (`rate`); it does not merely
-override settings on top of an always-on ramp.
+`ramp` -> `hold` -> user-confirm. Every step ramps toward `temperature`
+first - this is *not* gated by whether the step has its own `ramp` block
+(`_build_step()` always synthesizes one from `default.step.ramp`,
+`sud.py:35-52`); `_advance()` (`sud.py:192-204`) always enters `RAMPING`.
+How long that actually takes depends on the real gap, decided by the
+temperature controller's own gap-tracking FSM
+(`TempControllerBase.is_holding()`, `components/pid/temp_controller_base.py`)
+rather than any fixed tolerance - `SudTask.on_process()`
+(`tasks/sud.py:200-203`) calls `sud.temp_reached()` once `tc.is_holding()`
+is true. `set_theta_soll()` recomputes the FSM eagerly
+(`temp_controller_base.py:81-89`) so `is_holding()` can't read one tick
+stale after a new target is pushed.
+
+`temperature` is the one field deliberately *not* defaulted from
+`default.step` (`sud.py:52`) - every `sude/*.json`'s
+`default.step.temperature` is inert template filler, never a real shared
+target. A step omitting it has no new target of its own: `SudTask`/
+`SudForecastEstimator`/the demo driver all skip pushing `set_theta_soll`
+in that case (`step['temperature'] is not None` guard), so the controller
+just keeps whatever the previous step left running and the ramp resolves
+immediately. `hold` stays opt-in, gated by raw presence as before.
 
 ## Duration `0`
 
@@ -42,17 +52,10 @@ message": `user_message` (display text) and `user_wait_for_continue`
 (the actual block) are independent fields - a step can show a message
 without blocking, or block silently.
 
-## Open items
+## Resolved
 
-- [ ] **Pure-hold steps never (re-)set the controller's target
-  temperature.** For a step with only `hold` (no `ramp`),
-  `tasks/sud.py`'s `on_step_changed` never calls `tc.set_theta_soll()`
-  - the guard at line 73 requires `ramp is not None`. The
-  controller's target temperature for a pure-hold step is whatever
-  was left from the previous step's ramp; there's no explicit
-  fallback to the hold step's own `temperature` field. `README.md`
-  (Mash schedules section) documents this as designed behavior
-  ("`SudTask` drives ... `theta_soll`/`heatrate_soll` from ramp
-  steps ... counts down hold steps' duration"), so it's intentional,
-  not a bug - but it means a schedule can't express a temperature
-  jump straight into a hold without a `ramp` block.
+- [x] **Pure-hold steps never (re-)set the controller's target
+  temperature.** Moot now that ramping isn't gated by the `ramp` key:
+  `SudTask.on_step_changed` pushes `set_theta_soll`/`set_heatrate_soll`
+  for *any* step that specifies its own `temperature`, not just ones
+  with an explicit `ramp` block.

components/apid.py

@@ -41,3 +41,10 @@ class APid(AttributeChange):
 	@abc.abstractmethod
 	def process(self):
 		pass
+
+	@abc.abstractmethod
+	def is_holding(self):
+		"""Whether the controller's own gap-tracking FSM considers
+		theta_ist close enough to theta_soll_set to no longer be
+		actively ramping toward it."""
+		return None

components/pid/temp_controller_base.py

@@ -80,6 +80,13 @@ class TempControllerBase(APid):
 
 	def set_theta_soll(self, value):
 		self.theta_soll_set = value
+		# Recompute the FSM right away against the new target, rather than
+		# waiting for the next process() tick - otherwise self.state can
+		# still read HOLD from the previous target for up to one tick
+		# after a much-further-away one is pushed, which would make
+		# is_holding() report "reached" instantly instead of once the gap
+		# has actually closed.
+		self.process_fsm(self.theta_soll_set - self.theta_ist)
 
 	def get_theta_soll(self):
 		return self.theta_soll
@@ -87,6 +94,13 @@ class TempControllerBase(APid):
 	def get_theta_soll_set(self):
 		return self.theta_soll_set
 
+	def is_holding(self):
+		"""Whether the FSM currently considers theta_ist close enough to
+		theta_soll_set to no longer be actively heating/cooling toward
+		it - the single source of truth for "is a ramp toward the
+		current target done" (see tasks/sud.py's SudTask)."""
+		return self.state == States.HOLD
+
 	def set_heatrate_soll(self, value):
 		self.heatrate_soll_set = value
 

components/sud.py

@@ -33,15 +33,26 @@ def _merge_defaults(default, override):
 
 
 def _build_step(number, defaults, raw_step):
-	"""Fills in a schedule step with default.step's values. A step is a
-	ramp or a hold depending on which of those keys it specifies; the other
-	is left out rather than synthesized from defaults."""
+	"""Fills in a schedule step with default.step's values. 'ramp' is
+	always synthesized from defaults, even if raw_step doesn't specify
+	one of its own - every step ramps toward 'temperature' first, for as
+	long as the controller's own gap-tracking FSM says it needs to (see
+	Sud._advance()/temp_reached()), so its settings (e.g. 'rate') always
+	have to be available. 'hold' stays opt-in: only a step that
+	specifies it gets a hold-duration phase once the ramp is done.
+
+	'temperature' is deliberately *not* defaulted from default.step (every
+	sude/*.json's default.step.temperature is just inert template filler,
+	never a real shared target) - a step omitting it has no new target of
+	its own, so it's left None, telling SudTask not to push a new
+	theta_soll and just keep whatever the previous step left running."""
 	step = {'number': number}
-	for key in ('descr', 'user_message', 'user_wait_for_continue', 'grain_mass', 'water_mass', 'temperature'):
+	for key in ('descr', 'user_message', 'user_wait_for_continue', 'grain_mass', 'water_mass'):
 		step[key] = raw_step.get(key, defaults.get(key))
-	for key in ('ramp', 'hold'):
-		if key in raw_step:
-			step[key] = _merge_defaults(defaults.get(key, {}), raw_step[key])
+	step['temperature'] = raw_step.get('temperature')
+	step['ramp'] = _merge_defaults(defaults.get('ramp', {}), raw_step.get('ramp', {}))
+	if 'hold' in raw_step:
+		step['hold'] = _merge_defaults(defaults.get('hold', {}), raw_step['hold'])
 	return step
 
 
@@ -198,14 +209,12 @@ class Sud(AttributeChange):
 			return
 
 		next_step = self.schedule[self.index]
-		if 'ramp' in next_step:
+		# Every step ramps toward 'temperature' first - whether that takes
+		# any real time depends on the actual gap, which only the
+		# temperature controller's own FSM can tell (see temp_reached()'s
+		# caller, SudTask.on_process()). 'hold' only starts counting down
+		# once the controller reports the gap closed.
 		self.state = SudState.RAMPING
-		else:
-			# "duration" is in minutes; hold_remaining (ticked by tick()'s
-			# simulated-seconds dt) is in seconds.
-			self.hold_remaining = next_step['hold'].get('duration', 0) * 60.0
-			self.state = SudState.HOLDING
-
 		self.user_message = next_step.get('user_message')
 		self.step = next_step
 

components/sud_forecast.py

@@ -2,12 +2,6 @@ from components.plant import Pot
 from components.pid import PidFactory
 from components.sud import Sud, SudState
 
-# Real schedules need real time to spin up each ramp and settle within this
-# tolerance before "reached" fires - matching tasks/sud.py's
-# TEMP_REACHED_TOLERANCE exactly is what makes this simulation's predicted
-# duration line up with the real run's.
-TEMP_REACHED_TOLERANCE = 0.2
-
 # Safety cap so a schedule whose target a step can never actually reach
 # (e.g. a "hold" colder than ambient with no active cooling) can't hang the
 # simulation forever - the estimate is simply cut off there.
@@ -65,10 +59,9 @@ class SudForecastEstimator:
 			pot.set_thermal_params(params['M'], params['C'])
 			if hasattr(tc, 'set_model_params'):
 				tc.set_model_params(params['M'], params['C'])
-			ramp = step.get('ramp')
-			if sud.state == SudState.RAMPING and ramp is not None:
+			if sud.state == SudState.RAMPING and step['temperature'] is not None:
 				tc.set_theta_soll(step['temperature'])
-				tc.set_heatrate_soll(ramp['rate'])
+				tc.set_heatrate_soll(step['ramp']['rate'])
 		sud.set_on_changed('step', on_step_changed)
 
 		t = [0.0]
@@ -83,7 +76,7 @@ class SudForecastEstimator:
 			pot.set_power(max(0, self.heater_max_power * tc.get_power()))
 
 			if sud.state == SudState.RAMPING:
-				if abs(tc.get_theta_ist() - tc.get_theta_soll_set()) < TEMP_REACHED_TOLERANCE:
+				if tc.is_holding():
 					sud.temp_reached()
 			elif sud.state == SudState.WAIT_USER:
 				# A real user's confirm time isn't predictable - count it

scripts/demos/sud/demo_sud.py

@@ -7,9 +7,6 @@ from components.plant.pot import Pot
 from components.actor.stirrersim import StirrerSim
 from components.actor.heater_sim import HeaterSim
 
-# Mirrors tasks/sud.py's SudTask, but driven synchronously for a demo run.
-TEMP_REACHED_TOLERANCE = 0.2
-
 # A real user would click "Confirm" in the GUI; auto-confirm after this many
 # simulated seconds so the demo can run to completion unattended.
 WAIT_USER_AUTO_CONFIRM_S = 30.0
@@ -85,11 +82,11 @@ if __name__ == '__main__':
 		# sud.state tells us which phase is currently active.
 		if step is not None:
 			ramping = sud.state == SudState.RAMPING
-			phase = step['ramp'] if ramping and 'ramp' in step else step.get('hold', {})
+			phase = step['ramp'] if ramping else step.get('hold', {})
 
 			print(f"Step {step['number']}: {step['descr']}")
 			apply_plant_params(step)
-			if ramping and 'ramp' in step:
+			if ramping and step['temperature'] is not None:
 				ctrl.set_theta_soll(step['temperature'])
 				ctrl.set_heatrate_soll(step['ramp']['rate'])
 			apply_stirrer(phase)
@@ -138,7 +135,7 @@ if __name__ == '__main__':
 		stirrer.process()
 
 		if sud.state == SudState.RAMPING:
-			if abs(ctrl.get_theta_ist() - ctrl.get_theta_soll_set()) < TEMP_REACHED_TOLERANCE:
+			if ctrl.is_holding():
 				sud.temp_reached()
 
 		if sud.state == SudState.WAIT_USER:

tasks/sud.py

@@ -6,9 +6,6 @@ from components import APid, AStirrer
 from components.plant import APlant
 from components.sud import Sud, SudState
 
-# How close theta_ist needs to be to theta_soll_set to count as "reached".
-TEMP_REACHED_TOLERANCE = 0.2
-
 
 class SudTask(ATask):
 	def __init__(self, sud: Sud, tc: APid, stirrer: AStirrer, pot: APlant, dt, interval, msg_handler: MsgIo,
@@ -60,26 +57,26 @@ class SudTask(ATask):
 	def on_step_changed(self, step):
 		ramp = step.get('ramp') if step else None
 		hold = step.get('hold') if step else None
-		# A step may carry both 'ramp' and 'hold' (ramp to temp, then hold) -
+		# Every step ramps to 'temperature' first, then optionally holds -
 		# self.sud.state tells us which phase is currently active; it's
 		# already up to date by the time this callback fires, both on a
 		# full step transition and on the ramp->hold phase switch within
 		# one step (components/sud.py's temp_reached()).
 		ramping = self.sud.state == SudState.RAMPING
-		phase = ramp if ramping and ramp is not None else hold
+		phase = ramp if ramping else hold
 
 		if step is not None:
 			self.apply_plant_params(step)
-			if ramping and ramp is not None:
+			if ramping and step['temperature'] is not None:
 				self.tc.set_theta_soll(step['temperature'])
 				self.tc.set_heatrate_soll(ramp['rate'])
 			self.apply_stirrer(phase)
 
 		asyncio.create_task(self.send({'Step': {
 			'Index': self.sud.index,
-			'Type': 'ramp' if ramping and ramp is not None else 'hold' if hold is not None else None,
+			'Type': 'ramp' if ramping else 'hold' if hold is not None else None,
 			'Descr': step.get('descr') if step else None,
-			'Temp': step.get('temperature') if ramp is not None else None,
+			'Temp': step.get('temperature') if step else None,
 			'Rate': ramp.get('rate') if ramp else None,
 			'Duration': hold.get('duration') if (hold is not None and not ramping) else None,
 			'WaitForUser': step.get('user_wait_for_continue', False) if step else None,
@@ -136,14 +133,20 @@ class SudTask(ATask):
 			return rest
 		if self.sud.state == SudState.HOLDING:
 			# Keep the rest of the current (already fully-resolved) step -
-			# grain_mass/water_mass/temperature/etc. - SudForecastEstimator
-			# re-resolves each step against an empty default.step, so a
-			# bare {'hold': {...}} here would leave those None instead of
-			# inherited, breaking derive_plant_params(). Only the ramp
-			# phase is actually done; drop it and override the hold's
+			# grain_mass/water_mass/etc. - SudForecastEstimator re-resolves
+			# each step against an empty default.step, so a bare
+			# {'hold': {...}} here would leave those None instead of
+			# inherited, breaking derive_plant_params(). The ramp phase is
+			# already done, so drop 'temperature' (not 'ramp' - every step
+			# always carries a 'ramp' dict now, dropping it would leave
+			# 'rate' missing the moment this synthetic step is re-resolved):
+			# without a 'temperature' of its own, the rebuilt step pushes
+			# no new target, start_theta already seeds the simulation at
+			# the target anyway, so it resolves out of its (now synthetic)
+			# ramp phase in a tick or two regardless. Override the hold's
 			# duration to what's actually left.
 			remaining_minutes = max(self.sud.hold_remaining, 0.0) / 60.0
-			synthetic = {k: v for k, v in current.items() if k != 'ramp'}
+			synthetic = {k: v for k, v in current.items() if k != 'temperature'}
 			synthetic['hold'] = {**current.get('hold', {}), 'duration': remaining_minutes}
 			return [synthetic] + rest
 		return [current] + rest
@@ -205,11 +208,7 @@ class SudTask(ATask):
 		asyncio.create_task(self.send({'Name': self.sud.name, 'Description': self.sud.description}))
 
 		while True:
-			if self.sud.state == SudState.RAMPING:
-				# Compare against the controller's own live setpoint/measurement
-				# rather than its state machine, which can still read HOLD from
-				# the previous step for one tick after a new target is pushed.
-				if abs(self.tc.get_theta_ist() - self.tc.get_theta_soll_set()) < TEMP_REACHED_TOLERANCE:
+			if self.sud.state == SudState.RAMPING and self.tc.is_holding():
 				self.sud.temp_reached()
 			self.sud.tick(self.dt)
 			await asyncio.sleep(self.interval)