feat: warmup detection using heater power trend#562
feat: warmup detection using heater power trend#562bergetp wants to merge 7 commits intojniebuhr:masterfrom
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Per Berglund.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughHeater output is exposed and propagated as a new heaterPower value across BLE and UI: Heater gains getOutput(), GaggiMateController computes heaterPower and passes it to BLE sendSensorData, NimBLE client/server types and callbacks add heaterPower, Controller stores/exposes it, and DefaultUI implements warmup detection using heater power samples. Changes
Sequence Diagram(s)sequenceDiagram
participant Heater
participant GaggiMateCtrl as GaggiMateController
participant NimBLEServer
participant BLE as BLE_Link
participant NimBLEClient
participant DisplayCtrl as Controller
participant DefaultUI
Heater->>GaggiMateCtrl: getOutput()
GaggiMateCtrl->>GaggiMateCtrl: compute heaterPower (0-100)
GaggiMateCtrl->>NimBLEServer: sendSensorData(..., heaterPower)
NimBLEServer->>BLE: notify SENSOR_DATA (includes heaterPower)
BLE->>NimBLEClient: receive SENSOR_DATA
NimBLEClient->>NimBLEClient: parse fields (temp, pressure, ..., heaterPower)
NimBLEClient->>DisplayCtrl: sensorCallback(..., heaterPower)
DisplayCtrl->>DisplayCtrl: update heaterPower, emit boiler:heaterPower:change
DisplayCtrl->>DefaultUI: reactive handler receives heaterPower update
DefaultUI->>DefaultUI: aggregate heaterPower samples, update isWarmedUp & UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bergetp
force-pushed
the
feat/warmup-trend
branch
from
5bf1dce to
77d379d
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/display/ui/default/DefaultUI.cpp`:
- Around line 220-233: The heater power accumulation in the
pluginManager->on("boiler:heaterPower:change", ...) handler updates
heaterPowerTimeSum, heaterPowerTimeTotal and lastHeaterPowerSampleTime
non-atomically while updateTempStableFlag() (UI loop) reads them, causing a
race; fix by protecting these shared variables with a synchronization primitive:
add a mutex (e.g., heaterPowerMutex) and use lock_guard in the event handler
around updates to heaterPowerTimeSum, heaterPowerTimeTotal and
lastHeaterPowerSampleTime, and likewise lock_guard in updateTempStableFlag()
when reading/clearing them, or alternatively accumulate into a local accumulator
structure in the handler and perform a single atomic swap/assignment (or
protected assignment under the same mutex) so reads in updateTempStableFlag()
see a consistent snapshot.
🧹 Nitpick comments (1)
src/display/ui/default/DefaultUI.cpp (1)
91-98: Minor: Redundant reset before initialization.Calling
resetWarmupState()immediately before settingstableStartTimeandheaterPowerWindowStartresults in those fields being set twice. Consider either removing the redundant assignments fromresetWarmupState()in this path, or directly initializing without the reset call.This is cosmetic and doesn't affect correctness.
♻️ Optional: Inline the necessary resets
// Initialize stable state tracking when entering or re-entering stable state if (isTemperatureStable && stableStartTime == 0) { - resetWarmupState(); + isWarmedUp = false; + heaterPowerTimeSum = 0.0f; + heaterPowerTimeTotal = 0; + lastHeaterPowerSampleTime = 0; + lastHeaterPowerWindowAvg = NAN; stableStartTime = now; heaterPowerWindowStart = now;
bergetp
changed the title
bergetp
force-pushed
the
feat/warmup-trend
branch
from
77d379d to
2a6f39d
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
bergetp
force-pushed
the
feat/warmup-trend
branch
from
2a6f39d to
c9d9dd0
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/display/ui/default/DefaultUI.h`:
- Around line 80-90: The implementation in DefaultUI.cpp uses the misspelled
identifier lastHearerPowerSampleTime which doesn't match the field declared as
lastHeaterPowerSampleTime in DefaultUI.h; locate all occurrences of
lastHearerPowerSampleTime in DefaultUI.cpp (e.g., in functions
updateTempHistory(), updateTempStableFlag(), resetWarmupState() or any
heater-power sampling logic) and rename them to lastHeaterPowerSampleTime so the
implementation matches the declaration and the build error is resolved.
c9d9dd0 to
6a43847
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
3 similar comments
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/display/ui/default/DefaultUI.cpp`:
- Around line 110-118: The heating icon's opacity is only adjusted when
!isTemperatureStable, so if it was previously dimmed (heatingFlash -> LV_OPA_50)
it can remain semi-transparent after transitioning to a stable state; after
computing color using isWarmedUp and isTemperatureStable (and before returning),
explicitly set opacity for heatingIcon to LV_OPA_100 when isTemperatureStable is
true (or otherwise ensure an else branch sets lv_obj_set_style_opa(heatingIcon,
LV_OPA_100, LV_PART_MAIN | LV_STATE_DEFAULT)) so stable states always render
solid; reference symbols: heatingIcon, isWarmedUp, isTemperatureStable,
heatingFlash.
- Around line 151-164: The event handler for "boiler:heaterPower:change" only
zeroes lastHeaterPowerSampleTime on NaN or spikes, leaving heaterPowerTimeSum,
heaterPowerTimeTotal and isWarmedUp stale; update the
pluginManager->on("boiler:heaterPower:change", ...) handler so that when
currentHeaterPower is NaN or a spike (compare against
HEATER_POWER_SPIKE_THRESHOLD) you reset lastHeaterPowerSampleTime to 0, set
heaterPowerTimeSum = 0, heaterPowerTimeTotal = 0, and set isWarmedUp = false to
fully reset the warmup window and state. Ensure HEATER_POWER_SPIKE_THRESHOLD is
referenced (define it in the appropriate header if not present) and keep the
rest of the sampling logic unchanged for valid samples.
- Around line 76-105: The warmup logic can never mark isWarmedUp when
heaterPowerTimeTotal stays 0 (legacy controllers), so add a stable-time
fallback: introduce a timestamp variable (e.g., stableSinceMs) that is set when
isTemperatureStable first becomes true (and cleared in resetWarmupState()), then
in the warmup check (the block using heaterPowerWindowStart,
HEATER_POWER_WINDOW_MS, lastHeaterPowerWindowAvg, HEATER_POWER_TREND_THRESHOLD)
also check if heaterPowerTimeTotal == 0 and (now - stableSinceMs) >=
STABLE_FALLBACK_MS (set STABLE_FALLBACK_MS to 10*60*1000) and if so set
isWarmedUp = true; ensure stableSinceMs is reset whenever isTemperatureStable
becomes false or in resetWarmupState().
| pluginManager->on("boiler:heaterPower:change", [=](Event const &event) { | ||
| currentHeaterPower = event.getFloat("value"); | ||
| if (currentHeaterPower != currentHeaterPower || !isTemperatureStable || isWarmedUp) { | ||
| lastHeaterPowerSampleTime = 0; | ||
| return; | ||
| } | ||
| unsigned long now = millis(); | ||
| if (lastHeaterPowerSampleTime > 0) { | ||
| unsigned long dt = now - lastHeaterPowerSampleTime; | ||
| heaterPowerTimeSum += currentHeaterPower * dt; | ||
| heaterPowerTimeTotal += dt; | ||
| } | ||
| lastHeaterPowerSampleTime = now; | ||
| }); |
There was a problem hiding this comment.
Reset warmup on heater-power spikes and invalid samples.
Right now, NaN or a heater-power spike only zeroes lastHeaterPowerSampleTime, leaving the accumulated window and isWarmedUp untouched. This can preserve a warmed‑up state even after a ≥60% spike and keep stale averages. Resetting the warmup state on NaN/spikes will align behavior with the intended “reset on spike” rule.
✅ Proposed fix
- if (currentHeaterPower != currentHeaterPower || !isTemperatureStable || isWarmedUp) {
- lastHeaterPowerSampleTime = 0;
- return;
- }
+ if (currentHeaterPower != currentHeaterPower ||
+ currentHeaterPower >= HEATER_POWER_SPIKE_THRESHOLD ||
+ !isTemperatureStable) {
+ resetWarmupState();
+ return;
+ }
+ if (isWarmedUp) {
+ lastHeaterPowerSampleTime = 0;
+ return;
+ }(Define HEATER_POWER_SPIKE_THRESHOLD in the appropriate header/config if not already present.)
🤖 Prompt for AI Agents
In `@src/display/ui/default/DefaultUI.cpp` around lines 151 - 164, The event
handler for "boiler:heaterPower:change" only zeroes lastHeaterPowerSampleTime on
NaN or spikes, leaving heaterPowerTimeSum, heaterPowerTimeTotal and isWarmedUp
stale; update the pluginManager->on("boiler:heaterPower:change", ...) handler so
that when currentHeaterPower is NaN or a spike (compare against
HEATER_POWER_SPIKE_THRESHOLD) you reset lastHeaterPowerSampleTime to 0, set
heaterPowerTimeSum = 0, heaterPowerTimeTotal = 0, and set isWarmedUp = false to
fully reset the warmup window and state. Ensure HEATER_POWER_SPIKE_THRESHOLD is
referenced (define it in the appropriate header if not present) and keep the
rest of the sampling logic unchanged for valid samples.
There was a problem hiding this comment.
Not adding spike detection — it adds complexity without clear benefit. NaN samples are already handled: they zero lastHeaterPowerSampleTime, so the next valid sample won't accumulate a bogus delta. The accumulated window data stays intact, which is correct — throwing away a 60s window because of one bad sample would be worse. If the boiler actually spikes, temperature will go unstable and resetWarmupState() is called via updateTempStableFlag() anyway. Warmup is also latched intentionally to avoid flickering from PID noise.
There was a problem hiding this comment.
@bergetp, that makes sense—your reasoning is solid. The NaN handling via lastHeaterPowerSampleTime effectively breaks the accumulation chain without discarding valid window data, and the temperature-based reset already catches real instability. Intentional latching to avoid PID noise flickering is the right trade-off for UX. Thanks for the clarification!
✏️ Learnings added
Learnt from: bergetp
Repo: jniebuhr/gaggimate PR: 562
File: src/display/ui/default/DefaultUI.cpp:151-167
Timestamp: 2026-02-02T07:20:26.862Z
Learning: In src/display/ui/default/DefaultUI.cpp, the warmup detection (`isWarmedUp`) is intentionally latched once set to avoid flickering from PID noise. NaN heater power samples zero `lastHeaterPowerSampleTime` to prevent bogus accumulation without discarding the accumulated window data. Real heater power spikes causing instability are handled by temperature-based reset via `updateTempStableFlag()` calling `resetWarmupState()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
1 similar comment
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
bergetp
force-pushed
the
feat/warmup-trend
branch
from
b1ab601 to
200c067
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
Replace variance-plateau warmup detection with heater power trend analysis. Transmit heater power over BLE from controller, then on the display side collect 30s time-weighted average power windows and declare warmup complete when consecutive windows differ by <= 2%. - Add heater power as 6th field in BLE sensor data (backwards compatible) - Gate warmup on temperature stability with hysteresis to avoid flapping - Reset warmup on instability or heater power spike (>= 60%) - 10 min stable fallback - Consolidate currentTemp/targetTemp from int to float, removing redundant float copies - Use NAN sentinel for window state, float accumulator for thread safety Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Increase power trend window from 30s to 60s for better PID noise smoothing - Tighten trend threshold from 2% to 1% to delay warmup until truly stable - Fix heating icon opacity staying at 50% after transitioning to stable state - Always reset window after evaluation (don't stall when latched) - Fix isTemperatureStable type from int to bool - Add debug logging for heater power events Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
bergetp
force-pushed
the
feat/warmup-trend
branch
from
200c067 to
b9ad38d
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
1 participant
Summary
Adds heater‑power reporting over BLE and a warm‑up readiness indicator based on heater‑power trend convergence. Indicator: red flashing = not temp‑stable, yellow = temp‑stable but power trend not yet stable, green = power trend stable (warmed up).
Algorithm
Once latched, warmup stays green until a real state change (temp instability or setpoint change). This avoids flickering from normal PID duty‑cycle noise.
Changes
Controller
Heater::getOutput()and convert 0–1000 to 0–100% insendSensorData().Comms
Display
boiler:heaterPower:changeevent.isWarmedUpwhen the trend stabilizes.isTemperatureStabletype frominttobool.Testing
Summary by CodeRabbit
New Features
Enhancements
Chores