Improvements to the PWM Fan usermod #5015
Conversation
Use ms between actual measurements for calculation instead of configured seconds between checks
WalkthroughChanged PWM fan logic to use a configurable maxSpeedTemperature (34.5 default) and linear PWM interpolation between pwmMinValue and pwmMaxValue; removed step-based PWM logic. updateTacho() now takes an elapsed-ms parameter; tacho/RPM display and config persistence (including _temperatureMax) were updated and pins reinitialized when changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (5)
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)
usermods/PWM_fan/PWM_fan.cpp (1)
99-112: Improve RPM calculation precision.The current approach uses integer division
msInterval / 1000, which truncates fractional seconds and reduces precision. For example, withmsInterval = 30500ms, the division yields30, discarding 500ms. This contradicts the PR objective of "more precise RPM calculation."Apply this diff to eliminate intermediate truncation:
- 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 /= msInterval / 1000; - // reset counter - counter_rpm = 0; - // attach interrupt again - attachInterrupt(digitalPinToInterrupt(tachoPin), rpm_fan, FALLING); - } + 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 * 1000) / (numberOfInterrupsInOneSingleRotation * msInterval); + // reset counter + counter_rpm = 0; + // attach interrupt again + attachInterrupt(digitalPinToInterrupt(tachoPin), rpm_fan, FALLING); + }
🧹 Nitpick comments (1)
usermods/PWM_fan/PWM_fan.cpp (1)
183-183: Add explicit cast for clarity.The expression
speedFactor * pwmRange + pwmMinValueproduces afloat, which is implicitly converted touint8_twhen passed toupdateFanSpeed. While legal, an explicit cast improves clarity.Apply this diff:
- updateFanSpeed(speedFactor * pwmRange + pwmMinValue); + updateFanSpeed((uint8_t)(speedFactor * pwmRange + pwmMinValue));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PWM_fan/PWM_fan.cpp(8 hunks)
🔇 Additional comments (5)
usermods/PWM_fan/PWM_fan.cpp (5)
217-217: Correctly passes actual elapsed interval.The call now uses the actual milliseconds elapsed since the last measurement, aligning with the PR objective of more precise RPM calculation.
219-221: LGTM!Storing the measurement timestamp enables accurate interval calculation in the next iteration, supporting the improved RPM precision.
251-251: LGTM!The slider now correctly unlocks automatic control when speed is set to 0 (
'lock':parseInt(this.value)>0), implementing the PR objective.
259-260: Improved UI feedback.Now displays the lock/auto state alongside RPM values, implementing the PR objective of showing state when tacho is used.
319-319: Configuration persistence implemented correctly.The new
maxSpeedTemperatureparameter is properly persisted and restored, consistent with other configuration parameters.Also applies to: 353-353, 400-400
| if ((now - msLastTachoMeasurement) < (tachoUpdateSec * 1000)) return; | ||
|
|
||
| updateTacho(); | ||
| updateTacho(now - msLastTachoMeasurement); |
There was a problem hiding this comment.
That's bound to be roughly 30s due to the statement above.
There was a problem hiding this comment.
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.
Changing the speed more granularly makes it less noticeable when it happens and becoming less of a disturbance. What's your suggestion / request on the PR? Will it be accepted after fixing the requested changes? Do you need more explanation or not see it being accepted anyway? |
Apparently you must be using this usermod with different intention than the one used when developing it. The idea is to provide ventilation to a controller enclosure, not living room ventilation. Adding code to already crowded WLED which yields little real benefit is not really desired. However this is a (not default) usermod and some wiggle room may be exerted. I am willing to accept linear (or non linear for that matter) speed adjustment, but only that. No need to change polling interval beyond the ability for user to select one. Perhaps @srg74 would also like to be informed. |
Integer division evaluates before multiplication, causing minPWMValuePct / 100 to be 0 for all values less than 100. For example, with minPWMValuePct = 50: - Current: 50 / 100 * 255 = 0 * 255 = 0 ❌ - Correct: (50 * 255) / 100 = 127 ✓
When pwmMaxValue <= pwmMinValue (due to misconfiguration), the function returns without updating the fan speed, leaving it at its previous state. This could cause overheating if the fan isn't already running at an appropriate speed.
|
The fan I'm using is to used to ventilate the enclosure. The enclosure and LEDs are however in my living area. I understand and support preventing WLED to become some overcrowded multi-tool. The changes I'm providing reduce the complexity of calculations, reduce the number of parameters used, makes the RPM calculation more reliable and robust and adds a three features. If some of these are undesirable, could you please be more specific in the desirable changes I'm suggesting?
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/PWM_fan/PWM_fan.cpp (1)
57-58: Add validation to ensuremaxSpeedTemperature > targetTemperature.Despite the previous review comment, no validation has been added to prevent
maxSpeedTemperature <= targetTemperature. This will cause division by zero at line 185 when both temperatures are equal, or inverted fan behavior when max is less than target.The claim that "this is checked before the critical division" appears incorrect—line 182's check
temp > maxSpeedTemperatureonly validates the measured temperature, not the relationship between the two configuration parameters.Add validation in
readFromConfigafter line 356:maxSpeedTemperature = top[FPSTR(_temperatureMax)] | maxSpeedTemperature; +if (maxSpeedTemperature <= targetTemperature) { + maxSpeedTemperature = targetTemperature + 0.5f; // ensure at least 0.5°C difference +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PWM_fan/PWM_fan.cpp(8 hunks)
🔇 Additional comments (3)
usermods/PWM_fan/PWM_fan.cpp (3)
99-99: Good improvement for RPM accuracy.Passing the actual elapsed milliseconds makes the RPM calculation independent of the
tachoUpdateSecpolling interval and more robust under variable system load.
262-263: Good UX improvement.Displaying the lock/auto state alongside RPM provides clear feedback about the fan's current control mode.
223-224: ****The timestamp reset at line 224 (
msLastTachoMeasurement = now;) was already present in the previous version (HEAD~1). This is not a new addition or bug fix introduced in the current changes. The git history confirms the line existed before the latest commit.Likely an incorrect or invalid review comment.
**Prevent potential division by zero in RPM calculation.** The expression msInterval / 1000 uses integer division. If msInterval < 1000 (less than 1 second), the result is 0, causing division by zero when dividing last_rpm. While the loop check at line 218 ensures at least tachoUpdateSec * 1000 ms have elapsed, defensive programming should handle edge cases. **Use absolute maximum PWM for failsafe, not configured maximum.** When pwmMaxValue <= pwmMinValue due to misconfiguration, the failsafe sets the fan to pwmMaxValue, which is the lower of the two values. This could result in insufficient cooling. For a true failsafe, use _pwmMaxValue (255) to ensure maximum cooling.
This reverts commit 44fab44.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/PWM_fan/PWM_fan.cpp (1)
339-386: Validate temperature relationship in config loading.The
readFromConfigmethod loads bothtargetTemperatureandmaxSpeedTemperaturebut doesn't validate thatmaxSpeedTemperature > targetTemperature. This allows invalid configurations to be persisted and loaded, leading to division by zero during fan speed calculation.Add validation after line 356:
maxSpeedTemperature = top[FPSTR(_temperatureMax)] | maxSpeedTemperature; +// ensure maxSpeedTemperature exceeds targetTemperature +if (maxSpeedTemperature <= targetTemperature) { + DEBUG_PRINT(F("Invalid config: max-temp-C (")); + DEBUG_PRINT(maxSpeedTemperature); + DEBUG_PRINT(F(") must exceed target-temp-C (")); + DEBUG_PRINT(targetTemperature); + DEBUG_PRINTLN(F("). Using defaults.")); + targetTemperature = 34.5f; + maxSpeedTemperature = 38.0f; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PWM_fan/PWM_fan.cpp(8 hunks)
🔇 Additional comments (6)
usermods/PWM_fan/PWM_fan.cpp (6)
57-58: LGTM!The default values for the new linear PWM control parameters are reasonable and ensure
maxSpeedTemperature > targetTemperatureat initialization.Also applies to: 73-73
99-112: Add defensive check formsInterval == 0.While the loop logic (line 218) ensures at least
tachoUpdateSec * 1000ms have elapsed, defensive programming should explicitly guard against edge cases.Apply this diff to add a defensive check:
void updateTacho(unsigned long msInterval) { if (tachoPin < 0) return; + if (msInterval == 0) return; // defensive: avoid division by zero // start of tacho measurement // detach interrupt while calculating rpm detachInterrupt(digitalPinToInterrupt(tachoPin)); // calculate rpm last_rpm = (counter_rpm * 60) / numberOfInterrupsInOneSingleRotation; last_rpm = (last_rpm * 1000) / msInterval; // reset counter counter_rpm = 0; // attach interrupt again attachInterrupt(digitalPinToInterrupt(tachoPin), rpm_fan, FALLING); }
169-175: LGTM!The integer division order has been corrected, and the failsafe properly uses
_pwmMaxValuefor absolute maximum speed when configuration is invalid. These fixes address the critical issues identified in previous reviews.
214-225: LGTM!Passing the actual elapsed milliseconds (
now - msLastTachoMeasurement) toupdateTachomakes the RPM calculation more precise and robust, especially under high system load when the loop timing may vary slightly from the configured interval.
259-267: LGTM!The UI enhancement clearly indicates whether the fan is in locked or auto mode, improving transparency for users when a tachometer is present.
290-297: Ongoing discussion: Lock/unlock UI behavior.The current implementation at line 293 automatically locks the fan when
pwmValuePct > 0. Based on the past review discussion, the maintainer (@blazoncek) expressed preference for a separate UI button approach (option 2) rather than changing backend logic, stating: "I'm willing to take compromise in the form of separate UI button for Lock/Unlock functionality (2). But no other changes to UI or backend functionality."The current implementation doesn't align with that stated preference. Please clarify with the maintainer whether this automatic-locking approach is acceptable or if you should implement the separate button approach instead.
| if (temp < targetTemperature) { | ||
| updateFanSpeed(pwmMinValue); | ||
| } else if(temp > maxSpeedTemperature) { | ||
| updateFanSpeed(pwmMaxValue); | ||
| } else { | ||
| float speedFactor = (temp - targetTemperature) / (maxSpeedTemperature - targetTemperature); // 0 - 1 | ||
| updateFanSpeed(speedFactor * pwmRange + pwmMinValue); | ||
| } |
There was a problem hiding this comment.
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.
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.
2 participants
This is a combined PR for some improvements on the PWM Fan usermod.
More precise RPM calculation
Use milliseconds between actual measurements to calculate the desired speed instead of the configured seconds between checks.
Show locked state when tacho is used
The user interface shows Speed:
locked/autowhen the tacho pin is not used. This is useful information that is lost when the tacho pin is enabled, by showing Speed:1500rpm. This PR changes it to:1500 rpm (locked)1500 rpm (auto)Linear PWM control
Before this PW the PWM speed is configured and calculated using
This PR changes the calculation to allow linear PWM control, instead of steps, and includes defaults to match the previous outcomes.
Disable lock from UI with speed 0
There was no way to disable the fan lock from the user interface. It would stay on until a reset or JSON API request. A user without access to the JSON API would not be able to turn to automatic fan control from the interface.
This PR changes the client side javascript call to unlock the automatic control when the user selects speed 0.
Side effect: the user can no longer disable the fan using the manual override, but would have to change the fan settings.
If any of these changes are undesired or require further improvement, please let me know. I'd be happy to help and look forward to have these changes included in the beautiful WLED.
Summary by CodeRabbit
New Features
Bug Fixes
Chores