From 8896f216d131731b3b3b21d3173318e4bcbbc644 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Wed, 24 Jun 2026 18:15:45 +0200 Subject: [PATCH] Fix Progress tab highlighting the wrong step on reconnect/Save Reconnecting to (or any client-side Save fetch of) an already-running Sud showed the wrong step active and bogus remaining times, traced to two independent bugs: - recv()'s 'Save' handler always called send_forecast(doc), which resimulates the *entire* schedule cold from step 0 - fine for a not-yet-started Sud, but for one already running it silently overwrote the forecast/forecast_step_starts that _reanchor_forecast() had been accurately, continuously maintaining with a context-free "starting now" guess. Now skipped whenever a run is already in progress, re-sending what's already there instead. - _reanchor_forecast() recomputes each step's real start time asynchronously, so that recomputation can itself be (and routinely is) superseded and discarded by a later transition's reanchor before ever committing - and the next *successful* commit's "everything before my index is real" filter would then preserve whatever stale prediction was sitting there before, sometimes for a step that hadn't even happened yet by the schedule's real position. on_step_changed() now also records each step's start synchronously, immune to that race. - Client-side, update_sud_forecast() unconditionally reset sud_step_ index/forecast_step_starts/etc. on every 'Json' push - correct for an actual Load, but connect()'s unconditional Save request produces a standalone 'Json' push (no Step/State alongside it, unlike the subscribe-triggered replay) for the *same* already-running schedule, wiping the correct state with nothing left to restore it. Now only resets when the incoming doc actually differs from the last one processed. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_019qvu5giu7gvRCyEWzf2Vpx --- client/brewpi_gui.py | 55 +++++++++++++++++++++++++++++++------------- tasks/sud.py | 44 ++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/client/brewpi_gui.py b/client/brewpi_gui.py index e4f446c..bbf4e0e 100755 --- a/client/brewpi_gui.py +++ b/client/brewpi_gui.py @@ -436,6 +436,12 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): self.sud_step_descr = None self.sud_step_type = None self.sud_hold_remaining = 0.0 + # The last doc update_sud_forecast() actually processed - lets it + # tell a genuinely different schedule (a real Load/New Sud) apart + # from a standalone re-fetch of the *same*, already-running one + # (e.g. connect()'s unconditional Save request) - see its own + # comment for why that distinction matters. + self.sud_last_loaded_doc = None # [(t_min, theta), ...] actually measured since the current run # started - the "actual" half of the forecast-vs-actual # comparison (SudForecastPlot.show_dynamic()). @@ -1158,28 +1164,45 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): name, _, schedule, pot_mass, _, _, _, grain_mass, water_mass = Sud._parse_data(doc) except (KeyError, TypeError): return + # A genuinely different schedule (an actual Load, or "New Sud") + # means no step has actually started yet, regardless of whatever + # the previously loaded Sud last reported - don't wait for the + # server's own 'Step' (None) reset (asyncio.create_task() there + # means its arrival order relative to this doc isn't guaranteed) + # to clear a stale step from the *previous* schedule before + # showing this one's preview below. + # + # But this same 'Json' also arrives for the *same*, already- + # running schedule - e.g. connect()'s unconditional Save request, + # whose server-side reply is two standalone pushes (this Json, + # then a Forecast - see tasks/sud.py's recv()), unlike the bundled + # replay a subscribe triggers, which carries Step/State alongside + # it to immediately fix up any such reset. Without this doc + # equality check, that standalone Json would wipe sud_step_index + # back to None with nothing in the same message to restore it - + # exactly what made a freshly-connected client briefly highlight + # (and then, once the next real Step/Elapsed push finally + # arrived, fail to correct) the wrong step. + is_new_schedule = (doc != self.sud_last_loaded_doc) + self.sud_last_loaded_doc = doc self.sud_name = name self.sud_schedule = schedule self.sud_pot_mass = pot_mass self.sud_grain_mass = grain_mass self.sud_water_mass = water_mass - # A fresh Load means no step has actually started yet, regardless - # of whatever the previously loaded Sud last reported - don't wait - # for the server's own 'Step' (None) reset (asyncio.create_task() - # there means its arrival order relative to this doc isn't - # guaranteed) to clear a stale step from the *previous* schedule - # before showing this one's preview below. - self.sud_step_descr = None - self.sud_step_index = None - self.sud_step_type = None - self.sud_hold_remaining = 0.0 self.sud_empty = len(schedule) == 0 - # Belongs to whatever schedule was loaded before - on_sud_forecast_ - # received() re-fills these once a forecast for *this* one arrives. - self.sud_forecast_step_starts = {} - self.sud_forecast_finished = False - self.sud_forecast_final_t = None - self.step_actual_frozen = {} + if is_new_schedule: + self.sud_step_descr = None + self.sud_step_index = None + self.sud_step_type = None + self.sud_hold_remaining = 0.0 + # Belongs to whatever schedule was loaded before - on_sud_ + # forecast_received() re-fills these once a forecast for + # *this* one arrives. + self.sud_forecast_step_starts = {} + self.sud_forecast_finished = False + self.sud_forecast_final_t = None + self.step_actual_frozen = {} self._rebuild_step_plates() self.update_sud_actions() self.update_status_step_label() diff --git a/tasks/sud.py b/tasks/sud.py index 9f7ee73..14b5d24 100644 --- a/tasks/sud.py +++ b/tasks/sud.py @@ -137,6 +137,31 @@ class SudTask(ATask): self.tc.set_theta_soll(step['temperature']) self.tc.set_heatrate_soll(ramp['rate']) self.apply_stirrer(phase) + # Records the moment this step actually began *synchronously* - + # only on the genuine first entry (ramping, per Sud._advance() + # unconditionally setting state to RAMPING first - see this + # method's own comment above), not the same step's later ramp-> + # hold switch, so this can't itself get overwritten by that + # switch's slightly later timestamp. Unconditional, not + # setdefault: an *earlier* reanchor's own forward-looking + # simulation may already have written a prediction for this + # same index (it simulates every remaining step from its own + # anchor point, real ones included) - that guess must be + # replaced now that the real thing has actually happened, + # never left to linger as if it still were one. + # + # _reanchor_forecast() recomputes this same entry too, but + # asynchronously, so it can be (and routinely is, e.g. on the + # very next step boundary arriving before its own simulation + # finishes) superseded and discarded before ever committing - + # see its own comment. Without this synchronous copy, a later + # reanchor's "everything before my own index is real and + # immutable" filter would then preserve whatever *that* + # discarded call's predecessor had left behind instead - a + # stale prediction, sometimes even one that hasn't happened + # yet by the schedule's real current position. + if ramping: + self.forecast_step_starts[self.sud.index] = self.sud.elapsed # Every real step boundary (full step change or ramp->hold # within one) is a trustworthy checkpoint to correct the # forecast against - see _reanchor_forecast(). Catches drift @@ -370,7 +395,24 @@ class SudTask(ATask): elif 'Save' in pair[0]: doc = self.sud.save() await self.send({'Json': doc}) - await self.send_forecast(doc) + # A run already in progress (e.g. a client connecting mid- + # brew, which always fires this on connect - see client/ + # brewpi_gui.py's Window.connect()) must NOT get send_ + # forecast()'s cold, from-step-0 simulation here: it knows + # nothing of the real current step/elapsed/temperature, so + # it would silently overwrite the forecast/forecast_step_ + # starts that's been accurately, continuously maintained + # by _reanchor_forecast() all along with a context-free + # "starting fresh right now" guess - the exact bug that + # made a freshly-connected client highlight/countdown the + # wrong step. Just re-send what's already there instead; + # only a genuinely not-yet-started schedule (IDLE/DONE) + # has nothing accurate yet to preserve, so it alone still + # gets the real, full computation. + if self.sud.state in (SudState.IDLE, SudState.DONE): + await self.send_forecast(doc) + else: + await self._send_forecast() elif 'Load' in pair[0]: if self.sud.load(pair[1]): await self.send({'Name': self.sud.name, 'Description': self.sud.description})