Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions lib/GaggiMateController/src/GaggiMateController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,17 @@ void GaggiMateController::setup() {
pressureSensor->setup();
_ble.registerPressureScaleCallback([this](float scale) { this->pressureSensor->setScale(scale); });
}

// Set up thermal feedforward for main heater if pressure/dimming capability exists
if (heater && _config.capabilites.dimming && _config.capabilites.pressure) {
auto dimmedPump = static_cast<DimmedPump *>(pump);
float* pumpFlowPtr = dimmedPump->getPumpFlowPtr();
bool* valveStatusPtr = dimmedPump->getValveStatusPtr();

heater->setThermalFeedforward(pumpFlowPtr, 23.0f, valveStatusPtr);
heater->setFeedforwardScale(0.0f);


}
// Initialize last ping time
lastPingTime = millis();

Expand Down Expand Up @@ -111,7 +121,13 @@ void GaggiMateController::setup() {
dimmedPump->setValveState(valve);
});
_ble.registerAltControlCallback([this](bool state) { this->alt->set(state); });
_ble.registerPidControlCallback([this](float Kp, float Ki, float Kd) { this->heater->setTunings(Kp, Ki, Kd); });
_ble.registerPidControlCallback([this](float Kp, float Ki, float Kd, float Kf) {
this->heater->setTunings(Kp, Ki, Kd);

// Apply thermal feedforward parameters if available
this->heater->setFeedforwardScale(Kf);

});
_ble.registerPumpModelCoeffsCallback([this](float a, float b, float c, float d) {
if (_config.capabilites.dimming) {
auto dimmedPump = static_cast<DimmedPump *>(pump);
Expand Down
2 changes: 2 additions & 0 deletions lib/GaggiMateController/src/peripherals/DimmedPump.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class DimmedPump : public Pump {
float getPumpFlow();
float getPuckFlow();
float getPuckResistance();
float* getPumpFlowPtr() { return &_currentFlow; } // For thermal feedforward
bool* getValveStatusPtr() { return reinterpret_cast<bool*>(&_valveStatus); } // For thermal feedforward valve state
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  • int is typically 4 bytes, while bool is typically 1 byte
  • The reinterpret_cast creates a bool* pointing to the first byte of the int's memory
  • Reading through this pointer will only access the first byte, ignoring the upper 3 bytes
  • This breaks if _valveStatus stores 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.

void tare();

void setFlowTarget(float targetFlow, float pressureLimit);
Expand Down
84 changes: 83 additions & 1 deletion lib/GaggiMateController/src/peripherals/Heater.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,23 @@ void Heater::setTunings(float Kp, float Ki, float Kd) {
}
}


void Heater::setThermalFeedforward(float *pumpFlowPtr, float incomingWaterTemp, bool *valveStatusPtr) {
pumpFlowRate = pumpFlowPtr;
valveStatus = valveStatusPtr;
this->incomingWaterTemp = incomingWaterTemp;

ESP_LOGI(LOG_TAG, "Thermal feedforward setup - incoming water temp: %.1f°C, valve tracking: %s",
incomingWaterTemp, valveStatusPtr ? "enabled" : "disabled");
ESP_LOGI(LOG_TAG, "Feedforward will be %s based on Kff value (currently %.3f)",
combinedKff > 0.0f ? "ENABLED" : "DISABLED", combinedKff);
}

void Heater::setFeedforwardScale(float combinedKff) {
this->combinedKff = combinedKff;
ESP_LOGI(LOG_TAG, "Combined feedforward gain (Kff) set to: %.3f output units per watt", combinedKff);
}

void Heater::autotune(int goal, int windowSize) {
setupAutotune(goal, windowSize);
autotuning = true;
Expand All @@ -73,7 +90,35 @@ void Heater::autotune(int goal, int windowSize) {
void Heater::loopPid() {
softPwm(TUNER_OUTPUT_SPAN);
temperature = sensor->read();
if (simplePid->update()) {

// Calculate and set disturbance feedforward BEFORE PID update
// Only apply thermal feedforward when Kf>0, valve is open, and water is flowing
if (combinedKff > 0.0f && pumpFlowRate && *pumpFlowRate > 0.01f && valveStatus && *valveStatus) {
float currentFlowRate = *pumpFlowRate; // Use raw flow rate for fast response
float disturbanceGain = calculateDisturbanceFeedforwardGain();

// Apply smoothed temperature-based safety scaling
float tempError = temperature - setpoint;
float rawSafetyFactor = calculateSafetyScaling(tempError);

// Smooth safety factor transitions to reduce oscillations
const float safetyAlpha = 0.85f; // Faster response for quicker feedforward
float safetyFactor = safetyAlpha * rawSafetyFactor + (1.0f - safetyAlpha) * lastSafetyFactor;
lastSafetyFactor = safetyFactor;

disturbanceGain *= safetyFactor;

// Set the disturbance feedforward in SimplePID
simplePid->setDisturbanceFeedforward(currentFlowRate, disturbanceGain);

} else {
simplePid->setDisturbanceFeedforward(0.0f, 0.0f);
}

// Now run PID with proper feedforward integrated
bool pidUpdated = simplePid->update();

if (pidUpdated) {
plot(output, 1.0f, 1);
}
}
Expand Down Expand Up @@ -151,6 +196,43 @@ void Heater::plot(float optimumOutput, float outputScale, uint8_t everyNth) {
plotCount++;
}

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
}
}
Comment on lines +199 to +234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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:

  1. Header for std::abs
    calculateSafetyScaling uses std::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.

  2. Safety scaling at exactly zero error is inconsistent with comments
    For tempError:

    • Slightly above 0 (e.g., 0.0001), the second branch yields a factor ≈ 1.0.
    • Exactly 0.0, the third branch applies, returning 0.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.:

  • 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 -->


void Heater::loopTask(void *arg) {
TickType_t lastWake = xTaskGetTickCount();
auto *heater = static_cast<Heater *>(arg);
Expand Down
21 changes: 21 additions & 0 deletions lib/GaggiMateController/src/peripherals/Heater.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ class Heater {
void setSetpoint(float setpoint);
void setTunings(float Kp, float Ki, float Kd);
void autotune(int goal, int windowSize);


// Thermal feedforward control
void setThermalFeedforward(float *pumpFlowPtr = nullptr, float incomingWaterTemp = 23.0f, bool *valveStatusPtr = nullptr);
void setFeedforwardScale(float combinedKff); // Set combined Kff value (output units per watt)


private:
void setupPid();
Expand All @@ -34,6 +40,8 @@ class Heater {
float softPwm(uint32_t windowSize);
void plot(float optimumOutput, float outputScale, uint8_t everyNth);
void setTuningGoal(float percent);
float calculateDisturbanceFeedforwardGain();
float calculateSafetyScaling(float tempError);
TemperatureSensor *sensor;
uint8_t heaterPin;
xTaskHandle taskHandle;
Expand All @@ -59,6 +67,19 @@ class Heater {
bool startup = true;
bool autotuning = false;

// Thermal feedforward variables
float *pumpFlowRate = nullptr;
bool *valveStatus = nullptr;
float lastSafetyFactor = 1.0f; // For smooth safety scaling transitions
float incomingWaterTemp = 23.0f;
float heaterEfficiency = 0.95f; // 95% efficiency (immersion heater)
float heatLossWatts = 5.0f; // 5W heat loss (well-insulated boiler)
float combinedKff = 0.0f; // Combined feedforward gain (output units per watt) - disabled by default

// Thermal model constants
static constexpr float WATER_DENSITY = 1.0f; // g/ml
static constexpr float WATER_SPECIFIC_HEAT = 4.18f; // J/(g·°C)

const char *LOG_TAG = "Heater";
static void loopTask(void *arg);
};
Expand Down
16 changes: 14 additions & 2 deletions lib/NayrodPID/src/SimplePID/SimplePID.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ bool SimplePID::update() {

// Compute the filtered setpoint values
float FFOut = 0.0f;
float DistFFOut = 0.0f;

if (isfilterSetpointActive) {
setpointFiltering(setpointFilterFreq);
} else {
Expand All @@ -41,6 +43,10 @@ bool SimplePID::update() {

if (isFeedForwardActive)
FFOut = setpointDerivative * gainFF;

if (isDisturbanceFeedForwardActive)
DistFFOut = currentDisturbance * gainDistFF;

Serial.printf("%.2f\t %.2f\t %.2f\t %.2f\n", *setpointTarget, setpointFiltered, setpointDerivative, *sensorOutput);

float deltaTime = 1.0f / ctrl_freq_sampling; // Time step in seconds
Expand All @@ -57,7 +63,7 @@ bool SimplePID::update() {
float Dout = gainKd * derivative;

// Calculate the output before antiwindup clamping
float sumPID = Pout + Iout + Dout + FFOut;
float sumPID = Pout + Iout + Dout + FFOut + DistFFOut;
float sumPIDsat = constrain(sumPID, ctrlOutputLimits[0], ctrlOutputLimits[1]);

// Antiwindup clamping
Expand All @@ -71,7 +77,7 @@ bool SimplePID::update() {
error * deltaTime; // Forbide the integration to happen when the output is saturated and the error is in the same
// direction as the output (i.e. the system is not able to follow the setpoint)
Iout = gainKi * feedback_integralState; // Recompute the integral term with the new state
sumPID = Pout + Iout + Dout + FFOut; // Recompute the output with the new integral state
sumPID = Pout + Iout + Dout + FFOut + DistFFOut; // Recompute the output with the new integral state
sumPIDsat = constrain(sumPID, ctrlOutputLimits[0], ctrlOutputLimits[1]);
}

Expand Down Expand Up @@ -183,3 +189,9 @@ void SimplePID::activateFeedForward(bool flag) {
isFeedForwardActive = flag;
}
}

void SimplePID::setDisturbanceFeedforward(float disturbance, float gainDFF) {
currentDisturbance = disturbance;
gainDistFF = gainDFF;
isDisturbanceFeedForwardActive = (gainDFF != 0.0f);
}
12 changes: 12 additions & 0 deletions lib/NayrodPID/src/SimplePID/SimplePID.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ class SimplePID {
void setKi(float val) { gainKi = val; };
void setKd(float val) { gainKd = val; };
void setKFF(float val) { gainFF = val; };

// 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; };
Comment on lines +47 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


private:
// setpoint filtering
Expand All @@ -66,6 +72,12 @@ class SimplePID {
float gainKi = 0.0f; // Integral gain (multiplies by Kp if Kp,Ki,Kd are strictly parallèle (no factoring by Kp))
float gainKd = 0.0f; // Derivative gain (by default no derivative term)
float gainFF = 0.5 * 1000.0f / 2.5f; // Feedforward gain

// Disturbance feedforward variables
float gainDistFF = 0.0f; // Disturbance feedforward gain
float currentDisturbance = 0.0f; // Current disturbance value
bool isDisturbanceFeedForwardActive = false; // Flag to activate disturbance feedforward

float feedback_integralState = 0.0f; // Integral state
float prevError = 0.0f; // Previous error for derivative calculation
float prevOutput = 0.0f; // Previous output for derivative calculation
Expand Down
8 changes: 7 additions & 1 deletion lib/NimBLEComm/src/NimBLEClientController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,13 @@ void NimBLEClientController::notifyCallback(NimBLERemoteCharacteristic *pRemoteC
float Kp = get_token(settings, 0, ',').toFloat();
float Ki = get_token(settings, 1, ',').toFloat();
float Kd = get_token(settings, 2, ',').toFloat();
autotuneResultCallback(Kp, Ki, Kd);

// Handle optional Kf parameter with default
float Kf = 0.0f; // Default combined Kff
String kfToken = get_token(settings, 3, ',');
if (kfToken.length() > 0) Kf = kfToken.toFloat();

autotuneResultCallback(Kp, Ki, Kd, Kf);
}
}
if (pRemoteCharacteristic->getUUID().equals(NimBLEUUID(VOLUMETRIC_MEASUREMENT_UUID))) {
Expand Down
2 changes: 1 addition & 1 deletion lib/NimBLEComm/src/NimBLEComm.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ constexpr size_t ERROR_CODE_RUNAWAY = 4;
constexpr size_t ERROR_CODE_TIMEOUT = 5;

using pin_control_callback_t = std::function<void(bool isActive)>;
using pid_control_callback_t = std::function<void(float Kp, float Ki, float Kd)>;
using pid_control_callback_t = std::function<void(float Kp, float Ki, float Kd, float Kf)>;
using pump_model_coeffs_callback_t = std::function<void(float a, float b, float c, float d)>;
using ping_callback_t = std::function<void()>;
using remote_err_callback_t = std::function<void(int errorCode)>;
Expand Down
21 changes: 17 additions & 4 deletions lib/NimBLEComm/src/NimBLEServerController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,9 @@ void NimBLEServerController::sendSteamBtnState(bool steamButtonStatus) {

void NimBLEServerController::sendAutotuneResult(float Kp, float Ki, float Kd) {
if (deviceConnected) {
char pidStr[30];
snprintf(pidStr, sizeof(pidStr), "%.3f,%.3f,%.3f", Kp, Ki, Kd);
char pidStr[64];
// Send with default Kf=0.0 (disabled)
snprintf(pidStr, sizeof(pidStr), "%.3f,%.3f,%.3f,0.0", Kp, Ki, Kd);
autotuneResultChar->setValue(pidStr);
autotuneResultChar->notify();
}
Expand Down Expand Up @@ -236,9 +237,21 @@ void NimBLEServerController::onWrite(NimBLECharacteristic *pCharacteristic) {
float Kp = get_token(pid, 0, ',').toFloat();
float Ki = get_token(pid, 1, ',').toFloat();
float Kd = get_token(pid, 2, ',').toFloat();
ESP_LOGV(LOG_TAG, "Received PID settings: %.2f, %.2f, %.2f", Kp, Ki, Kd);

// Optional thermal feedforward parameter (default value if not provided)
float Kf = 0.0f; // Default combined feedforward gain

String kfToken = get_token(pid, 3, ',');

if (kfToken.length() > 0 && kfToken.toFloat() > 0.0f) {
Kf = kfToken.toFloat();
}

ESP_LOGI(LOG_TAG, "BLE received PID string: '%s'", pid.c_str());
ESP_LOGI(LOG_TAG, "Parsed PID: Kp=%.2f, Ki=%.2f, Kd=%.2f, Kf=%.3f (combined)",
Kp, Ki, Kd, Kf);
if (pidControlCallback != nullptr) {
pidControlCallback(Kp, Ki, Kd);
pidControlCallback(Kp, Ki, Kd, Kf);
}
} else if (pCharacteristic->getUUID().equals(NimBLEUUID(PUMP_MODEL_COEFFS_CHAR_UUID))) {
auto pumpModelCoeffs = String(pCharacteristic->getValue().c_str());
Expand Down
9 changes: 5 additions & 4 deletions src/display/core/Controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,11 @@ void Controller::setupBluetooth() {
ESP_LOGE(LOG_TAG, "Received error %d", error);
}
});
clientController.registerAutotuneResultCallback([this](const float Kp, const float Ki, const float Kd) {
ESP_LOGI(LOG_TAG, "Received new autotune values: %.3f, %.3f, %.3f", Kp, Ki, Kd);
char pid[30];
snprintf(pid, sizeof(pid), "%.3f,%.3f,%.3f", Kp, Ki, Kd);
clientController.registerAutotuneResultCallback([this](const float Kp, const float Ki, const float Kd, const float Kf) {
ESP_LOGI(LOG_TAG, "Received autotune values: Kp=%.3f, Ki=%.3f, Kd=%.3f, Kf=%.3f (combined)", Kp, Ki, Kd, Kf);
char pid[64];
// Store in simplified format with combined Kf
snprintf(pid, sizeof(pid), "%.3f,%.3f,%.3f,%.3f", Kp, Ki, Kd, Kf);
settings.setPid(String(pid));
pluginManager->trigger("controller:autotune:result");
autotuning = false;
Expand Down
2 changes: 1 addition & 1 deletion src/display/core/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#define MAX_TEMP 160
#define DEFAULT_TEMPERATURE_OFFSET 0
#define DEFAULT_PRESSURE_SCALING 16.0f
#define DEFAULT_PID "58.397,1.027,249.055"
#define DEFAULT_PID "58.397,1.027,249.055,0.0"
#define DEFAULT_PUMP_MODEL_COEFFS "10.205,5.521"
#define DEFAULT_MDNS_NAME "gaggimate"
#define DEFAULT_OTA_CHANNEL "latest"
Expand Down
Loading
Loading