From 41b06d8961f1ffe472a94cd0f81f4196835400d5 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Mon, 22 Jun 2026 14:04:02 +0200 Subject: [PATCH] Eliminate the naive forecast; fix two bugs that hijacked the plot Root cause of "forecast vanishes right after loading, before Start": Sud.elapsed resets to 0 on every fresh Load too, not just on an actual run start (components/sud.py's _reset_run_state()), and that reset broadcasts unconditionally. The GUI's Elapsed handler applied it unconditionally too, flipping sud_elapsed_seconds from None to 0.0 purely from loading a schedule - which made on_plot_timer() think a run had started, hijacking the plot into show_dynamic() (empty history, visible progress line, "(est.)" title) instead of the static forecast preview. Fixed by only applying an incoming Elapsed value once the State-transition logic has already put the client into dynamic-view mode, so a mere Load while idle can't masquerade as a run starting. Separately, eliminates the naive abs(delta)/rate forecast entirely, as instructed - both the immediate static placeholder shown the instant a schedule loaded (show_schedule()/_estimate_course()) and the dynamic per-tick fallback used until the server's simulated RemainingForecast arrived after a step change. The static preview now always waits for and shows only the simulation-based forecast (show_computing() while it's pending, then show_precomputed_course()); the dynamic dashed projection simply holds its last value during that brief gap instead of guessing. _remaining_schedule() and SUD_HOLDING_STATE, now unused, removed too. Also fixes the axes-scaling bug found along the way: relim()+ autoscale_view() leaves autoscaling switched on, so a later, unrelated redraw (e.g. set_current_temp()'s continuous updates, which keep that indicator live regardless of whether a forecast is even showing) could silently re-autoscale and stretch the view out to fit it - e.g. leftover heat from a previous, different run sitting well outside a newly loaded schedule's own range. show_precomputed_course() now computes and locks explicit xlim/ylim from the forecast data alone via the new _set_fixed_limits(). Verified live: a fresh load shows only the simulated forecast, correctly scaled, with no premature dynamic-mode hijacking; loading a schedule after a much hotter previous run no longer distorts the axes to fit the leftover actual temperature. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_01LhiQe64F74uHV8jzuoSa5K --- client/brewpi_gui.py | 178 ++++++++++++++++++++----------------------- 1 file changed, 83 insertions(+), 95 deletions(-) diff --git a/client/brewpi_gui.py b/client/brewpi_gui.py index 52ea29a..33551ce 100755 --- a/client/brewpi_gui.py +++ b/client/brewpi_gui.py @@ -21,7 +21,6 @@ from user_config import UserConfig # str(SudState.X) values (as sent in the 'State' message) that count as # "running" for enabling/disabling the Start/Pause/Stop toolbar actions. SUD_RUNNING_STATES = {"SudState.RAMPING", "SudState.HOLDING", "SudState.WAIT_USER"} -SUD_HOLDING_STATE = "SudState.HOLDING" SUD_WAIT_USER_STATE = "SudState.WAIT_USER" SUD_PAUSED_STATE = "SudState.PAUSED" @@ -110,13 +109,17 @@ class RealtimePlot(FigureCanvasQTAgg): class SudForecastPlot(FigureCanvasQTAgg): - """A preview of a Sud schedule's temperature course, computed from its - nominal ramp rates/hold durations alone (no plant/controller model - - just how long the declared schedule says each step should take), so it - can be shown as soon as a schedule is loaded/edited, before a brew is - actually started (show_schedule()). + """A preview of a Sud schedule's temperature course, simulated + server-side with the same plant/controller model the real run uses + (components/sud_forecast.py's SudForecastEstimator - see + show_computing()/show_precomputed_course()). There used to also be an + immediate, naive abs(delta)/rate placeholder shown the instant a + schedule loaded, before the simulated one arrived a moment later - + removed because it was structurally wrong often enough (no model of + PID settling time, ramp dead time, etc.) to not be worth showing even + briefly. - Once running, nonideal ramp rates/user pauses make that static estimate + Once running, nonideal ramp rates/user pauses make any static estimate drift from reality - show_dynamic() instead re-anchors the projection every tick: the already-elapsed part is the actual measured trace (line, solid), and only the remaining steps are projected forward from @@ -140,64 +143,65 @@ class SudForecastPlot(FigureCanvasQTAgg): figure.tight_layout() - @staticmethod - def _estimate_course(schedule, start_theta): - """Walks the schedule, accumulating (t, theta) waypoints: ramps take - abs(delta)/rate minutes at the declared rate, holds last for their - duration at the temperature the preceding ramp reached. A step may - carry both - ramp then hold - contributing both segments in turn.""" - t = [0.0] - theta = [start_theta] - for step in schedule: - ramp = step.get('ramp') - hold = step.get('hold') - target = step.get('temperature') - if ramp is not None and target is not None: - rate = ramp.get('rate', 0) - if rate > 0: - t.append(t[-1] + abs(target - theta[-1]) / rate * 60.0) - theta.append(target) - if hold is not None: - t.append(t[-1] + hold.get('duration', 0) * 60.0) - theta.append(theta[-1]) - return t, theta - - def show_schedule(self, schedule, start_theta, name): - """Quick, approximate preview shown immediately when a schedule is - loaded - assumes every ramp instantly achieves/sustains its - declared rate, which real plants/controllers don't. Superseded by - show_precomputed_course() shortly after, once the server's - simulation-based forecast (components/sud_forecast.py) arrives.""" - t, theta = self._estimate_course(schedule, start_theta) - t_min = [seconds / 60.0 for seconds in t] - self._show_static_course(t_min, theta, name) + def show_computing(self, name): + """Shown right after a schedule loads, while the simulation-based + forecast is still being computed server-side (see + Window.on_sud_forecast_received()) - typically resolved within a + second. Deliberately shows nothing rather than an approximate + placeholder; leaves the axes' current view alone (there's nothing + meaningful to fit yet), so whatever was on screen before just + stays frozen underneath the title change until show_precomputed_ + course() replaces it.""" + self.line.set_data([], []) + self.line_projected.set_data([], []) + self.ax.set_title("{} - computing forecast...".format(name), fontsize='small') + self.figure.tight_layout() + self.draw_idle() def show_precomputed_course(self, t_min, theta, name): - """Like show_schedule(), but (t_min, theta) were already computed - by the server simulating the schedule with its real plant/ - controller - see Window.on_sud_forecast_received().""" - self._show_static_course(t_min, theta, name) - - def _show_static_course(self, t_min, theta, name): + """Static forecast computed by the server simulating the schedule + with its real plant/controller (components/sud_forecast.py) - see + Window.on_sud_forecast_received().""" self.line.set_data(t_min, theta) self.line_projected.set_data([], []) - self.ax.relim() - self.ax.autoscale_view() + self._set_fixed_limits(t_min, theta) self.ax.set_title("{} - {:.0f} min total".format(name, t_min[-1] if t_min else 0.0), fontsize='small') self.figure.tight_layout() self.draw_idle() + def _set_fixed_limits(self, t_list, theta_list): + """Explicitly sets (and, unlike relim()+autoscale_view(), locks) + the axes' view to fit exactly the given data, with matplotlib's + usual ~5% margin. Locking matters here: relim()+autoscale_view() + leaves autoscaling switched on, so a later, unrelated redraw - + e.g. set_current_temp()'s continuous updates, which keep that + indicator live regardless of whether a forecast is even showing - + can silently re-autoscale and stretch the view back out to fit it + instead, squashing the actual forecast curve down to nothing.""" + if not t_list or not theta_list: + return + t_lo, t_hi = min(t_list), max(t_list) + th_lo, th_hi = min(theta_list), max(theta_list) + t_pad = (t_hi - t_lo) * 0.05 or 1.0 + th_pad = (th_hi - th_lo) * 0.05 or 1.0 + self.ax.set_xlim(t_lo - t_pad, t_hi + t_pad) + self.ax.set_ylim(th_lo - th_pad, th_hi + th_pad) + def show_dynamic(self, history, t_rem_min, theta_rem, name): """Re-anchored forecast for a run in progress: history is the [(t_min, theta), ...] actually measured so far (drawn solid); - t_rem_min/theta_rem is the projected remainder (dashed), already - computed by the caller - either the server's simulation-based - SudForecastEstimator (preferred, see Window.on_plot_timer()) or, - until that arrives, a quick naive abs(delta)/rate fallback.""" + t_rem_min/theta_rem is the projected remainder (dashed), the + server's simulation-based SudForecastEstimator result for + whatever's left of the schedule, already anchored by the caller + (see Window.on_plot_timer()). Either can be None - meaning the + server hasn't recomputed it yet for the step just entered - in + which case the dashed line is simply left showing whatever it + last did, rather than guessing at a replacement.""" hist_t = [t for t, _ in history] hist_theta = [theta for _, theta in history] self.line.set_data(hist_t, hist_theta) - self.line_projected.set_data(t_rem_min, theta_rem) + if t_rem_min is not None: + self.line_projected.set_data(t_rem_min, theta_rem) self.ax.relim() self.ax.autoscale_view() @@ -461,15 +465,15 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): # projection further right every tick until the next # step change, instead of holding it fixed. anchor_min, t_rem, theta_rem = self.server_remaining_forecast + t_rem_min = [anchor_min + seconds / 60.0 for seconds in t_rem] + self.forecast_plot.show_dynamic(self.forecast_history, t_rem_min, theta_rem, self.sud_name) else: - # Server's simulation-based remainder hasn't arrived - # yet (it's recomputed on every step change, off the - # event loop) - fall back to the quick naive estimate - # for this one tick. - anchor_min = elapsed_min - t_rem, theta_rem = self.forecast_plot._estimate_course(self._remaining_schedule(), self.plot_temp_ist) - t_rem_min = [anchor_min + seconds / 60.0 for seconds in t_rem] - self.forecast_plot.show_dynamic(self.forecast_history, t_rem_min, theta_rem, self.sud_name) + # Server's simulation-based remainder hasn't arrived yet + # for the step just entered (recomputed off the event + # loop on every step change) - leave the dashed + # projection as whatever it last showed instead of + # guessing; it catches up within a tick or two. + self.forecast_plot.show_dynamic(self.forecast_history, None, None, self.sud_name) else: self.forecast_plot.clear_progress() @@ -478,30 +482,6 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): else: self.forecast_plot.clear_current_temp() - def _remaining_schedule(self): - """The not-yet-done part of sud_schedule, for SudForecastPlot's - dynamic re-anchoring: the current step's already-finished phase is - dropped, and its still-to-run phase is left for _estimate_course() - to size from the live theta_now/hold_remaining it's seeded with, - rather than the step's nominal start.""" - schedule = self.sud_schedule - index = self.sud_step_index - if not schedule or index is None or not (0 <= index < len(schedule)): - return [] - current = schedule[index] - rest = schedule[index + 1:] - if self.sud_state == SUD_WAIT_USER_STATE: - # The current step (both its ramp and hold, if any) is already - # done - only waiting on the user to advance. - return rest - if self.sud_state == SUD_HOLDING_STATE: - remaining_minutes = max(self.sud_hold_remaining, 0.0) / 60.0 - return [{'hold': {'duration': remaining_minutes}}] + rest - # RAMPING: _estimate_course() recomputes the ramp's remaining - # distance from whatever start_theta it's given, so the step is - # used as-is; its hold phase (if any) hasn't started yet either. - return [current] + rest - def disconnect(self): # ws_client.disconnect() blocks until the websocket's close handshake # completes (or times out, ~10s if the server doesn't ack promptly), @@ -844,10 +824,11 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): elif self.sud_schedule: # Fresh load (or reconnecting to an already-idle # server) - no run has happened yet, so the upfront - # plan starting from the current temperature is - # exactly right. - start_theta = self.plot_temp_ist if self.plot_temp_ist else 20.0 - self.forecast_plot.show_schedule(self.sud_schedule, start_theta, self.sud_name) + # plan is exactly right; on_sud_forecast_received() + # fills it in once the simulated forecast arrives + # (a fresh Save request goes out on every connect, + # so this resolves shortly either way). + self.forecast_plot.show_computing(self.sud_name) if self.sud_state == SUD_WAIT_USER_STATE and prev_state != SUD_WAIT_USER_STATE and self.sud_user_message: self.show_user_message(self.sud_user_message) @@ -867,7 +848,15 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): self.sud_hold_remaining = msg['HoldRemaining'] self.update_status_step_label() elif "Elapsed" in key: - self.sud_elapsed_seconds = msg['Elapsed'] + # Sud.elapsed resets to 0 on every fresh Load too (not just + # on an actual run start - see components/sud.py's + # _reset_run_state()) and broadcasts unconditionally - only + # apply it once the State handling above has already put + # this client into dynamic-view mode (sud_elapsed_seconds + # not None), so a mere Load while idle can't masquerade as + # a run starting. + if self.sud_elapsed_seconds is not None: + self.sud_elapsed_seconds = msg['Elapsed'] elif "RemainingForecast" in key: forecast = msg['RemainingForecast'] # Anchor to elapsed_min as of receipt - close enough to @@ -893,7 +882,7 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): # The server computes this asynchronously (it simulates the whole # schedule) and it can arrive slightly after the schedule itself - # only apply it if we're still looking at the static "not running - # yet" preview show_schedule() drew; a run already in progress (or + # yet" preview show_computing() drew; a run already in progress (or # one that's since finished) owns the plot via show_dynamic()/its # frozen last frame instead. if self.sud_empty or self.sud_elapsed_seconds is not None: @@ -923,12 +912,11 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): if self.sud_empty: self.forecast_plot.show_no_schedule() return - # No measured starting temperature is available before a brew has - # started; fall back to a plausible ambient guess. If a run is - # already in progress (e.g. reconnecting mid-brew), the next - # on_plot_timer() tick switches this over to the dynamic view. - start_theta = self.plot_temp_ist if self.plot_temp_ist else 20.0 - self.forecast_plot.show_schedule(schedule, start_theta, name) + # on_sud_forecast_received() fills this in once the simulated + # forecast arrives. If a run is already in progress (e.g. + # reconnecting mid-brew), the next on_plot_timer() tick switches + # this over to the dynamic view instead. + self.forecast_plot.show_computing(name) def closeEvent(self, event): print("Exitting gracefully!")