Sagemate purge valve #486
Sagemate purge valve #486
Conversation
WalkthroughExtended PID control to include a fourth feed‑forward parameter (FF) across BLE, controller, and heater layers; added a PurgeValvePlugin that triggers a short purge after brew events; and commented out most BLE scale plugin includes/apply calls leaving only BookooScalesPlugin active. Changes
Sequence Diagram(s)sequenceDiagram
participant BLE as BLE Layer
participant Controller
participant Heater
participant PID as PID Module
rect rgb(220,240,250)
Note over BLE,Controller: Prior flow (3 params)
BLE->>Controller: autotuneResult(Kp,Ki,Kd)
Controller->>Heater: setTunings(Kp,Ki,Kd)
Heater->>PID: updateGains(Kp,Ki,Kd)
end
rect rgb(240,250,220)
Note over BLE,Controller: New flow (4 params with FF)
BLE->>Controller: autotuneResult(Kp,Ki,Kd,FF)
Controller->>Heater: setTunings(Kp,Ki,Kd,FF)
Heater->>PID: updateGains(Kp,Ki,Kd,FF)
end
sequenceDiagram
participant EventMgr as Event Manager
participant Purge as PurgeValvePlugin
participant Grinder as GrindProcess
EventMgr->>Purge: controller:brew:start
Purge->>Purge: _brew_started = true
EventMgr->>Purge: controller:brew:end
Purge->>Purge: _brew_finished = true
loop controller.loop()
alt purge condition met and not already purging
Purge->>Grinder: Start purge (1500 ms)
Purge->>Purge: _is_purging = true
else purge elapsed
Purge->>Purge: reset _brew_started/_brew_finished/_is_purging
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (4)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/GaggiMateController/src/peripherals/Heater.cpp (1)
107-113: Missing FF parameter in error callback.Line 111 calls
pid_callbackwith only 3 parameters (0, 0, 0), but based on the PR changes, the callback signature now requires 4 parameters including FF.Apply this diff:
if (temperature > MAX_AUTOTUNE_TEMP) { output = 0.0f; autotuning = false; softPwm(TUNER_OUTPUT_SPAN); - pid_callback(0, 0, 0); + pid_callback(0, 0, 0, 0); return; }
🧹 Nitpick comments (3)
lib/NimBLEComm/src/NimBLEServerController.cpp (1)
122-129: Consider including FF parameter for protocol consistency.While the current implementation is backward-compatible (the client defaults FF to 0), the autotune result sends only three parameters (Kp, Ki, Kd), creating an asymmetry with the extended four-parameter PID control protocol. For consistency and clarity, consider either:
- Updating the signature to
sendAutotuneResult(float Kp, float Ki, float Kd, float FF = 0.0f)and explicitly sending four values- Documenting that autotune does not tune the feed-forward term
src/display/plugins/PurgeValvePlugin.cpp (2)
10-21: Add null pointer validation for controller parameter.The setup method should validate that the controller pointer is not null before storing it to prevent potential null pointer dereferences in the loop method.
Apply this diff:
void PurgeValvePlugin::setup(Controller *controller, PluginManager *pluginManager) { + if (controller == nullptr || pluginManager == nullptr) { + ESP_LOGE(LOG_TAG, "Invalid controller or pluginManager in PurgeValvePlugin::setup"); + return; + } _controller = controller;
23-28: Address commented code and consider semantic clarity.
Commented code (Line 25): The commented
sendAltControl(true)call should either be removed if no longer needed or uncommented if it's required for the purge valve operation. Leaving it commented creates ambiguity about the intended behavior.Semantic mismatch (Line 26): Using
GrindProcessfor a purge valve operation is semantically confusing, as "Grind" implies coffee grinding rather than valve purging. Consider either:
- Creating a dedicated
PurgeProcessclass for clarity- Adding a comment explaining why GrindProcess is reused
- Using the alt relay control (as suggested by the commented line) instead of process-based control
If the alt relay approach is correct, consider:
if (!_is_purging && _brew_started && _brew_finished) { - // _controller->getClientController()->sendAltControl(true); - this->_controller->startProcess(new GrindProcess(ProcessTarget::TIME, PURGE_TIME_MS, 0.0, 0.0)); + _controller->getClientController().sendAltControl(true); _is_purging = true; } - if (_is_purging && millis() > _last_brew_finished + PURGE_TIME_MS) { + if (_is_purging && millis() > _last_brew_finished + PURGE_TIME_MS) { + _controller->getClientController().sendAltControl(false); _is_purging = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/GaggiMateController/src/GaggiMateController.cpp(1 hunks)lib/GaggiMateController/src/peripherals/Heater.cpp(3 hunks)lib/GaggiMateController/src/peripherals/Heater.h(1 hunks)lib/NimBLEComm/src/NimBLEClientController.cpp(1 hunks)lib/NimBLEComm/src/NimBLEComm.h(1 hunks)lib/NimBLEComm/src/NimBLEServerController.cpp(1 hunks)src/display/core/Controller.cpp(3 hunks)src/display/plugins/BLEScalePlugin.cpp(2 hunks)src/display/plugins/PurgeValvePlugin.cpp(1 hunks)src/display/plugins/PurgeValvePlugin.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
lib/GaggiMateController/src/peripherals/Heater.h (1)
lib/GaggiMateController/src/peripherals/Heater.cpp (2)
setTunings(68-74)setTunings(68-68)
src/display/core/Controller.cpp (1)
src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/plugins/PurgeValvePlugin.h (4)
src/display/core/Plugin.h (1)
Plugin(6-12)src/display/plugins/PurgeValvePlugin.cpp (4)
setup(10-21)setup(10-10)loop(23-35)loop(23-23)src/display/core/Controller.cpp (4)
setup(33-92)setup(33-33)loop(250-333)loop(250-250)src/display/core/Controller.h (1)
pluginManager(134-134)
src/display/plugins/PurgeValvePlugin.cpp (2)
src/display/core/Controller.cpp (4)
setup(33-92)setup(33-33)loop(250-333)loop(250-250)src/display/core/Controller.h (1)
pluginManager(134-134)
lib/NimBLEComm/src/NimBLEServerController.cpp (1)
lib/NimBLEComm/src/NimBLEComm.cpp (2)
get_token(3-23)get_token(3-3)
lib/NimBLEComm/src/NimBLEClientController.cpp (1)
lib/NimBLEComm/src/NimBLEComm.cpp (2)
get_token(3-23)get_token(3-3)
🪛 Clang (14.0.6)
src/display/plugins/PurgeValvePlugin.h
[error] 4-4: 'display/core/Plugin.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (13)
lib/GaggiMateController/src/peripherals/Heater.h (1)
26-26: LGTM! Feed-forward parameter correctly added to setTunings.The signature extension to four parameters (Kp, Ki, Kd, FF) is consistent with the implementation in Heater.cpp and the updated call sites.
lib/NimBLEComm/src/NimBLEClientController.cpp (1)
288-289: LGTM! Backward-compatible FF parameter parsing.The use of
get_tokenwith a default value of "0" ensures backward compatibility when the autotune result contains only three parameters.lib/NimBLEComm/src/NimBLEComm.h (1)
36-36: LGTM! Callback signature correctly extended.The type alias update to include the FF parameter is consistent with the extended PID control flow throughout the codebase.
lib/NimBLEComm/src/NimBLEServerController.cpp (1)
239-242: LGTM! PID control parsing correctly extended.The parsing of the FF parameter with a default value ensures backward compatibility, and the callback invocation matches the updated signature.
lib/GaggiMateController/src/GaggiMateController.cpp (1)
134-134: LGTM! PID control callback correctly extended.The lambda properly accepts four parameters and forwards them to
setTunings, maintaining consistency with the extended PID control flow.src/display/core/Controller.cpp (3)
24-24: LGTM! PurgeValvePlugin properly included.
72-72: LGTM! PurgeValvePlugin correctly registered.The registration follows the established pattern for other plugins in the system.
165-169: LGTM! Autotune callback correctly extended to include FF.The callback signature, logging, and PID string formatting all properly handle the fourth parameter. The buffer size is adequate for the formatted output.
src/display/plugins/BLEScalePlugin.cpp (1)
6-16: Change is verified as intentional code cleanup.The commit message "Remove unused scales" confirms this is deliberate. A codebase search found no references to the removed scale plugins (Acaia, Decent, Difluid, Eclair, Eureka, FelicitaScale, Timemore, Varia, WeighMyBrew, myscale) anywhere outside this file, confirming they were genuinely unused code. BookooScalesPlugin remains active.
This is legitimate cleanup of stale code, not a breaking change affecting users.
lib/GaggiMateController/src/peripherals/Heater.cpp (3)
23-23: LGTM: Feed-forward activation aligns with new FF parameter.Enabling feed-forward control is appropriate now that the FF parameter is fully supported throughout the PID control stack.
68-74: LGTM: setTunings implementation correctly handles FF parameter.The new 4-parameter signature properly checks all gains (including FF), updates the controller, resets the PID, and logs the new values.
121-121: LGTM: Correct FF parameter passed to setTunings.The
setTuningscall correctly includes the FF parameter from the autotuner.src/display/plugins/PurgeValvePlugin.h (1)
1-8: Header structure looks good.The header guard, includes, and constant definition follow standard conventions. The static analysis error about
Plugin.hnot being found is a false positive from the sandbox environment.Note: The
PURGE_TIME_MSconstant is set to 1500ms (1.5 seconds). Verify this duration is appropriate for your purge valve hardware and requirements.
|
|
Why are all the scales disabled? |
2 participants
Summary by CodeRabbit
New Features
Changes