From 2bdd3203cf58c520409765ad3a4569061b5c1130 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Mon, 22 Jun 2026 09:35:53 +0200 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01LhiQe64F74uHV8jzuoSa5K --- README.md | 40 ++++++++++++------- client/brewpi_gui.py | 4 +- components/TODO.md | 53 ++++++++++++++------------ components/apid.py | 7 ++++ components/pid/temp_controller_base.py | 14 +++++++ components/sud.py | 39 +++++++++++-------- components/sud_forecast.py | 13 ++----- scripts/demos/sud/demo_sud.py | 9 ++--- tasks/sud.py | 39 +++++++++---------- 9 files changed, 126 insertions(+), 92 deletions(-) diff --git a/README.md b/README.md index 578a324..e49ccd4 100644 --- a/README.md +++ b/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 diff --git a/client/brewpi_gui.py b/client/brewpi_gui.py index e4acc0f..2e6e751 100755 --- a/client/brewpi_gui.py +++ b/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) diff --git a/components/TODO.md b/components/TODO.md index a0d9f53..e7296fe 100644 --- a/components/TODO.md +++ b/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. diff --git a/components/apid.py b/components/apid.py index ada426e..1f5a8b1 100644 --- a/components/apid.py +++ b/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 diff --git a/components/pid/temp_controller_base.py b/components/pid/temp_controller_base.py index 24daade..bedcbe7 100644 --- a/components/pid/temp_controller_base.py +++ b/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 diff --git a/components/sud.py b/components/sud.py index 93a66ca..ecc5638 100644 --- a/components/sud.py +++ b/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: - 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 - + # 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 self.user_message = next_step.get('user_message') self.step = next_step diff --git a/components/sud_forecast.py b/components/sud_forecast.py index 3fd2fc3..a213080 100644 --- a/components/sud_forecast.py +++ b/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 diff --git a/scripts/demos/sud/demo_sud.py b/scripts/demos/sud/demo_sud.py index 6551c19..7685a8b 100644 --- a/scripts/demos/sud/demo_sud.py +++ b/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: diff --git a/tasks/sud.py b/tasks/sud.py index 53fb635..d1a6252 100644 --- a/tasks/sud.py +++ b/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: - self.sud.temp_reached() + 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)