jens/brewpi
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix Progress tab highlighting the wrong step on reconnect/Save

Browse Source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qvu5giu7gvRCyEWzf2Vpx
This commit is contained in:
jens
2026-06-24 18:15:45 +02:00
co-authored by Claude Sonnet 4.6
parent 6faf1a1f12
commit 8896f216d1
2 changed files with 82 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/brewpi_gui.py
+39 -16
View File
@@ -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()
tasks/sud.py
+43 -1
View File
@@ -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})