Feature/cleaning schedule #483
Conversation
Remove debug menu
WalkthroughThis PR introduces a new CleaningSchedulePlugin to manage espresso machine maintenance schedules. It adds Settings fields to track backflush intervals, descaling intervals, and last-performed timestamps. The plugin integrates with existing components via WebSocket messages and event subscriptions, allowing the web UI to trigger cleaning actions and display maintenance-due notifications on the machine's display and web interface. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebUI as Web UI
participant API as WebUIPlugin
participant Plugin as CleaningSchedulePlugin
participant Settings as Settings
participant DefaultUI as DefaultUI
participant Display as Machine Display
User->>WebUI: Configure cleaning intervals
WebUI->>API: POST /api/settings (intervals)
API->>Settings: setBackflushIntervalDays, setDescalingIntervalWeeks
Settings->>Settings: doSave() to storage
Plugin->>Plugin: loop() runs periodically
Plugin->>Plugin: isBackflushDue()?
alt Due
Plugin->>DefaultUI: emit cleaning:backflush:due
DefaultUI->>Display: show "Backflush Due" notification
Display->>User: render button on standby screen
end
User->>WebUI: Click "Start Backflush"
WebUI->>API: WebSocket req:cleaning:backflush:start
API->>Plugin: trigger backflush event
Plugin->>Plugin: loadCleaningProfile(BACKFLUSH_PROFILE_ID)
Plugin->>Plugin: switch to brew mode
Note over Plugin,Settings: Cleaning cycle runs...
Plugin->>Settings: setLastBackflushTime(now)
Settings->>Settings: doSave() to storage
Plugin->>DefaultUI: emit cleaning:backflush:timer:reset
DefaultUI->>Display: clear notification
Display->>User: return to normal standby
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 4
🧹 Nitpick comments (2)
src/display/plugins/CleaningSchedulePlugin.cpp (2)
144-244: Consider refactoring to eliminate code duplication.The backflush profile creation contains significant duplication. Phases 1, 3, 5, 7, and 9 are identical, as are phases 2, 4, 6, and 8. This makes the code verbose and harder to maintain.
Consider refactoring with a helper function:
Profile CleaningSchedulePlugin::createBackflushProfile() const { Profile profile{}; profile.id = BACKFLUSH_PROFILE_ID; profile.label = "[Utility] Backflush"; profile.utility = true; profile.description = ""; profile.temperature = 93; profile.type = "standard"; // Helper lambda to create pressurize/depressurize phases auto createPhase = [](const char* name, int valve, int pumpSimple) { Phase phase{}; phase.name = name; phase.phase = PhaseType::PHASE_TYPE_BREW; phase.valve = valve; phase.duration = 10.0f; phase.pumpIsSimple = true; phase.pumpSimple = pumpSimple; return phase; }; // Add 5 pressurize/depressurize cycles (9 phases total) for (int i = 0; i < 5; i++) { profile.phases.push_back(createPhase("Pressurize", 1, 100)); if (i < 4) { // Last cycle has no depressurize phase profile.phases.push_back(createPhase("Depressurize", 0, 0)); } } return profile; }This reduces ~100 lines to ~30 while maintaining the same functionality.
390-394: Consider using static_cast for type safety.The cast from
time_t(typically a signed type) tounsigned longworks in practice but could be more explicit.Apply this diff for better type safety:
unsigned long CleaningSchedulePlugin::getCurrentTimeSeconds() const { time_t now; time(&now); - return (unsigned long)now; + return static_cast<unsigned long>(now); }Alternatively, consider keeping the time as
time_tthroughout and only using it for comparisons, avoiding the cast entirely. However, the current approach works for the use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/display/core/Controller.cpp(2 hunks)src/display/core/Settings.cpp(3 hunks)src/display/core/Settings.h(3 hunks)src/display/plugins/CleaningSchedulePlugin.cpp(1 hunks)src/display/plugins/CleaningSchedulePlugin.h(1 hunks)src/display/plugins/WebUIPlugin.cpp(3 hunks)src/display/ui/default/DefaultUI.cpp(2 hunks)src/display/ui/default/DefaultUI.h(1 hunks)web/src/pages/Home/ProcessControls.jsx(6 hunks)web/src/pages/Settings/index.jsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/display/ui/default/DefaultUI.cpp (1)
src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/core/Controller.cpp (1)
src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/core/Settings.h (1)
src/display/core/Settings.cpp (8)
setBackflushIntervalDays(469-472)setBackflushIntervalDays(469-469)setDescalingIntervalWeeks(474-477)setDescalingIntervalWeeks(474-474)setLastBackflushTime(479-482)setLastBackflushTime(479-479)setLastDescalingTime(484-487)setLastDescalingTime(484-484)
src/display/plugins/WebUIPlugin.cpp (2)
src/display/core/Controller.h (1)
pluginManager(134-134)web/src/pages/Home/ProcessControls.jsx (1)
settings(179-189)
web/src/pages/Home/ProcessControls.jsx (1)
src/display/plugins/CleaningSchedulePlugin.cpp (8)
isBackflushDue(90-103)isBackflushDue(90-90)isDescalingDue(105-118)isDescalingDue(105-105)startBackflush(58-63)startBackflush(58-58)startDescaling(65-70)startDescaling(65-65)
src/display/plugins/CleaningSchedulePlugin.h (4)
src/display/core/Controller.h (2)
Controller(21-175)pluginManager(134-134)src/display/core/PluginManager.h (1)
PluginManager(14-33)src/display/core/Plugin.h (1)
Plugin(6-12)src/display/core/Controller.cpp (4)
setup(33-92)setup(33-33)loop(250-333)loop(250-250)
src/display/plugins/CleaningSchedulePlugin.cpp (3)
src/display/plugins/WebUIPlugin.cpp (4)
setup(28-60)setup(28-28)loop(62-161)loop(62-62)src/display/core/Controller.cpp (4)
setup(33-92)setup(33-33)loop(250-333)loop(250-250)src/display/core/Controller.h (2)
pluginManager(134-134)profileManager(135-135)
web/src/pages/Settings/index.jsx (2)
web/src/index.jsx (1)
apiService(23-23)web/src/services/ApiService.js (2)
ApiServiceContext(202-202)ApiServiceContext(202-202)
🪛 Clang (14.0.6)
src/display/plugins/CleaningSchedulePlugin.h
[error] 4-4: 'display/core/Plugin.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (8)
src/display/plugins/CleaningSchedulePlugin.h (1)
1-51: LGTM! Clean plugin header structure.The header follows the plugin architecture pattern correctly with proper separation of public API and private implementation details. Const correctness is maintained for status query methods, and the interface is well-designed.
src/display/plugins/CleaningSchedulePlugin.cpp (7)
10-11: LGTM!Static profile ID constants are correctly defined.
13-46: LGTM! Setup method correctly initializes the plugin.The event listener registrations and initialization sequence are appropriate. The lambda captures using
[this]are safe given the plugin's lifecycle.
48-56: LGTM!The periodic notification check is implemented correctly with proper handling of millis() rollover.
58-88: LGTM!The cleaning action and timer reset methods are implemented correctly with appropriate logging and event notifications.
246-344: LGTM! Descaling profile is well-structured.The descaling sequence is logical and clearly defined. While phases 1 and 3 are identical (both 300ml steam flush), extracting them into a helper would provide minimal benefit given they're only duplicated once.
346-388: LGTM! Profile management and timer reset logic are correct.The helper methods properly handle profile creation, loading, and automatic timer reset upon profile completion.
396-409: LGTM!The notification event triggering logic correctly checks due status and emits appropriate events for UI updates.
| bool CleaningSchedulePlugin::isBackflushDue() const { | ||
| const Settings &settings = controller->getSettings(); | ||
| int intervalDays = settings.getBackflushIntervalDays(); | ||
| unsigned long lastBackflushTime = settings.getLastBackflushTime(); | ||
|
|
||
| if (lastBackflushTime == 0) { | ||
| return false; // Never performed, not due yet | ||
| } | ||
|
|
||
| unsigned long currentTime = getCurrentTimeSeconds(); | ||
| unsigned long daysSince = (currentTime - lastBackflushTime) / (24 * 60 * 60); | ||
|
|
||
| return daysSince >= intervalDays; | ||
| } | ||
|
|
||
| bool CleaningSchedulePlugin::isDescalingDue() const { | ||
| const Settings &settings = controller->getSettings(); | ||
| int intervalWeeks = settings.getDescalingIntervalWeeks(); | ||
| unsigned long lastDescalingTime = settings.getLastDescalingTime(); | ||
|
|
||
| if (lastDescalingTime == 0) { | ||
| return false; // Never performed, not due yet | ||
| } | ||
|
|
||
| unsigned long currentTime = getCurrentTimeSeconds(); | ||
| unsigned long weeksSince = (currentTime - lastDescalingTime) / (7 * 24 * 60 * 60); | ||
|
|
||
| return weeksSince >= intervalWeeks; | ||
| } |
There was a problem hiding this comment.
Add validation for interval values and protect against time going backwards.
Two issues need attention:
-
Missing interval validation: If
intervalDaysorintervalWeeksis 0 or negative (via corrupted settings), the due check could behave incorrectly. A zero interval would make maintenance always due (except when lastTime == 0). -
Time arithmetic underflow risk: If system time goes backwards (RTC adjustment, NTP sync, clock issues), the calculation
(currentTime - lastBackflushTime) / (24 * 60 * 60)will underflow due to unsigned arithmetic, makingdaysSinceextremely large and maintenance appear severely overdue.
Apply these diffs to add guards:
bool CleaningSchedulePlugin::isBackflushDue() const {
const Settings &settings = controller->getSettings();
int intervalDays = settings.getBackflushIntervalDays();
unsigned long lastBackflushTime = settings.getLastBackflushTime();
+ if (intervalDays <= 0) {
+ return false; // Invalid interval
+ }
+
if (lastBackflushTime == 0) {
return false; // Never performed, not due yet
}
unsigned long currentTime = getCurrentTimeSeconds();
+ if (currentTime < lastBackflushTime) {
+ return false; // Time went backwards, skip check
+ }
+
unsigned long daysSince = (currentTime - lastBackflushTime) / (24 * 60 * 60);
return daysSince >= intervalDays;
} bool CleaningSchedulePlugin::isDescalingDue() const {
const Settings &settings = controller->getSettings();
int intervalWeeks = settings.getDescalingIntervalWeeks();
unsigned long lastDescalingTime = settings.getLastDescalingTime();
+ if (intervalWeeks <= 0) {
+ return false; // Invalid interval
+ }
+
if (lastDescalingTime == 0) {
return false; // Never performed, not due yet
}
unsigned long currentTime = getCurrentTimeSeconds();
+ if (currentTime < lastDescalingTime) {
+ return false; // Time went backwards, skip check
+ }
+
unsigned long weeksSince = (currentTime - lastDescalingTime) / (7 * 24 * 60 * 60);
return weeksSince >= intervalWeeks;
}🤖 Prompt for AI Agents
In src/display/plugins/CleaningSchedulePlugin.cpp around lines 90 to 118,
validate interval values and guard against time going backwards: if intervalDays
or intervalWeeks is <= 0 treat the interval as disabled (return false), and
before subtracting check if currentTime < lastBackflushTime/lastDescalingTime —
if so treat as not due (return false) or compute zero elapsed time; implement
these checks prior to any unsigned subtraction to avoid underflow (or use signed
arithmetic after validation) and then perform the usual division and comparison.
| int CleaningSchedulePlugin::getDaysSinceLastBackflush() const { | ||
| const Settings &settings = controller->getSettings(); | ||
| unsigned long lastBackflushTime = settings.getLastBackflushTime(); | ||
|
|
||
| if (lastBackflushTime == 0) { | ||
| return -1; // Never performed | ||
| } | ||
|
|
||
| unsigned long currentTime = getCurrentTimeSeconds(); | ||
| return (currentTime - lastBackflushTime) / (24 * 60 * 60); | ||
| } | ||
|
|
||
| int CleaningSchedulePlugin::getWeeksSinceLastDescaling() const { | ||
| const Settings &settings = controller->getSettings(); | ||
| unsigned long lastDescalingTime = settings.getLastDescalingTime(); | ||
|
|
||
| if (lastDescalingTime == 0) { | ||
| return -1; // Never performed | ||
| } | ||
|
|
||
| unsigned long currentTime = getCurrentTimeSeconds(); | ||
| return (currentTime - lastDescalingTime) / (7 * 24 * 60 * 60); | ||
| } |
There was a problem hiding this comment.
Protect against time going backwards.
Similar to the due check methods, these functions need protection against time underflow when currentTime < lastTime. Without guards, the unsigned arithmetic will wrap around and return an extremely large (but incorrect) value.
Apply these diffs:
int CleaningSchedulePlugin::getDaysSinceLastBackflush() const {
const Settings &settings = controller->getSettings();
unsigned long lastBackflushTime = settings.getLastBackflushTime();
if (lastBackflushTime == 0) {
return -1; // Never performed
}
unsigned long currentTime = getCurrentTimeSeconds();
+ if (currentTime < lastBackflushTime) {
+ return 0; // Time went backwards, return 0 to avoid underflow
+ }
+
return (currentTime - lastBackflushTime) / (24 * 60 * 60);
} int CleaningSchedulePlugin::getWeeksSinceLastDescaling() const {
const Settings &settings = controller->getSettings();
unsigned long lastDescalingTime = settings.getLastDescalingTime();
if (lastDescalingTime == 0) {
return -1; // Never performed
}
unsigned long currentTime = getCurrentTimeSeconds();
+ if (currentTime < lastDescalingTime) {
+ return 0; // Time went backwards, return 0 to avoid underflow
+ }
+
return (currentTime - lastDescalingTime) / (7 * 24 * 60 * 60);
}🤖 Prompt for AI Agents
In src/display/plugins/CleaningSchedulePlugin.cpp around lines 120 to 142, both
getDaysSinceLastBackflush() and getWeeksSinceLastDescaling() can underflow when
currentTime < lastBackflushTime/lastDescalingTime because of unsigned
arithmetic; add a check after obtaining currentTime that if currentTime <
lastBackflushTime (or lastDescalingTime) return -1 (or otherwise treat as "never
performed"/invalid) before computing the difference and dividing by seconds per
day/week so the functions never wrap to a huge value.
| if (!cleaningSettings.lastBackflushTime) return true; | ||
| const daysSince = Math.floor((Date.now() / 1000 - cleaningSettings.lastBackflushTime) / (24 * 60 * 60)); | ||
| return daysSince > cleaningSettings.backflushIntervalDays; | ||
| }; | ||
|
|
||
| const isDescalingDue = () => { | ||
| if (!cleaningSettings.lastDescalingTime) return true; | ||
| const weeksSince = Math.floor((Date.now() / 1000 - cleaningSettings.lastDescalingTime) / (7 * 24 * 60 * 60)); | ||
| return weeksSince > cleaningSettings.descalingIntervalWeeks; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Match cleaning-due logic to CleaningSchedulePlugin.
Lines [223-232] and [522-564] mark backflush/descaling as “due” even when the machine has never been cleaned (last*Time === 0) and require > instead of >=, so the UI lags a full interval behind the backend (which returns false for “never” and true as soon as the threshold is met). This inconsistency confuses users and makes the two UIs disagree. Align the checks with the plugin.
Apply this diff (and mirror it for descaling):
- const isBackflushDue = () => {
- if (!cleaningSettings.lastBackflushTime) return true;
- const daysSince = Math.floor((Date.now() / 1000 - cleaningSettings.lastBackflushTime) / (24 * 60 * 60));
- return daysSince > cleaningSettings.backflushIntervalDays;
- };
+ const isBackflushDue = () => {
+ if (!cleaningSettings.lastBackflushTime) {
+ return false;
+ }
+ const daysSince = Math.floor((Date.now() / 1000 - cleaningSettings.lastBackflushTime) / (24 * 60 * 60));
+ return daysSince >= cleaningSettings.backflushIntervalDays;
+ };Also applies to: 522-565
| if (confirm('Mark backflush as completed? This will reset the backflush timer.')) { | ||
| const newTime = Math.floor(Date.now() / 1000); | ||
| const formDataToSubmit = new FormData(); | ||
| formDataToSubmit.set('lastBackflushTime', newTime.toString()); | ||
|
|
||
| const response = await fetch('/api/settings', { | ||
| method: 'post', | ||
| body: formDataToSubmit, | ||
| }); | ||
| const data = await response.json(); | ||
| setFormData(prev => ({...prev, lastBackflushTime: data.lastBackflushTime})); | ||
| } | ||
| }} | ||
| title='Mark as Complete' | ||
| > | ||
| <FontAwesomeIcon icon={faCheck} /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <div className='flex items-center justify-between rounded-lg bg-base-200 p-3'> | ||
| <div> | ||
| <div className='text-sm font-medium'>Last Descaling</div> | ||
| <div className={`text-xs ${ | ||
| formData.lastDescalingTime === 0 || | ||
| Math.floor((Date.now() / 1000 - formData.lastDescalingTime) / (7 * 24 * 60 * 60)) > (formData.descalingIntervalWeeks || 6) | ||
| ? 'text-orange-600 font-medium' | ||
| : 'text-base-content/70' | ||
| }`}> | ||
| {formData.lastDescalingTime > 0 | ||
| ? (() => { | ||
| const weeks = Math.floor((Date.now() / 1000 - formData.lastDescalingTime) / (7 * 24 * 60 * 60)); | ||
| return weeks === 0 ? 'Today' : `${weeks} weeks ago`; | ||
| })() | ||
| : 'Never performed' | ||
| } | ||
| </div> | ||
| </div> | ||
| <div className='flex gap-2'> | ||
| <button | ||
| type='button' | ||
| className='btn btn-warning btn-sm' | ||
| onClick={() => { | ||
| apiService.send({ tp: 'req:cleaning:descaling:start' }); | ||
| }} | ||
| title='Start Descaling Now' | ||
| > | ||
| <FontAwesomeIcon icon={faLemon} /> | ||
| </button> | ||
| <button | ||
| type='button' | ||
| className='btn btn-success btn-sm' | ||
| onClick={async () => { | ||
| if (confirm('Mark descaling as completed? This will reset the descaling timer.')) { | ||
| const newTime = Math.floor(Date.now() / 1000); | ||
| const formDataToSubmit = new FormData(); | ||
| formDataToSubmit.set('lastDescalingTime', newTime.toString()); | ||
|
|
||
| const response = await fetch('/api/settings', { | ||
| method: 'post', | ||
| body: formDataToSubmit, | ||
| }); | ||
| const data = await response.json(); | ||
| setFormData(prev => ({...prev, lastDescalingTime: data.lastDescalingTime})); | ||
| } | ||
| }} | ||
| title='Mark as Complete' | ||
| > | ||
| <FontAwesomeIcon icon={faCheck} /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Avoid clobbering unrelated settings when marking cleaning complete.
Lines [751-819] post only lastBackflushTime/lastDescalingTime by constructing a fresh FormData() that omits every other field. The backend interprets missing args as unchecked checkboxes, so this request forces HomeKit, boiler fill, auto-wakeup, etc. to false. Users who click “Mark as Complete” will silently lose all boolean settings—a critical regression. Reuse the existing form payload (or hit a dedicated endpoint) so the POST preserves untouched fields.
Apply this diff (and mirror it in the descaling handler):
-const formDataToSubmit = new FormData();
-formDataToSubmit.set('lastBackflushTime', newTime.toString());
+const formDataToSubmit = new FormData(formRef.current);
+formDataToSubmit.set('lastBackflushTime', newTime.toString());Committable suggestion skipped: line range outside the PR's diff.
1 participant
Adds descale and backflush cleaning intervals.
Creates utility profiles if they don't exist
Shows notification on the standby page of WebUI (provisions for touch UI code)
Running the profile resets the appropriate interval or it can be manually reset in the settings page
Summary by CodeRabbit