feat: Allow heating of the boiler in standby#615
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a configurable standby heating temperature: a new ChangesStandby Temperature Feature
Heating Theme Color Change
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/src/pages/OTA/index.jsx (1)
67-83: Consider moving relateduseStatedeclarations above thisuseEffect.
setRebuildProgress,setRebuilding, andsetRebuiltare used in this effect's callback but their correspondinguseStatecalls are declared further down at lines 111–113. While this is safe at runtime (the closure fires after the component function completes, past theconstTDZ), it's non-idiomatic and makes the dependency between these hooks harder to follow at a glance.♻️ Suggested reordering
+ const [rebuilding, setRebuilding] = useState(false); + const [rebuilt, setRebuilt] = useState(false); + const [rebuildProgress, setRebuildProgress] = useState({ total: 0, current: 0, status: '' }); + useEffect(() => { const listenerId = apiService.on('evt:history-rebuild-progress', msg => { setRebuildProgress({ ... }); ... }); ... }, [apiService]); // ... (remove the duplicate declarations at lines 111-113) - const [rebuilding, setRebuilding] = useState(false); - const [rebuilt, setRebuilt] = useState(false); - const [rebuildProgress, setRebuildProgress] = useState({ total: 0, current: 0, status: '' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/OTA/index.jsx` around lines 67 - 83, Move the related useState declarations so they appear before the useEffect that references them: locate the useState hooks that create setRebuildProgress, setRebuilding, and setRebuilt and place them above the useEffect block (the one registering 'evt:history-rebuild-progress') so hook declaration order is consistent and easier to read; ensure you only reorder those useState calls (not change their names) and keep the effect implementation and apiService dependency unchanged.web/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.md (1)
15-15: Nit: Simplify redundant "points in time" phrasing (×2).LanguageTool flags both occurrences of "points in time" as redundant — "points" alone conveys the same meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.md` at line 15, Simplify redundant phrasing by replacing both occurrences of "points in time" with "points" in the sentence that begins "At the three points in time at which we examine the data..." inside PhaseEndStop_Algorithm_English.md so it reads "At the three points at which we examine the data..." (and similarly change the later "these three points in time" to "these three points") to remove repetition while preserving meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/display/core/Settings.cpp`:
- Around line 427-430: The setter Settings::setStandbyTemperature should
validate and clamp incoming values before persisting; update it to clamp
standby_temperature to a safe range (use std::clamp with named constants like
MIN_STANDBY_TEMPERATURE and MAX_STANDBY_TEMPERATURE declared in the Settings
class or file) and then assign to standbyTemperature and call save(); if those
constants don’t exist, add them with appropriate bounds and use them in the
setter to ensure out-of-range (negative or excessively high) values are
normalized at the source of truth.
In `@web/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.md`:
- Line 3: Remove or replace the empty "##" heading in
PhaseEndStop_Algorithm_English.md: either delete the bare "##" line if no
section is needed, or replace it with a meaningful heading title (e.g., "##
Phase End Stop Algorithm") or with a visual separator "---" if the intent was
solely visual; update the single "##" token in the file accordingly.
In `@web/src/pages/Statistics/utils/statisticsSearchDsl.js`:
- Around line 320-327: The free-text token list for __free currently returns
shotMeta fields including profile but omits profileName, causing __free queries
to miss matches that explicit profile: filters would catch; update the array
returned in the __free construction (the code that returns [shotMeta?.name,
shotMeta?.label, shotMeta?.title, shotMeta?.profile, shotMeta?.id,
shotMeta?.source]) to also include shotMeta?.profileName so free-text searches
cover the same profile data as field-specific matching.
---
Nitpick comments:
In `@web/src/pages/OTA/index.jsx`:
- Around line 67-83: Move the related useState declarations so they appear
before the useEffect that references them: locate the useState hooks that create
setRebuildProgress, setRebuilding, and setRebuilt and place them above the
useEffect block (the one registering 'evt:history-rebuild-progress') so hook
declaration order is consistent and easier to read; ensure you only reorder
those useState calls (not change their names) and keep the effect implementation
and apiService dependency unchanged.
In `@web/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.md`:
- Line 15: Simplify redundant phrasing by replacing both occurrences of "points
in time" with "points" in the sentence that begins "At the three points in time
at which we examine the data..." inside PhaseEndStop_Algorithm_English.md so it
reads "At the three points at which we examine the data..." (and similarly
change the later "these three points in time" to "these three points") to remove
repetition while preserving meaning.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
src/display/core/Controller.cppsrc/display/core/Settings.cppsrc/display/core/Settings.hsrc/display/plugins/WebUIPlugin.cppsrc/display/ui/default/DefaultUI.cppsrc/display/ui/default/lvgl/screens/ui_StandbyScreen.csrc/display/ui/default/lvgl/screens/ui_StandbyScreen.hsrc/display/ui/default/lvgl/ui_themes.cppui/GaggiMate.sllui/GaggiMate.spjui/Themes.sltui/project.infoweb/src/index.jsxweb/src/pages/OTA/index.jsxweb/src/pages/Settings/index.jsxweb/src/pages/ShotAnalyzer/components/AnalysisTable.jsxweb/src/pages/ShotAnalyzer/components/LibraryPanel.jsxweb/src/pages/ShotAnalyzer/components/NotesBar.jsxweb/src/pages/ShotAnalyzer/components/NotesBarExpanded.jsxweb/src/pages/ShotAnalyzer/components/ShotChart.jsxweb/src/pages/ShotAnalyzer/components/StatusBar.jsxweb/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.mdweb/src/pages/ShotAnalyzer/index.jsxweb/src/pages/ShotAnalyzer/services/AnalyzerService.jsweb/src/pages/ShotAnalyzer/utils/analyzerUtils.jsweb/src/pages/Statistics/components/MetricsTable.jsxweb/src/pages/Statistics/components/PhaseStatistics.jsxweb/src/pages/Statistics/components/StatisticsMultiSelectDropdown.jsxweb/src/pages/Statistics/components/StatisticsSearchHelp.jsxweb/src/pages/Statistics/components/StatisticsToolbar.jsxweb/src/pages/Statistics/components/StatisticsView.jsxweb/src/pages/Statistics/components/SummaryCards.jsxweb/src/pages/Statistics/components/TrendChart.jsxweb/src/pages/Statistics/utils/statisticsRoute.jsweb/src/pages/Statistics/utils/statisticsSearchDsl.jsweb/src/pages/Statistics/utils/trendBuckets.jsweb/src/style.css
| void Settings::setStandbyTemperature(int standby_temperature) { | ||
| standbyTemperature = standby_temperature; | ||
| save(); | ||
| } |
There was a problem hiding this comment.
Clamp standby temperature in the backend setter.
Line 427 currently persists any integer (including negative or very high values). Since this value drives standby boiler targeting, add range validation at the source-of-truth setter.
🔧 Proposed fix
void Settings::setStandbyTemperature(int standby_temperature) {
- standbyTemperature = standby_temperature;
+ constexpr int kMinStandbyTemperature = 0;
+ constexpr int kMaxStandbyTemperature = 160; // keep aligned with controller/boiler safety limits
+ standbyTemperature = std::clamp(standby_temperature, kMinStandbyTemperature, kMaxStandbyTemperature);
save();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Settings::setStandbyTemperature(int standby_temperature) { | |
| standbyTemperature = standby_temperature; | |
| save(); | |
| } | |
| void Settings::setStandbyTemperature(int standby_temperature) { | |
| constexpr int kMinStandbyTemperature = 0; | |
| constexpr int kMaxStandbyTemperature = 160; // keep aligned with controller/boiler safety limits | |
| standbyTemperature = std::clamp(standby_temperature, kMinStandbyTemperature, kMaxStandbyTemperature); | |
| save(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/display/core/Settings.cpp` around lines 427 - 430, The setter
Settings::setStandbyTemperature should validate and clamp incoming values before
persisting; update it to clamp standby_temperature to a safe range (use
std::clamp with named constants like MIN_STANDBY_TEMPERATURE and
MAX_STANDBY_TEMPERATURE declared in the Settings class or file) and then assign
to standbyTemperature and call save(); if those constants don’t exist, add them
with appropriate bounds and use them in the setter to ensure out-of-range
(negative or excessively high) values are normalized at the source of truth.
| 3. In Step 3, Step 2 is repeated with the second sample of the following phase. If the correct target value is found here, the parameter is set and it is indicated that a review may be necessary. | ||
| # PhaseEndStop_Algorithm_English | ||
|
|
||
| ## |
There was a problem hiding this comment.
Remove or replace the empty ## heading.
An ## with no title text renders as an empty heading element in markdown, which is semantically incorrect and can cause accessibility issues with screen readers. If the intent is a visual separator, use --- instead; otherwise, remove it.
📝 Proposed fix
-##
-
## Simplified Explanation:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## | |
| ## Simplified Explanation: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/ShotAnalyzer/docs/PhaseEndStop_Algorithm_English.md` at line 3,
Remove or replace the empty "##" heading in PhaseEndStop_Algorithm_English.md:
either delete the bare "##" line if no section is needed, or replace it with a
meaningful heading title (e.g., "## Phase End Stop Algorithm") or with a visual
separator "---" if the intent was solely visual; update the single "##" token in
the file accordingly.
| return [ | ||
| shotMeta?.name, | ||
| shotMeta?.label, | ||
| shotMeta?.title, | ||
| shotMeta?.profile, | ||
| shotMeta?.id, | ||
| shotMeta?.source, | ||
| ]; |
There was a problem hiding this comment.
Free-text search misses profileName, creating inconsistent query behavior.
At Line 320-327, __free includes profile but not profileName, while profile: matching checks both. This can cause free-text queries to miss results that field queries would return.
💡 Suggested fix
case '__free':
return [
shotMeta?.name,
shotMeta?.label,
shotMeta?.title,
+ shotMeta?.fileName,
+ shotMeta?.exportName,
shotMeta?.profile,
+ shotMeta?.profileName,
shotMeta?.id,
shotMeta?.source,
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return [ | |
| shotMeta?.name, | |
| shotMeta?.label, | |
| shotMeta?.title, | |
| shotMeta?.profile, | |
| shotMeta?.id, | |
| shotMeta?.source, | |
| ]; | |
| return [ | |
| shotMeta?.name, | |
| shotMeta?.label, | |
| shotMeta?.title, | |
| shotMeta?.fileName, | |
| shotMeta?.exportName, | |
| shotMeta?.profile, | |
| shotMeta?.profileName, | |
| shotMeta?.id, | |
| shotMeta?.source, | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Statistics/utils/statisticsSearchDsl.js` around lines 320 -
327, The free-text token list for __free currently returns shotMeta fields
including profile but omits profileName, causing __free queries to miss matches
that explicit profile: filters would catch; update the array returned in the
__free construction (the code that returns [shotMeta?.name, shotMeta?.label,
shotMeta?.title, shotMeta?.profile, shotMeta?.id, shotMeta?.source]) to also
include shotMeta?.profileName so free-text searches cover the same profile data
as field-specific matching.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/display/core/Settings.cpp (1)
427-430:⚠️ Potential issue | 🟠 MajorClamp
standby_temperatureat the setter.The setter persists any integer, including negative values or values exceeding the boiler's safe operating range. Since this value directly drives the boiler target in standby via
Controller::getTargetTemp(), validate here (e.g.,std::clampwithMIN_TEMP/MAX_TEMPor dedicated bounds) rather than relying on callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/display/core/Settings.cpp` around lines 427 - 430, Clamp the incoming value in Settings::setStandbyTemperature before assigning and saving: validate the standby_temperature parameter against the allowed min/max (e.g., use std::clamp with MIN_TEMP and MAX_TEMP or explicit bounds) and assign the clamped result to the standbyTemperature member, then call save(); this ensures Controller::getTargetTemp() will only ever see a safe persisted standby temperature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/display/core/Controller.cpp`:
- Around line 411-412: getTargetTemp returns the standby temperature for
MODE_STANDBY but setTargetTemp lacks a MODE_STANDBY branch, so
raiseTemp()/lowerTemp() calls are ignored in standby; update
Controller::setTargetTemp to handle MODE_STANDBY by routing changes to
settings.setStandbyTemperature(...) (or otherwise persist the standby temp via
the settings API) and ensure the WebUI plugin (the handler that forwards remote
raise/lower requests) checks current mode (MODE_STANDBY) and either blocks
requests or routes them to the new standby-temperature path to prevent silent
drops.
---
Duplicate comments:
In `@src/display/core/Settings.cpp`:
- Around line 427-430: Clamp the incoming value in
Settings::setStandbyTemperature before assigning and saving: validate the
standby_temperature parameter against the allowed min/max (e.g., use std::clamp
with MIN_TEMP and MAX_TEMP or explicit bounds) and assign the clamped result to
the standbyTemperature member, then call save(); this ensures
Controller::getTargetTemp() will only ever see a safe persisted standby
temperature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46d90642-882a-4d96-9bd4-d11b1bd56a91
📒 Files selected for processing (5)
src/display/core/Controller.cppsrc/display/core/Settings.cppsrc/display/core/Settings.hsrc/display/plugins/WebUIPlugin.cppsrc/display/ui/default/DefaultUI.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/display/plugins/WebUIPlugin.cpp
- src/display/core/Settings.h
- src/display/ui/default/DefaultUI.cpp
| case MODE_STANDBY: | ||
| return settings.getStandbyTemperature(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\b(raiseTemp|lowerTemp)\s*\('Repository: jniebuhr/gaggimate
Length of output: 3661
🏁 Script executed:
cat -n src/display/core/Controller.cpp | sed -n '410,440p'Repository: jniebuhr/gaggimate
Length of output: 1144
🏁 Script executed:
rg -n "MODE_STANDBY" src/display/core/Controller.cpp -A 2 -B 2Repository: jniebuhr/gaggimate
Length of output: 1802
🏁 Script executed:
fd -e cpp -e h | xargs rg -l "standby.*[Ss]creen|Standby.*[Ss]creen" | head -10Repository: jniebuhr/gaggimate
Length of output: 221
🏁 Script executed:
cat -n src/display/ui/default/lvgl/screens/ui_StandbyScreen.hRepository: jniebuhr/gaggimate
Length of output: 1179
🏁 Script executed:
rg -n "StandbyScreen|standby" src/display/ui/default/DefaultUI.cpp | head -20Repository: jniebuhr/gaggimate
Length of output: 1816
🏁 Script executed:
rg -n "onBrewTempRaise\|onBrewTempLower\|onSteamTempRaise\|onSteamTempLower" src/display/ui/default/lvgl/ui_events.cpp -B 5 -A 2Repository: jniebuhr/gaggimate
Length of output: 44
🏁 Script executed:
rg -n "onBrewTempRaise|onSteamTempRaise|onBrewTempLower|onSteamTempLower" src/display/ui/default/lvgl/ui_events.cpp -B 10 -A 2 | head -60Repository: jniebuhr/gaggimate
Length of output: 1038
🏁 Script executed:
rg -n "lv_obj_add_event_cb.*Temp" src/display/ui/default/lvgl/ -r 'match'Repository: jniebuhr/gaggimate
Length of output: 455
🏁 Script executed:
fd -e cpp -o -e h | xargs rg "ui_StandbyScreen.*event\|StandbyScreen.*raiseTemp\|StandbyScreen.*lowerTemp"Repository: jniebuhr/gaggimate
Length of output: 180
🏁 Script executed:
rg -n "req:raise-temp|req:lower-temp" src/display/plugins/WebUIPlugin.cpp -B 5 -A 3Repository: jniebuhr/gaggimate
Length of output: 719
🏁 Script executed:
rg -n "MODE_STANDBY.*raise|MODE_STANDBY.*lower|getMode\(\).*STANDBY" src/display/plugins/WebUIPlugin.cppRepository: jniebuhr/gaggimate
Length of output: 44
raiseTemp()/lowerTemp() won't persist when in standby.
getTargetTemp() returns the standby temperature for MODE_STANDBY, but setTargetTemp() (lines 418-434) has no MODE_STANDBY case, so adjustments via raiseTemp()/lowerTemp() while in standby are silently dropped. The regular UI mitigates this by not exposing temperature controls on the standby screen. However, the WebUI plugin (lines 316-319) lacks a guard against temperature adjustment requests in standby mode—if the WebUI exposes such controls, consider either routing to settings.setStandbyTemperature(...) or adding a mode check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/display/core/Controller.cpp` around lines 411 - 412, getTargetTemp
returns the standby temperature for MODE_STANDBY but setTargetTemp lacks a
MODE_STANDBY branch, so raiseTemp()/lowerTemp() calls are ignored in standby;
update Controller::setTargetTemp to handle MODE_STANDBY by routing changes to
settings.setStandbyTemperature(...) (or otherwise persist the standby temp via
the settings API) and ensure the WebUI plugin (the handler that forwards remote
raise/lower requests) checks current mode (MODE_STANDBY) and either blocks
requests or routes them to the new standby-temperature path to prevent silent
drops.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/GaggiMateController/src/ControllerConfig.h`:
- Around line 222-223: The trailing comment next to the ControllerConfig
GM_STANDARD_REV_3X constant is stale after changing autodetectValue to 6; update
or remove it and add a brief note explaining why 6 was chosen (and why 5 is
skipped) so future readers understand the detection mapping in detectBoard(),
and ensure no other entries like GM_STANDARD_REV_2X still clash with this value;
also add a short sentence to the PR description noting that hardware-detection
behavior was changed (autodetectValue change) for traceability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b137ba42-e187-4be3-be18-63a8ca0a09c6
📒 Files selected for processing (1)
lib/GaggiMateController/src/ControllerConfig.h
|
# Conflicts: # src/display/core/Settings.cpp # src/display/core/Settings.h # src/display/ui/default/DefaultUI.cpp # src/display/ui/default/lvgl/screens/ui_StandbyScreen.c # src/display/ui/default/lvgl/screens/ui_StandbyScreen.h # ui/GaggiMate.sll # ui/GaggiMate.spj # ui/project.info # web/src/index.jsx # web/src/pages/Settings/index.jsx # web/src/pages/ShotAnalyzer/components/AnalysisTable.jsx # web/src/pages/ShotAnalyzer/components/LibraryPanel.jsx # web/src/pages/ShotAnalyzer/components/NotesBar.jsx # web/src/pages/ShotAnalyzer/components/NotesBarExpanded.jsx # web/src/pages/ShotAnalyzer/components/ShotChart.jsx # web/src/pages/ShotAnalyzer/components/StatusBar.jsx # web/src/pages/ShotAnalyzer/index.jsx # web/src/pages/ShotAnalyzer/services/AnalyzerService.js # web/src/pages/Statistics/components/MetricsTable.jsx # web/src/pages/Statistics/components/StatisticsMultiSelectDropdown.jsx # web/src/pages/Statistics/components/StatisticsSearchHelp.jsx # web/src/pages/Statistics/components/StatisticsToolbar.jsx # web/src/pages/Statistics/components/StatisticsView.jsx # web/src/pages/Statistics/components/SummaryCards.jsx # web/src/pages/Statistics/components/TrendChart.jsx # web/src/pages/Statistics/utils/statisticsSearchDsl.js
jniebuhr
force-pushed
the
feature/standby-heating
branch
from
06807e9 to
70b3127
Compare
|
1 participant
Summary by CodeRabbit
New Features
Style