Show/Hide Grind TouchUI and WebUI elements based on SmartGrind Toggle #415
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds end-to-end Grind mode: firmware Settings/constants and Controller APIs, GrindProcess support and alt-relay gating, device UI grind-button visibility, WebUI status fields and websocket commands, and web app Grind tab/controls plus Alt Relay setting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser UI
participant Api as ApiService
participant WS as WebSocket
participant Controller as Controller
participant Settings as Settings
participant Grind as GrindProcess
note over Browser,Settings: UI reads /api/settings to decide SmartGrind
Browser->>Api: GET /api/settings
Api-->>Browser: { altRelayFunction, smartGrindEnabled }
note over Controller,WS: Status stream includes grind fields
Controller-->>WS: status { gtd, gtv, gt, gact, ... }
WS-->>Api: status message
Api-->>Browser: update machine.status
rect rgba(220,245,230,0.4)
Browser->>WS: req:grind:activate
WS->>Controller: activateGrind()
Controller->>Grind: start()
Grind-->>Controller: running / elapsed
Controller-->>WS: status { gact:true, e, tt, pt, pp }
WS-->>Api: status update
Api-->>Browser: refresh grind progress
end
alt Change grind target (volumetric)
Browser->>WS: req:change-grind-target
WS->>Settings: setVolumetricTarget(...)
else Adjust target incrementally
Browser->>WS: req:raise-grind-target / req:lower-grind-target
WS->>Controller: raiseGrindTarget() / lowerGrindTarget()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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: 2
🧹 Nitpick comments (8)
src/display/ui/default/DefaultUI.h (1)
90-90: Name/type clarity for SmartGrind flagThe flag represents an enabled/disabled setting, not activity. Consider renaming to smartGrindEnabled for clarity. Keeping int is consistent with nearby flags, so this is optional.
src/display/ui/default/DefaultUI.cpp (1)
602-609: Grind button visibility gated by SmartGrindThis effect is scoped to the Menu screen and correctly toggles LV_OBJ_FLAG_HIDDEN based on the setting. Optional: if SmartGrind is toggled off while currently in Grind mode/screen, consider auto-navigating back to Brew or Standby to avoid stranded UI.
web/src/pages/Home/ProcessControls.jsx (4)
29-76: Avoid division by zero in progress calcGuard when pt is 0 or undefined to prevent NaN/Infinity widths.
- const progress = (processInfo.pp / processInfo.pt) * 100.0; + const progress = processInfo.pt > 0 ? (processInfo.pp / processInfo.pt) * 100.0 : 0;
134-135: Grind mode detectionmode === 4 is clear; consider a shared constant to avoid “magic number” across files.
154-156: Simplify shouldExpand expression (readability)The brew branch always evaluates to brew. Consider simplifying for clarity.
-const shouldExpand = (brew && (active || finished || (brew && !active && !finished))) || - (grind && showGrindTab && (active || finished || (grind && !active && !finished))); +const shouldExpand = brew || (grind && showGrindTab && (active || finished || (!active && !finished)));
456-486: Grind target nudge controlsOnly shown when SmartGrind is enabled; good guard. Consider also guarding by volumetricAvailable when grindTarget === 1 (weight) to avoid showing grams on machines without a scale.
src/display/plugins/WebUIPlugin.cpp (2)
113-127: Grind status block fields and units verified; optional underflow guard
- Confirmed
GrindProcessdefinestarget,time,grindVolume,currentVolume,started,finishedwith appropriate types (ProcessTarget, int, double, unsigned long) and usesmillis()for timestamps. Elapsed math and pt/pp semantics matchBrewProcess.- No further changes required.
Optional: prevent underflow in elapsed time calculation:
- pObj["e"] = ts - grind->started; + pObj["e"] = ts > grind->started ? ts - grind->started : 0;
258-275: Guard grind commands and clamp volumetric target
- Wrap grind WS commands (
req:grind:activate,req:grind:deactivate,req:raise-grind-target,req:lower-grind-target) in aisSmartGrindActive()check and clamp thetargetinreq:change-grind-targetto a boolean:} else if (msgType == "req:grind:activate") { - controller->activateGrind(); + if (controller->getSettings().isSmartGrindActive()) { + controller->activateGrind(); + } } else if (msgType == "req:grind:deactivate") { - controller->deactivateGrind(); + if (controller->getSettings().isSmartGrindActive()) { + controller->deactivateGrind(); + } } else if (msgType == "req:change-grind-target") { - if (doc["target"].is<uint8_t>()) { - auto target = doc["target"].as<uint8_t>(); - controller->getSettings().setVolumetricTarget(target); - } + if (controller->getSettings().isSmartGrindActive() && doc["target"].is<uint8_t>()) { + uint8_t target = doc["target"].as<uint8_t>(); + controller->getSettings().setVolumetricTarget(target ? 1 : 0); + } } else if (msgType == "req:raise-grind-target") { - controller->raiseGrindTarget(); + if (controller->getSettings().isSmartGrindActive()) { + controller->raiseGrindTarget(); + } } else if (msgType == "req:lower-grind-target") { - controller->lowerGrindTarget(); + if (controller->getSettings().isSmartGrindActive()) { + controller->lowerGrindTarget(); + }
- Both grind and brew handlers invoke
Settings::setVolumetricTarget(bool)(Settings.h:106), sharing a single flag—verify if separate volumetric-target flags for brew vs. grind are required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/display/plugins/WebUIPlugin.cpp(4 hunks)src/display/ui/default/DefaultUI.cpp(3 hunks)src/display/ui/default/DefaultUI.h(1 hunks)web/src/pages/Home/ProcessControls.jsx(10 hunks)web/src/services/ApiService.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/display/ui/default/DefaultUI.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
settings(139-145)
web/src/pages/Home/ProcessControls.jsx (4)
src/display/core/process/GrindProcess.h (2)
progress(39-52)finished(17-17)web/src/pages/Home/index.jsx (1)
mode(50-50)web/src/services/ApiService.js (2)
ApiServiceContext(198-198)ApiServiceContext(198-198)src/display/core/Controller.cpp (10)
activate(509-535)activate(509-509)deactivate(537-551)deactivate(537-537)clear(553-560)clear(553-553)raiseTemp(402-406)raiseTemp(402-402)lowerTemp(408-412)lowerTemp(408-408)
src/display/plugins/WebUIPlugin.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
grind(134-134)
⏰ 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 (18)
src/display/ui/default/DefaultUI.cpp (2)
220-221: Propagate SmartGrind setting into UI stateAssigning from settings.isSmartGrindActive() here is correct and ensures the reactive effect can toggle visibility promptly.
311-313: Initialize SmartGrind state on startupMirrors loop() assignment; good to avoid a one-frame mismatch at boot.
web/src/services/ApiService.js (3)
166-173: Status parsing extended for grind fieldsReading btd/btv/gtd/gtv/gt/gact with sane defaults is fine and backward compatible.
Also applies to: 169-173
200-213: Initialize machine.status with new grind fieldsDefaults align with parsing above; avoids undefined checks downstream.
156-176: Verified emission of grind and brew target fieldsWebUIPlugin.cpp sets
doc["bt"],doc["btd"],doc["btv"],doc["gtd"],doc["gtv"],doc["gt"], anddoc["gact"], and includes handlers forreq:grind:activate,req:grind:deactivate,req:change-grind-target,req:raise-grind-target, andreq:lower-grind-target, preventing NaNs in the UI. No further changes required.web/src/pages/Home/ProcessControls.jsx (10)
4-4: Using useQuery to fetch settingsFetching settings with caching is fine; avoids tight polling.
16-17: Icon importsPlus/Minus icons are used below; good.
149-152: Grind tab visibility logicKeeping the tab visible when already in mode 4 avoids UI confusion. LGTM.
309-339: Progress panesConditional rendering for GrindProgress mirrors BrewProgress; looks consistent.
488-507: Start/Stop button visibilityHidden for Grind when SmartGrind is disabled; correct.
514-515: PropTypes updated to include Grind modeExtending to [0,1,2,3,4] is correct.
138-146: SmartGrind settings key matches API
Verified that the settings endpoint returnssmartGrindActiveas a boolean (WebUIPlugin.cpp doc["smartGrindActive"] at line 510), matching its use in ProcessControls.jsx. No changes required.
157-166: Confirm backend handlers for change-target messages: Unable to locate any event listeners forreq:change-grind-targetorreq:change-brew-targetin the backend—please verify they exist or add them.
200-213: Raise/Lower target routing
No matching backend handlers forreq:raise-brew-target,req:raise-grind-target,req:lower-brew-targetorreq:lower-grind-targetwere found; verify that both brew and grind cases are implemented.
175-181: Backend handler for grind deactivation verified
req:grind:deactivateis already handled in WebUIPlugin.cpp (callscontroller->deactivateGrind()), so no further changes needed.src/display/plugins/WebUIPlugin.cpp (3)
7-7: Include looks correct.Header aligns with MODE_GRIND usage below.
81-82: Verify brew target units align with frontend
src/display/plugins/WebUIPlugin.cpp:81-82addsbtdandbtv, which the frontend ingests asbrewTargetDurationandbrewTargetVolumeinweb/src/services/ApiService.js. Ensurecontroller->getTargetDuration()returns the same time unit (ms vs s) that the UI expects.
84-87: Expose SmartGrind flag in periodic status
SmartGrind active is now emitted viasgact; confirmedisVolumetricTargetis a single shared setting for both brew and grind—no per-mode volumetric flags exist.
| const activate = useCallback(() => { | ||
| const messageType = grind ? 'req:grind:activate' : 'req:process:activate'; | ||
| apiService.send({ | ||
| tp: 'req:process:activate', | ||
| tp: messageType, | ||
| }); | ||
| }, [apiService]); | ||
| }, [apiService, grind]); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Activate command for grind
Ensure req:grind:activate is implemented server-side; otherwise fallback to a generic activate path.
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'on\(\"req:grind:activate\"' --type=cppLength of output: 0
🏁 Script executed:
#!/bin/bash
echo "--- Checking req:grind:activate in C++ ---"
rg -nP 'on\("req:grind:activate"' --type=cpp || echo "No C++ handler found for grind activate"
echo "--- Checking req:grind:activate in JS/TS ---"
rg -n "req:grind:activate" --type js --type ts || echo "No JS/TS handler found for grind activate"
echo "--- Checking req:process:activate in C++ ---"
rg -nP 'on\("req:process:activate"' --type=cpp || echo "No C++ handler found for process activate"
echo "--- Checking req:process:activate in JS/TS ---"
rg -n "req:process:activate" --type js --type ts || echo "No JS/TS handler found for process activate"Length of output: 505
Require server‐side activation handler or fallback
No C++ handlers were found for either req:grind:activate or req:process:activate; add the corresponding server‐side implementation or update the client to fall back to a supported activate request.
🤖 Prompt for AI Agents
In web/src/pages/Home/ProcessControls.jsx around lines 168-174, the client sends
either 'req:grind:activate' or 'req:process:activate' but there are no
server-side C++ handlers for those message types; either add matching server
handlers or change the client to fall back to a supported activation message.
Fix by (a) adding server-side C++ handlers named 'req:grind:activate' and
'req:process:activate' that validate the payload and perform the activation
logic and return success/error responses, or (b) update this client code to
detect available server message types and send an existing supported
activate/start message (e.g., 'req:process:start' or the documented activation
type), include robust error handling for unsupported responses, and log/fallback
if the server responds with unknown-message-type.
|
Added Alt Relay Settings (None/Grind/Steam (disabled)) with Grind being default for legacy. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/src/pages/Home/ProcessControls.jsx (2)
172-178: Activation message may lack server handler; add fallback or use request/ackPrior review flagged no C++ handler for
req:grind:activate/req:process:activate. Either add handlers or fall back to a supported message and handle errors.#!/bin/bash # Verify handlers exist for new message types set -euo pipefail echo "Searching server-side handlers..." rg -nP -C2 'on\("req:(grind:activate|process:activate|grind:deactivate|process:deactivate|change-grind-target|raise-grind-target|lower-grind-target|raise-temp|lower-temp)"' --type=cpp || true
374-399: Regression: Brew time/weight toggle hidden when Grind is unavailableThe outer condition requires
isGrindAvailableeven for Brew, hiding the Brew toggle when SmartGrind is off or Alt Relay ≠ Grind. GateisGrindAvailableonly for Grind.- {(brew || (grind && showGrindTab)) && !active && !finished && status.value.volumetricAvailable && isGrindAvailable && ( + {status.value.volumetricAvailable && !active && !finished && ( + (brew) || + (grind && showGrindTab && isGrindAvailable) + ) && (
🧹 Nitpick comments (9)
src/display/core/constants.h (1)
35-39: ALT_RELAY_ constants added — consider stronger typing and docstring*Looks good. To prevent accidental misuses and keep frontend/backend values in sync, consider switching these to a small enum/constexpr with a short comment mapping to WebUI values (0=None, 1=Grind, 2=Steam). This avoids magic numbers elsewhere and clarifies intent.
-#define ALT_RELAY_NONE 0 -#define ALT_RELAY_GRIND 1 -#define ALT_RELAY_STEAM_BOILER 2 +enum AltRelayFunction : int { + ALT_RELAY_NONE = 0, + ALT_RELAY_GRIND = 1, + ALT_RELAY_STEAM_BOILER = 2 +};src/display/core/Controller.cpp (1)
483-493: Alt-relay gating is correct; align intent with WebUI visibility and avoid unnecessary BLE chatter
- Logic correctly restricts SSR2 to Grind when ALT_RELAY_GRIND is selected.
- If product intent is “hide WebUI grind when SmartGrind is disabled,” verify whether alt-relay alone should still allow grind UI; current WebUI shows grind when either SmartGrind OR alt-relay=Grind (see ProcessControls.jsx). Align both sides.
- Minor: only send sendAltControl on state change to reduce traffic.
- clientController.sendAltControl(altRelayActive); + static bool lastAltRelayActive = false; + if (altRelayActive != lastAltRelayActive) { + clientController.sendAltControl(altRelayActive); + lastAltRelayActive = altRelayActive; + }src/display/core/Settings.h (2)
83-83: Add brief comment mapping to constantsReturning an int is fine, but a note like “0=None, 1=Grind, 2=Steam” reduces guesswork for callers.
219-219: Avoid duplicated defaults across .h/.cppDefault here and in the preferences loader both set to GRIND. Consider a single DEFAULT_ALT_RELAY_FUNCTION constant to keep them in sync.
- int altRelayFunction = ALT_RELAY_GRIND; // Default to grind + static constexpr int DEFAULT_ALT_RELAY_FUNCTION = ALT_RELAY_GRIND; + int altRelayFunction = DEFAULT_ALT_RELAY_FUNCTION; // Default to grindweb/src/pages/Settings/index.jsx (2)
112-113: CoercealtRelayFunctionto a number before postingSelect values are strings; send a numeric to match backend’s
getInt.- formDataToSubmit.set('altRelayFunction', formData.altRelayFunction !== undefined ? formData.altRelayFunction : 1); + formDataToSubmit.set( + 'altRelayFunction', + String(formData.altRelayFunction !== undefined ? Number(formData.altRelayFunction) : 1) + );
585-602: Alt Relay select: normalize value to number and keep UI/server in syncEnsure controlled value is numeric to avoid equality mismatches.
- <select + <select id='altRelayFunction' name='altRelayFunction' className='select select-bordered w-full' - value={formData.altRelayFunction !== undefined ? formData.altRelayFunction : 1} + value={formData.altRelayFunction !== undefined ? Number(formData.altRelayFunction) : 1} onChange={onChange('altRelayFunction')} >Additionally, in onChange add:
if (key === 'altRelayFunction') { value = Number(e.currentTarget.value); }web/src/pages/Home/ProcessControls.jsx (3)
32-33: Guard division by zero when computing progressIf
ptis 0 or missing, UI shows NaN and progress bar breaks.- const progress = (processInfo.pp / processInfo.pt) * 100.0; + const progress = processInfo.pt > 0 ? Math.min(100, (processInfo.pp / processInfo.pt) * 100.0) : 0;
148-152: Type safety: coercealtRelayFunctionto numberPrevents subtle bugs if backend ever returns it as a string.
- const altRelayFunction = settings?.altRelayFunction !== undefined ? settings.altRelayFunction : 1; + const altRelayFunction = Number(settings?.altRelayFunction ?? 1);
158-160: SimplifyshouldExpand(current logic is tautological)
(active || finished || (!active && !finished))is always true; net result: expand for Brew or Grind. Replace with an explicit, readable condition.- const shouldExpand = (brew && (active || finished || (brew && !active && !finished))) || - (grind && showGrindTab && (active || finished || (grind && !active && !finished))); + const shouldExpand = brew || (grind && showGrindTab);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/display/core/Controller.cpp(1 hunks)src/display/core/Settings.cpp(3 hunks)src/display/core/Settings.h(3 hunks)src/display/core/constants.h(1 hunks)src/display/plugins/WebUIPlugin.cpp(6 hunks)src/display/ui/default/DefaultUI.cpp(3 hunks)src/display/ui/default/DefaultUI.h(1 hunks)web/src/pages/Home/ProcessControls.jsx(10 hunks)web/src/pages/Settings/index.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/display/ui/default/DefaultUI.h
- src/display/ui/default/DefaultUI.cpp
- src/display/plugins/WebUIPlugin.cpp
🧰 Additional context used
🧬 Code graph analysis (4)
web/src/pages/Settings/index.jsx (1)
web/src/pages/OTA/index.jsx (1)
formData(10-10)
src/display/core/Settings.h (2)
web/src/pages/Home/ProcessControls.jsx (1)
altRelayFunction(148-148)src/display/core/Settings.cpp (2)
setAltRelayFunction(411-414)setAltRelayFunction(411-411)
src/display/core/Settings.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
altRelayFunction(148-148)
web/src/pages/Home/ProcessControls.jsx (5)
src/display/core/process/BrewProcess.h (1)
progress(130-149)src/display/core/process/GrindProcess.h (2)
progress(39-52)finished(17-17)web/src/pages/Home/index.jsx (1)
mode(50-50)web/src/services/ApiService.js (2)
ApiServiceContext(198-198)ApiServiceContext(198-198)src/display/core/Controller.cpp (10)
activate(518-544)activate(518-518)deactivate(546-560)deactivate(546-546)clear(562-569)clear(562-562)raiseTemp(402-406)raiseTemp(402-402)lowerTemp(408-412)lowerTemp(408-408)
🔇 Additional comments (4)
src/display/core/Settings.cpp (2)
73-73: Defaulting alt_relay to GRIND preserves legacy behavior — LGTM
488-488: Persistence key added — LGTMKey name “alt_relay” matches loader and is consistent with other ints.
web/src/pages/Home/ProcessControls.jsx (2)
150-156: Confirm intended UX: Grind shown when SmartGrind is off but Alt Relay=GrindCode shows Grind when either SmartGrind is enabled OR Alt Relay=Grind. PR text says “Disable all WebUI grind elements when SmartGrind plugin is disabled.” Please confirm which source of truth should gate WebUI. Align with TouchUI and Controller.
492-510: Common controls: allow Grind button visibility even when SmartGrind is off if that’s intendedCondition looks fine if Grind requires availability; just ensure consistency with the agreed UX (see earlier verification comment).
| void Settings::setAltRelayFunction(int alt_relay_function) { | ||
| altRelayFunction = alt_relay_function; | ||
| save(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input for setAltRelayFunction to allowed values (0,1,2)
Currently any int persists; out-of-range values silently disable features. Clamp or reject invalid values.
-void Settings::setAltRelayFunction(int alt_relay_function) {
- altRelayFunction = alt_relay_function;
+void Settings::setAltRelayFunction(int alt_relay_function) {
+ if (alt_relay_function < ALT_RELAY_NONE || alt_relay_function > ALT_RELAY_STEAM_BOILER) {
+ ESP_LOGW("Settings", "Invalid altRelayFunction=%d. Keeping previous=%d", alt_relay_function, altRelayFunction);
+ return;
+ }
+ altRelayFunction = alt_relay_function;
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::setAltRelayFunction(int alt_relay_function) { | |
| altRelayFunction = alt_relay_function; | |
| save(); | |
| } | |
| void Settings::setAltRelayFunction(int alt_relay_function) { | |
| if (alt_relay_function < ALT_RELAY_NONE || alt_relay_function > ALT_RELAY_STEAM_BOILER) { | |
| ESP_LOGW("Settings", "Invalid altRelayFunction=%d. Keeping previous=%d", alt_relay_function, altRelayFunction); | |
| return; | |
| } | |
| altRelayFunction = alt_relay_function; | |
| save(); | |
| } |
🤖 Prompt for AI Agents
In src/display/core/Settings.cpp around lines 411-414, the setter currently
writes any int to altRelayFunction and persists it; modify it to validate the
input so only 0, 1, or 2 are accepted: if the incoming alt_relay_function is
outside that range either clamp it to the nearest allowed value (0 if <0, 2 if
>2) or reject it by logging/erroring and returning without calling save(); after
validation assign the sanitized value to altRelayFunction and then call save();
ensure any error path does not persist an invalid value.
| void setSunriseExtBrightness(int sunrise_ext_brightness); | ||
| void setEmptyTankDistance(int empty_tank_distance); | ||
| void setFullTankDistance(int full_tank_distance); | ||
| void setAltRelayFunction(int alt_relay_function); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document and enforce valid range for setter
Pair this with the clamping/validation change in Settings.cpp.
🤖 Prompt for AI Agents
In src/display/core/Settings.h around line 146, the setter declaration for
setAltRelayFunction lacks documentation of the valid value range and the
contract for out-of-range inputs; add a concise comment/doxygen above the
declaration stating the allowed integer range (e.g. 0–N) and the chosen behavior
for invalid values (e.g. value will be clamped to that range or an
assert/exception will be raised), and ensure the Settings.cpp change
clamps/validates to that same range so the header and implementation match.
dragm83
force-pushed
the
feature/grind-toggle
branch
from
2332517 to
64bc8f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/display/core/Settings.cpp (1)
411-414: ValidatealtRelayFunctioninput (repeat).Same concern as earlier review: reject out‑of‑range values instead of persisting them.
-void Settings::setAltRelayFunction(int alt_relay_function) { - altRelayFunction = alt_relay_function; - save(); -} +void Settings::setAltRelayFunction(int alt_relay_function) { + if (alt_relay_function < ALT_RELAY_NONE || alt_relay_function > ALT_RELAY_STEAM_BOILER) { + ESP_LOGW("Settings", "Invalid altRelayFunction=%d. Keeping previous=%d", alt_relay_function, altRelayFunction); + return; + } + if (altRelayFunction == alt_relay_function) return; // no-op to reduce flash writes + altRelayFunction = alt_relay_function; + save(); +}web/src/pages/Home/ProcessControls.jsx (2)
172-177: Server handlers now exist for Grind activate/deactivateClient requests
req:grind:activate|deactivateare wired server-side; good to go.
374-399: Fix regression: Brew time/weight toggle incorrectly gated by isGrindAvailableBrew UI shouldn’t depend on SmartGrind/Alt Relay. Current condition hides Brew toggle when SmartGrind is disabled.
Apply:
- {(brew || (grind && showGrindTab)) && !active && !finished && status.value.volumetricAvailable && isGrindAvailable && ( + {( + (brew && !active && !finished && status.value.volumetricAvailable) || + (grind && showGrindTab && !active && !finished && status.value.volumetricAvailable && isGrindAvailable) + ) && (
🧹 Nitpick comments (4)
src/display/core/Controller.cpp (1)
483-492: GuardcurrentProcessaccess; make logic future-proof for other Alt Relay modes.Avoid races/null deref by snapshotting
currentProcess. Also switch onaltRelayFunctionso adding Steam later is trivial.- // Check if alt relay should be active based on process type and alt relay function setting - bool altRelayActive = false; - if (isActive() && currentProcess->isAltRelayActive()) { - if (currentProcess->getType() == MODE_GRIND && settings.getAltRelayFunction() == ALT_RELAY_GRIND) { - altRelayActive = true; - } - } + // Alt relay gating + Process* p = currentProcess; // snapshot to avoid TOCTOU with other tasks + bool altRelayActive = false; + if (p && p->isActive() && p->isAltRelayActive()) { + switch (settings.getAltRelayFunction()) { + case ALT_RELAY_GRIND: + altRelayActive = (p->getType() == MODE_GRIND); + break; + case ALT_RELAY_STEAM_BOILER: + // Reserved for future use; no-op today if Steam option is disabled + altRelayActive = (p->getType() == MODE_STEAM); + break; + default: // ALT_RELAY_NONE or invalid + break; + } + }Please confirm
sendAltControl(false)reliably drives the alt relay off on all hardware revisions; if any require level inversion, we should gate that via capabilities/settings.web/src/pages/Home/ProcessControls.jsx (3)
157-160: Simplify always-true expansion logicThe current expression reduces to
brew || (grind && showGrindTab).- const shouldExpand = (brew && (active || finished || (brew && !active && !finished))) || - (grind && showGrindTab && (active || finished || (grind && !active && !finished))); + const shouldExpand = brew || (grind && showGrindTab);
29-36: Guard divide-by-zero in Grind progressIf
processInfo.ptis 0, the width becomesInfinity%.- const progress = (processInfo.pp / processInfo.pt) * 100.0; + const progress = (active && processInfo.pt > 0) + ? (processInfo.pp / processInfo.pt) * 100.0 + : 0;
147-152: Avoid magic number for Alt Relay GrindUse a named constant to keep UI aligned with firmware constants.
+const ALT_RELAY_GRIND = 1; ... -const altRelayFunction = settings?.altRelayFunction !== undefined ? settings.altRelayFunction : 1; +const altRelayFunction = settings?.altRelayFunction !== undefined ? settings.altRelayFunction : ALT_RELAY_GRIND; ... -const isGrindAvailable = isSmartGrindEnabled || altRelayFunction === 1; // ALT_RELAY_GRIND = 1 +const isGrindAvailable = isSmartGrindEnabled || altRelayFunction === ALT_RELAY_GRIND;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/display/core/Controller.cpp(1 hunks)src/display/core/Settings.cpp(3 hunks)src/display/core/Settings.h(3 hunks)src/display/core/constants.h(1 hunks)src/display/plugins/WebUIPlugin.cpp(6 hunks)src/display/ui/default/DefaultUI.cpp(3 hunks)src/display/ui/default/DefaultUI.h(1 hunks)web/src/pages/Home/ProcessControls.jsx(10 hunks)web/src/pages/Settings/index.jsx(2 hunks)web/src/services/ApiService.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/display/core/constants.h
- src/display/ui/default/DefaultUI.h
- src/display/core/Settings.h
- src/display/ui/default/DefaultUI.cpp
- web/src/pages/Settings/index.jsx
- web/src/services/ApiService.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/display/core/Settings.cpp (1)
web/src/pages/Home/ProcessControls.jsx (1)
altRelayFunction(148-148)
src/display/plugins/WebUIPlugin.cpp (1)
web/src/pages/Home/ProcessControls.jsx (2)
grind(134-134)settings(139-145)
web/src/pages/Home/ProcessControls.jsx (4)
src/display/core/process/BrewProcess.h (1)
progress(130-149)src/display/core/process/GrindProcess.h (2)
progress(39-52)finished(17-17)web/src/pages/Home/index.jsx (1)
mode(50-50)src/display/core/Controller.cpp (10)
activate(518-544)activate(518-518)deactivate(546-560)deactivate(546-546)clear(562-569)clear(562-562)raiseTemp(402-406)raiseTemp(402-402)lowerTemp(408-412)lowerTemp(408-408)
⏰ 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: deploy
- GitHub Check: test
🔇 Additional comments (6)
src/display/core/Settings.cpp (2)
73-73: Default = GRIND aligns with legacy behavior.Good: missing preference falls back to
ALT_RELAY_GRINDas per PR goals.
488-488: Persistence key matches load path.
preferences.putInt("alt_relay", altRelayFunction);correctly mirrors the constructor load. LGTM.src/display/plugins/WebUIPlugin.cpp (4)
121-135: Grind status payload looks correctFields
s/l/e/tt/pt/ppalign with UI expectations for Grind progress.
267-285: Grind message handlers added — resolves client wiring
req:grind:activate|deactivate, target change, and target adjust are present.
490-491: Alt Relay Settings surfaced via GET/POST — matches UI usage
altRelayFunctionis now round-tripped; aligns with WebUI gating.Also applies to: 544-544
270-271: No change required — deactivateGrind already clears stateVerified: Controller::deactivateGrind() calls deactivate() then clear() (src/display/core/Controller.cpp:584–587).
Likely an incorrect or invalid review comment.
Disables Alt Relay when set to None
dragm83
force-pushed
the
feature/grind-toggle
branch
from
64bc8f4 to
0217ffa
Compare
|
|
2 participants
This commit is on top of the WebUI Grind PR #412
Can merge only DefaultUI.cpp/h for TouchUI changes only.
Disables all WebUI grind elements if the SmartGrind plugin is disabled.
Disables the Touch UI grind menu button if plugin is disabled.
Added Alt Relay Settings (None/Grind/Steam (disabled)) with Grind being default for legacy.
Summary by CodeRabbit