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})