Implement flow-based thermal feedforward for heater control #492
Conversation
- only enabled when 3wv is on - use calculated flow rate - use Kff parameter (combined thermal gain) - additional ff post-scaling to limit upside overshoot
WalkthroughThis PR introduces thermal feedforward control to the espresso machine controller. It extends the PID callback interface from three parameters (Kp, Ki, Kd) to four (Kp, Ki, Kd, Kf), implements disturbance feedforward calculations in the Heater class with safety scaling, exposes internal pump flow and valve state pointers, and integrates feedforward logic into the SimplePID controller. Changes
Sequence DiagramsequenceDiagram
participant BLE as BLE/Remote
participant GMC as GaggiMateController
participant DPump as DimmedPump
participant Heater
participant PID as SimplePID
BLE->>GMC: registerPidControlCallback(Kp, Ki, Kd, Kf)
GMC->>Heater: setThermalFeedforward(pumpFlowPtr, temp, valvePtr)
GMC->>Heater: setFeedforwardScale(Kf)
loop Runtime Control Loop
Heater->>DPump: read _currentFlow, _valveStatus
Heater->>Heater: calculateDisturbanceFeedforwardGain()
Heater->>Heater: calculateSafetyScaling(tempError)
rect rgb(200, 220, 255)
note right of Heater: Disturbance Feedforward Phase
Heater->>PID: setDisturbanceFeedforward(flow, gain)
end
rect rgb(220, 240, 220)
note right of PID: PID Update Phase
PID->>PID: compute FFOut + DistFFOut
end
Heater->>PID: loopPid()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 (5)
lib/GaggiMateController/src/GaggiMateController.cpp (1)
76-86: Document or externalize the magic number for incoming water temperature.Line 82 uses a hardcoded value of
23.0ffor incoming water temperature. This appears to be room temperature in Celsius, but it should either be:
- Documented with a comment explaining the assumption
- Defined as a named constant (e.g.,
AMBIENT_WATER_TEMP_C)- Made configurable if water temperature varies significantly by environment
Apply this diff to add documentation:
- heater->setThermalFeedforward(pumpFlowPtr, 23.0f, valveStatusPtr); + // Assume incoming water is at ambient temperature (~23°C) + heater->setThermalFeedforward(pumpFlowPtr, 23.0f, valveStatusPtr);web/src/pages/Settings/index.jsx (1)
49-60: Minor type inconsistency in default Kf value.Line 58 sets the default
kfto the string'0.000', but the input field at line 575 expects a numeric type. While JavaScript's type coercion will handle this, it would be more consistent to use a numeric default.Apply this diff for better type consistency:
} else { // No Kf in PID string, use default - settingsWithToggle.kf = '0.000'; + settingsWithToggle.kf = 0.0; }lib/GaggiMateController/src/peripherals/Heater.h (1)
28-33: Feedforward API and state look coherent; consider minor polish for clarity/extensibilityThe new thermal feedforward API and backing state are consistent and default-safe (Kff=0, null pointers). Two small follow-ups you might consider:
- The
pumpFlowRate/valveStatuspointers are treated read‑only; if the providers can support it,const float*/const bool*here would tighten const‑correctness.incomingWaterTemp,heaterEfficiency, andheatLossWattsare baked-in model constants; if you expect machines with different plumbing/insulation, exposing these via configuration (or at least documenting expected units and typical ranges) would make future tuning easier.None of this blocks the current feature, but they would make the interface more self‑documenting and adaptable.
Also applies to: 43-45, 70-82
lib/NimBLEComm/src/NimBLEServerController.cpp (1)
236-255: Kf parsing path is sound; clarify behavior for non‑positive KfThe optional Kf parsing and logging are wired correctly, and forwarding
(Kp, Ki, Kd, Kf)through the callback keeps the interface consistent.Note that the current condition
kfToken.length() > 0 && kfToken.toFloat() > 0.0fforces any zero or negative Kf to0.0f, effectively disabling feedforward. If you ever want to support negative gains (e.g., for experimentation), you’ll need to relax this check or document that Kf must be strictly positive to be applied.lib/GaggiMateController/src/peripherals/Heater.cpp (1)
68-83: Feedforward setup is safe; consider tightening logging and pointer expectations
setThermalFeedforwardandsetFeedforwardScalecorrectly initialize the new state and keep feedforward effectively disabled untilcombinedKff > 0.Two small follow-ups to consider:
- The log
"Feedforward will be %s based on Kff value"assumes that the only gating factor is Kff, butloopPidalso requires validpumpFlowRateandvalveStatus. You might want a more explicit log (or a warning) when Kff>0 but the pointers are null, to avoid confusion during tuning.- If
pumpFlowPtr/valveStatusPtrare expected to remain valid for the lifetime of theHeater, it’s worth documenting that expectation in the header; otherwise a dangling pointer could be hard to diagnose.Behavior-wise, the current implementation is fine and keeps FF off by default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/GaggiMateController/src/GaggiMateController.cpp(2 hunks)lib/GaggiMateController/src/peripherals/DimmedPump.h(1 hunks)lib/GaggiMateController/src/peripherals/Heater.cpp(3 hunks)lib/GaggiMateController/src/peripherals/Heater.h(3 hunks)lib/NayrodPID/src/SimplePID/SimplePID.cpp(5 hunks)lib/NayrodPID/src/SimplePID/SimplePID.h(2 hunks)lib/NimBLEComm/src/NimBLEClientController.cpp(1 hunks)lib/NimBLEComm/src/NimBLEComm.h(1 hunks)lib/NimBLEComm/src/NimBLEServerController.cpp(2 hunks)src/display/core/Controller.cpp(1 hunks)src/display/core/constants.h(1 hunks)web/src/pages/Settings/index.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/GaggiMateController/src/peripherals/Heater.h (1)
lib/GaggiMateController/src/peripherals/Heater.cpp (8)
setThermalFeedforward(69-78)setThermalFeedforward(69-69)setFeedforwardScale(80-83)setFeedforwardScale(80-80)calculateDisturbanceFeedforwardGain(199-218)calculateDisturbanceFeedforwardGain(199-199)calculateSafetyScaling(220-234)calculateSafetyScaling(220-220)
lib/NimBLEComm/src/NimBLEServerController.cpp (1)
lib/NimBLEComm/src/NimBLEComm.cpp (2)
get_token(3-23)get_token(3-3)
lib/NayrodPID/src/SimplePID/SimplePID.h (1)
lib/NayrodPID/src/SimplePID/SimplePID.cpp (2)
setDisturbanceFeedforward(193-197)setDisturbanceFeedforward(193-193)
lib/NimBLEComm/src/NimBLEClientController.cpp (1)
lib/NimBLEComm/src/NimBLEComm.cpp (2)
get_token(3-23)get_token(3-3)
⏰ 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 (13)
src/display/core/constants.h (1)
19-19: LGTM!The DEFAULT_PID constant has been correctly extended to include the fourth Kf parameter with a safe default of 0.0 (feedforward disabled).
lib/NimBLEComm/src/NimBLEComm.h (1)
36-36: LGTM - Breaking API change.The callback signature has been correctly extended to include the Kf parameter. This is a breaking change but is consistent with the feedforward feature implementation across the codebase.
lib/NayrodPID/src/SimplePID/SimplePID.cpp (3)
36-48: LGTM!The disturbance feedforward implementation is correctly structured:
DistFFOutis properly initialized to 0.0f- Calculation is guarded by the activation flag
- Clean integration with existing feedforward logic
66-82: LGTM!The disturbance feedforward term is correctly integrated into both the initial PID sum calculation and the anti-windup recomputation. This ensures consistent behavior when output saturation occurs.
193-197: LGTM!The
setDisturbanceFeedforwardmethod provides a convenient single-call interface to configure both the disturbance value and gain while automatically managing the activation state.lib/GaggiMateController/src/GaggiMateController.cpp (1)
124-130: LGTM!The PID control callback has been correctly extended to accept the Kf parameter and properly applies it via
setFeedforwardScale. The integration is clean and consistent with the thermal feedforward feature.lib/NimBLEComm/src/NimBLEClientController.cpp (1)
288-294: LGTM - Good backward compatibility.The implementation handles the optional Kf parameter gracefully with a sensible default of 0.0, ensuring backward compatibility with 3-parameter PID strings while supporting the new 4-parameter format.
web/src/pages/Settings/index.jsx (2)
195-199: LGTM!The PID and Kf values are correctly combined into a single comma-separated string for submission, maintaining consistency with the backend's expected format.
568-582: LGTM!The new Kf input field is properly configured with:
- Appropriate numeric type and step precision (0.001)
- Clear labeling indicating it can be disabled with 0
- Correct value binding and change handling
lib/GaggiMateController/src/peripherals/DimmedPump.h (1)
24-24: LGTM!The
getPumpFlowPtr()method safely returns a pointer to the internal_currentFlowmember for thermal feedforward integration.lib/NimBLEComm/src/NimBLEServerController.cpp (1)
122-127: Autotune result formatting with default Kf looks goodThe larger buffer and explicit
Kf=0.0in the autotune result string make the BLE protocol backward compatible while exposing the new slot for clients that understand four‑component PID. No issues here.src/display/core/Controller.cpp (1)
163-171: All PID consumers correctly handle four-field format—verification successfulThe implementation is consistent across the full chain:
- Web UI (Settings/index.jsx:49-59): Safely parses 4 fields with fallback for missing Kf
- BLE Client (NimBLEClientController.cpp:283-295): Parses all 4 fields, defaults Kf=0.0 if absent
- BLE Server (NimBLEServerController.cpp:122-127): Sends
"%.3f,%.3f,%.3f,0.0"format- Controller (GaggiMateController.cpp:124-128): Applies all 4 parameters including feedforward scale via
setFeedforwardScale(Kf)No legacy 3-field format strings remain. Backward compatibility is preserved where consumers default missing Kf to 0.0.
lib/GaggiMateController/src/peripherals/Heater.cpp (1)
90-120: No action required; the feedforward implementation is correctly separated.The concern raised in the original review is unfounded. Verification confirms that
activateFeedForward(false)and disturbance feedforward operate on completely independent code paths:
activateFeedForward()controlsisFeedForwardActive, which gates only the setpoint feedforward output (FFOut).setDisturbanceFeedforward()manages its own flag,isDisturbanceFeedForwardActive, which gates only the disturbance feedforward output (DistFFOut).- Both outputs are computed separately (lines 44–48) and summed independently into the final PID calculation (line 66).
Calling
activateFeedForward(false)insetupPid()does not affect disturbance feedforward, so the current implementation is correct.Likely an incorrect or invalid review comment.
| float* getPumpFlowPtr() { return &_currentFlow; } // For thermal feedforward | ||
| bool* getValveStatusPtr() { return reinterpret_cast<bool*>(&_valveStatus); } // For thermal feedforward valve state |
There was a problem hiding this comment.
Critical: Unsafe type cast in getValveStatusPtr().
Line 25 uses reinterpret_cast<bool*>(&_valveStatus) where _valveStatus is declared as int (line 52). This is unsafe and leads to undefined behavior:
intis typically 4 bytes, whileboolis typically 1 byte- The reinterpret_cast creates a
bool*pointing to the first byte of theint's memory - Reading through this pointer will only access the first byte, ignoring the upper 3 bytes
- This breaks if
_valveStatusstores values beyond 0/1 or if endianness matters
Recommended solutions:
Option 1 (preferred): Change _valveStatus to bool type if it only stores true/false values.
Option 2: Return int* and let the caller handle the type conversion safely:
int* getValveStatusPtr() { return &_valveStatus; }Option 3: Add a separate bool member that shadows _valveStatus:
bool _valveStatusBool = false;
// Update _valveStatusBool whenever _valveStatus changes
bool* getValveStatusPtr() { return &_valveStatusBool; }🤖 Prompt for AI Agents
In lib/GaggiMateController/src/peripherals/DimmedPump.h around lines 24-25,
getValveStatusPtr() currently does an unsafe
reinterpret_cast<bool*>(&_valveStatus) where _valveStatus is an int; change the
implementation to avoid undefined behavior by either (preferred) changing the
_valveStatus member to type bool and returning its address as bool*, or (if int
must be kept) change the accessor to return an int* so callers handle conversion
safely; if you choose to keep both representations, add a dedicated bool member
maintained in sync with _valveStatus and return its pointer instead.
| float Heater::calculateDisturbanceFeedforwardGain() { | ||
| if (combinedKff <= 0.0f || !pumpFlowRate || *pumpFlowRate <= 0.01f) { | ||
| return 0.0f; | ||
| } | ||
|
|
||
| float currentFlowRate = *pumpFlowRate; // Use raw flow rate for fast response | ||
|
|
||
| // Calculate temperature difference (target - incoming water temperature) | ||
| float tempDelta = setpoint - incomingWaterTemp; | ||
| if (tempDelta <= 0.0f) return 0.0f; | ||
|
|
||
| // Calculate thermal power needed per ml/s of flow (Watts per ml/s) | ||
| float powerPerFlowRate = WATER_DENSITY * WATER_SPECIFIC_HEAT * tempDelta + (heatLossWatts / currentFlowRate); | ||
| powerPerFlowRate /= heaterEfficiency; | ||
|
|
||
| // Apply combined Kff directly (output units per watt) | ||
| float gainPerFlowRate = powerPerFlowRate * combinedKff; | ||
|
|
||
| return gainPerFlowRate; | ||
| } | ||
|
|
||
| float Heater::calculateSafetyScaling(float tempError) { | ||
| // tempError = temperature - setpoint | ||
| // Use smoother, less aggressive safety scaling to reduce oscillations | ||
| if (tempError > 1.0f) { | ||
| return 0.0f; // No FF if more than 1.0°C above setpoint | ||
| } else if (tempError > 0.0f) { | ||
| // Gradual reduction: 100% at 0°C error, 70% at +1.0°C error | ||
| return 0.7f + 0.3f * (1.0f - tempError / 1.0f); | ||
| } else if (tempError > -1.0f) { | ||
| // Scale from 70% to 100% as temperature drops below setpoint | ||
| return 0.7f + 0.3f * std::abs(tempError) / 1.0f; | ||
| } else { | ||
| return 1.0f; // Full FF when more than 1.0°C below setpoint | ||
| } | ||
| } |
There was a problem hiding this comment.
Thermal gain math is reasonable, but fix std::abs include and a safety scaling edge case
The disturbance gain computation and safety scaling logic are generally sound and well guarded, but there are two nits worth fixing:
-
Header for
std::abs
calculateSafetyScalingusesstd::abs(tempError)but this TU doesn’t explicitly include<cmath>. Depending on transitive includes, this may or may not compile cleanly. Adding<cmath>(or moving the helper to a header that already includes it) would make this robust. -
Safety scaling at exactly zero error is inconsistent with comments
FortempError:- Slightly above 0 (e.g.,
0.0001), the second branch yields a factor ≈ 1.0. - Exactly
0.0, the third branch applies, returning0.7.
So there’s a discontinuity at 0°C error, and the comment “100% at 0°C error” does not match the code. A simple fix is to adjust the conditions so 0 belongs to the “100%” side, e.g.:
- Slightly above 0 (e.g.,
- if (tempError > 1.0f) {
- if (tempError > 1.0f) {
return 0.0f;
- } else if (tempError > 0.0f) {
- } else if (tempError >= 0.0f) {
// 100% at 0°C error, 70% at +1.0°C error
return 0.7f + 0.3f * (1.0f - tempError / 1.0f);
- } else if (tempError > -1.0f) {
- } else if (tempError > -1.0f) {
// Scale from 70% to 100% as temperature drops below setpoint
return 0.7f + 0.3f * std::abs(tempError) / 1.0f;
} else {
return 1.0f;
}
These are small tweaks but will make the behavior more predictable and aligned with the documentation.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In lib/GaggiMateController/src/peripherals/Heater.cpp around lines 199–234, add
an explicit #include at the top of the TU to guarantee std::abs is
available, and adjust the safety-scaling branch boundaries so 0.0°C is treated
as “100% at 0°C” (i.e., make the second branch use tempError >= 0.0f and compute
a linear interpolation that yields 1.0 at tempError == 0.0f and 0.7 at tempError
== 1.0f, and keep the third branch handling (-1.0f,0.0) using -tempError or
std::abs(tempError) to produce 0.7→1.0 as tempError goes from +0.0→-1.0,
preserving the >1.0 and <=-1.0 endpoints).
</details>
<!-- fingerprinting:phantom:medusa:olive -->
<!-- This is an auto-generated comment by CodeRabbit -->
| // Disturbance feedforward methods | ||
| void setDisturbanceFeedforward(float disturbance, float gainDFF); | ||
| void setDisturbanceGain(float gainDFF) { gainDistFF = gainDFF; }; | ||
| float getDisturbanceGain() { return gainDistFF; }; | ||
| void activateDisturbanceFeedForward(bool flag) { isDisturbanceFeedForwardActive = flag; }; |
There was a problem hiding this comment.
Verify the interaction between setDisturbanceFeedforward and activateDisturbanceFeedForward.
The setDisturbanceFeedforward method automatically sets isDisturbanceFeedForwardActive = (gainDFF != 0.0f) (line 196 in SimplePID.cpp), but the separate activateDisturbanceFeedForward(bool flag) method allows manual activation regardless of the gain value. This could lead to inconsistent behavior if activateDisturbanceFeedForward(true) is called when gainDistFF is zero, resulting in zero contribution from disturbance feedforward.
Consider either removing the separate activation method or adding validation to ensure gainDistFF is non-zero before allowing activation.
🤖 Prompt for AI Agents
In lib/NayrodPID/src/SimplePID/SimplePID.h around lines 47-51, the separate
activateDisturbanceFeedForward(bool) can be called independently of gainDistFF
(set by setDisturbanceFeedforward), causing activation with zero gain and no
effect; change activateDisturbanceFeedForward to validate gainDistFF before
enabling: if flag is true only set isDisturbanceFeedForwardActive when
gainDistFF != 0.0f (otherwise leave it false or no-op), and keep allowing
flag=false to always disable; update the method comment/signature accordingly so
activation cannot succeed when the disturbance gain is zero.
1 participant
Summary by CodeRabbit
Release Notes