Skip to content

Improvements to the PWM Fan usermod #5015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Improvements to the PWM Fan usermod #5015

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ea98f18
Fan: more precise RPM calculation
qistoph Oct 21, 2025
ed1c357
Fan: Show locked state when tacho is used
qistoph Oct 21, 2025
3ec1528
Fan: Lineair PWM control
qistoph Oct 21, 2025
44fab44
Fan: Disable lock from UI with speed: 0
qistoph Oct 21, 2025
a33de5d
Fan: fix code review issue
qistoph Oct 23, 2025
8c28def
Fan: fix code review issue
qistoph Oct 23, 2025
05bfebb
Fan: fix code review issues
qistoph Oct 23, 2025
44f96de
Revert "Fan: Disable lock from UI with speed: 0"
qistoph Oct 23, 2025
834556d
Fan: fix code review issues
qistoph Oct 23, 2025
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
58 changes: 32 additions & 26 deletions usermods/PWM_fan/PWM_fan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,23 +54,23 @@ class PWMFanUsermod : public Usermod {
int8_t tachoPin = TACHO_PIN;
int8_t pwmPin = PWM_PIN;
uint8_t tachoUpdateSec = 30;
float targetTemperature = 35.0;
float targetTemperature = 34.5;
float maxSpeedTemperature = 38;
uint8_t minPWMValuePct = 0;
uint8_t maxPWMValuePct = 100;
uint8_t numberOfInterrupsInOneSingleRotation = 2; // Number of interrupts ESP32 sees on tacho signal on a single fan rotation. All the fans I've seen trigger two interrups.
uint8_t pwmValuePct = 0;

// constant values
static const uint8_t _pwmMaxValue = 255;
static const uint8_t _pwmMaxStepCount = 7;
float _pwmTempStepSize = 0.5f;


// strings to reduce flash memory usage (used more than twice)
static const char _name[];
static const char _enabled[];
static const char _tachoPin[];
static const char _pwmPin[];
static const char _temperature[];
static const char _temperatureMax[];
static const char _tachoUpdateSec[];
static const char _minPWMValuePct[];
static const char _maxPWMValuePct[];
Expand All @@ -96,17 +96,15 @@ class PWMFanUsermod : public Usermod {
tachoPin = -1;
}

void updateTacho(void) {
// store milliseconds when tacho was measured the last time
msLastTachoMeasurement = millis();
void updateTacho(unsigned long msInterval) {
if (tachoPin < 0) return;

// start of tacho measurement
// detach interrupt while calculating rpm
detachInterrupt(digitalPinToInterrupt(tachoPin));
// calculate rpm
last_rpm = (counter_rpm * 60) / numberOfInterrupsInOneSingleRotation;
last_rpm /= tachoUpdateSec;
last_rpm = (last_rpm * 1000) / msInterval;
// reset counter
counter_rpm = 0;
// attach interrupt again
Expand Down Expand Up @@ -167,25 +165,26 @@ class PWMFanUsermod : public Usermod {

void setFanPWMbasedOnTemperature(void) {
float temp = getActualTemperature();
// dividing minPercent and maxPercent into equal pwmvalue sizes
int pwmStepSize = ((maxPWMValuePct - minPWMValuePct) * _pwmMaxValue) / (_pwmMaxStepCount*100);
int pwmStep = calculatePwmStep(temp - targetTemperature);
// minimum based on full speed - not entered MaxPercent
int pwmMinimumValue = (minPWMValuePct * _pwmMaxValue) / 100;
updateFanSpeed(pwmMinimumValue + pwmStep*pwmStepSize);
}

uint8_t calculatePwmStep(float diffTemp){
if ((diffTemp == NAN) || (diffTemp <= -100.0)) {
DEBUG_PRINTLN(F("WARNING: no temperature value available. Cannot do temperature control. Will set PWM fan to 255."));
return _pwmMaxStepCount;
int pwmMinValue = minPWMValuePct * _pwmMaxValue / 100;
int pwmMaxValue = maxPWMValuePct * _pwmMaxValue / 100;

if (pwmMaxValue <= pwmMinValue) {
updateFanSpeed(_pwmMaxValue); // fail safe: run at max speed
return;
}
if(diffTemp <=0){
return 0;

int pwmRange = pwmMaxValue - pwmMinValue;


if (temp < targetTemperature) {
updateFanSpeed(pwmMinValue);
} else if(temp > maxSpeedTemperature) {
updateFanSpeed(pwmMaxValue);
} else {
float speedFactor = (temp - targetTemperature) / (maxSpeedTemperature - targetTemperature); // 0 - 1
updateFanSpeed(speedFactor * pwmRange + pwmMinValue);
}
Comment on lines 180 to 191
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add validation to prevent division by zero.

The linear interpolation at line 185 computes (temp - targetTemperature) / (maxSpeedTemperature - targetTemperature). If maxSpeedTemperature == targetTemperature, this results in division by zero.

Your past comment stated "This is checked before the critical division," but no such check is present in the code. The else clause is reached when targetTemperature <= temp <= maxSpeedTemperature, but there is no validation preventing maxSpeedTemperature from equaling targetTemperature.

Apply this diff to add a defensive check before the interpolation:

   } else if(temp > maxSpeedTemperature) {
     updateFanSpeed(pwmMaxValue);
   } else {
+    if (maxSpeedTemperature <= targetTemperature) {
+      updateFanSpeed(pwmMaxValue); // failsafe: invalid config
+      return;
+    }
     float speedFactor = (temp - targetTemperature) / (maxSpeedTemperature - targetTemperature); // 0 - 1
     updateFanSpeed(speedFactor * pwmRange + pwmMinValue);
   }

Additionally, consider adding validation in readFromConfig (after line 356) to log a warning and clamp the value:

if (maxSpeedTemperature <= targetTemperature) {
  DEBUG_PRINTLN(F("WARNING: max-temp-C must exceed target-temp-C. Adjusting."));
  maxSpeedTemperature = targetTemperature + 0.5f;
}
🤖 Prompt for AI Agents 
In usermods/PWM_fan/PWM_fan.cpp around lines 180-187 the else-branch performs a
division by (maxSpeedTemperature - targetTemperature) without guarding against
zero; add a defensive check so that if maxSpeedTemperature equals
targetTemperature you skip the interpolation and set speedFactor to 0 (or
directly set the appropriate fan speed), avoiding division by zero. Also, in
readFromConfig after line 356 add validation that if maxSpeedTemperature <=
targetTemperature you log a warning (using DEBUG_PRINTLN with the provided
message) and clamp maxSpeedTemperature to targetTemperature + 0.5f to ensure the
range is valid.

int calculatedStep = (diffTemp / _pwmTempStepSize)+1;
// anything greater than max stepcount gets max
return (uint8_t)min((int)_pwmMaxStepCount,calculatedStep);
}

public:
Expand Down Expand Up @@ -218,8 +217,11 @@ class PWMFanUsermod : public Usermod {
unsigned long now = millis();
if ((now - msLastTachoMeasurement) < (tachoUpdateSec * 1000)) return;

updateTacho();
updateTacho(now - msLastTachoMeasurement);
Copy link
Collaborator

@blazoncek blazoncek Oct 22, 2025

Choose a reason for hiding this comment

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

That's bound to be roughly 30s due to the statement above.

Copy link
Author

@qistoph qistoph Oct 23, 2025

Choose a reason for hiding this comment

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

That assumes that the call is effectively called every 30s. It could throw off the result if this is not the case due to high load.

It's a little improvement to make the code more robust.

if (!lockFan) setFanPWMbasedOnTemperature();

// store milliseconds when tacho was measured the last time
msLastTachoMeasurement = now;
}

/*
Expand Down Expand Up @@ -257,7 +259,8 @@ class PWMFanUsermod : public Usermod {
JsonArray data = user.createNestedArray(F("Speed"));
if (tachoPin >= 0) {
data.add(last_rpm);
data.add(F("rpm"));
if (lockFan) data.add(F(" rpm (locked)"));
else data.add(F(" rpm (auto)"));
} else {
if (lockFan) data.add(F("locked"));
else data.add(F("auto"));
Expand Down Expand Up @@ -316,6 +319,7 @@ class PWMFanUsermod : public Usermod {
top[FPSTR(_tachoPin)] = tachoPin;
top[FPSTR(_tachoUpdateSec)] = tachoUpdateSec;
top[FPSTR(_temperature)] = targetTemperature;
top[FPSTR(_temperatureMax)] = maxSpeedTemperature;
top[FPSTR(_minPWMValuePct)] = minPWMValuePct;
top[FPSTR(_maxPWMValuePct)] = maxPWMValuePct;
top[FPSTR(_IRQperRotation)] = numberOfInterrupsInOneSingleRotation;
Expand Down Expand Up @@ -349,6 +353,7 @@ class PWMFanUsermod : public Usermod {
tachoUpdateSec = top[FPSTR(_tachoUpdateSec)] | tachoUpdateSec;
tachoUpdateSec = (uint8_t) max(1,(int)tachoUpdateSec); // bounds checking
targetTemperature = top[FPSTR(_temperature)] | targetTemperature;
maxSpeedTemperature = top[FPSTR(_temperatureMax)] | maxSpeedTemperature;
minPWMValuePct = top[FPSTR(_minPWMValuePct)] | minPWMValuePct;
minPWMValuePct = (uint8_t) min(100,max(0,(int)minPWMValuePct)); // bounds checking
maxPWMValuePct = top[FPSTR(_maxPWMValuePct)] | maxPWMValuePct;
Expand Down Expand Up @@ -395,6 +400,7 @@ const char PWMFanUsermod::_enabled[] PROGMEM = "enabled";
const char PWMFanUsermod::_tachoPin[] PROGMEM = "tacho-pin";
const char PWMFanUsermod::_pwmPin[] PROGMEM = "PWM-pin";
const char PWMFanUsermod::_temperature[] PROGMEM = "target-temp-C";
const char PWMFanUsermod::_temperatureMax[] PROGMEM = "max-temp-C";
const char PWMFanUsermod::_tachoUpdateSec[] PROGMEM = "tacho-update-s";
const char PWMFanUsermod::_minPWMValuePct[] PROGMEM = "min-PWM-percent";
const char PWMFanUsermod::_maxPWMValuePct[] PROGMEM = "max-PWM-percent";
Expand Down