Add staircase fan curve#486
Conversation
• Replaced raw sensor.GetValue() with the smoothed, background-polled sensor.GetMovingAvg() to utilize standard architecture patterns and avoid expensive, redundant active polling.
• Added checking for !exists || sensor == nil to prevent runtime panics.
• Replaced the 0 -initialized targetTemp logic with a math.MinInt sentinel. This allows negative step temperatures (e.g., -10°C ) to evaluate correctly and allows step values at exactly 0°C
without conflicts.
• Cleaned up tab indentation styling.
2. Simplified Configuration Verification (config.go):
• Removed the nested redundant length checks in applyTransformations() , making sure empty step configs trigger a fatal config error correctly.
3. Added Tests & Verified (staircase_test.go):
• Added TestStaircaseCurveWithNegativeTemperatures to assert proper hysteresis and matching of sub-zero steps.
• Added TestStaircaseCurveWithStepAtZero to verify steps configured at exactly 0°C function properly.
• Ran make test and go test on the packages; all test suites compiled successfully and passed.
|
AI PR description: OverviewThis PR introduces a new fan curve type: Staircase Fan Curve. A staircase speed curve maintains static fan speeds at defined temperature ranges. This prevents fans from constantly ramping up and down (oscillating) in response to minor, rapid fluctuations in sensor temperature, increasing hardware lifespan and offering a more consistent acoustic profile. It implements:
Configuration ExampleTo configure a staircase curve, define it under curves:
- id: staircase_curve
staircase:
sensor: cpu_package
threshold: 6 # Temperature drop threshold (in °C) before reducing fan speed
steps:
# Temperature (in °C) -> Speed (0-255 or 0%-100%)
- 40: 10%
- 50: 50
- 80: 255Changes1. Configuration & Validation
2. Curve Implementation
3. Testing
4. Miscellaneous & Documentation
|
|
@0xIO32 I have pushed some changes, let me know if you see any issues with them. |
I wonder if it would make more sense to configure a time duration threshold, instead of a difference in sensor input 🤔 But since I have no use case for this anyway we can use whatever you need. |
I just added a description. Somehow in my mind I thought I already explained it in the commit msg or README.md, but apparently not. Intended use: alternative for linear fan curve.
I noticed #444 after already making this pr. |
Only a merge commit or are there changes in functionality? Just to know if I only should do testing again or also look at the changes. |
|
Disclaimer: I haven't coded in go that long, primarily other languages like Rust. Still 2,5 things I noticed: - value = steps[targetTemp]
+ if targetTemp == math.MinInt {
+ value = 0.0
+ } else {
+ value = steps[targetTemp]
+ }Isn't 0 the default if the value isn't present in steps map? - if targetTemp < c.LastTemp && (c.LastTemp-int(measured/1000)) < c.Config.Staircase.Threshold {
+
+ if targetTemp < c.LastTemp && c.LastTemp != math.MinInt && (c.LastTemp-int(measured/1000)) < c.Config.Staircase.Threshold {How is it possible for - targetTemp = max(targetTemp, temp)
+ if targetTemp == math.MinInt || temp > targetTemp {
+ targetTemp = temp
+ }Same here. |
|
Sorry for the noise, this release-drafter error is killing me. |
|
Just tested this build, only for a few minutes. Nothing unusual. |
Apparently it is, I didn't know that.
It isn't, thx. I have simplified the implementation. |
@0xIO32 Is this intentional? Only the down-ramping is supposed to be debounced? |
From my understanding it is. Reacting to a temperature rise should be instant to protect the hardware, reducing the fan speed can be delayed. It is supposed to be a standard staircase / stepped fan curve, although I couldn't actually find a good detailed description about that. |
Alright, I just wanted to make sure because the thing that it probably is supposed to resolve is "too quick speed step changes", and technically that can still occur in the upwards case. But we can keep it like this for now and see if the need for thresholds in both directions is needed. But I have one thing I am still thinking about: curves:
- id: staircase_curve
staircase:
sensor: cpu_package
hysteresis:
down: 6 # Temperature drop threshold (in °C) before reducing fan speed
steps:
# Temperature (in °C) -> Speed (0-255 or 0%-100%)
- 40: 10%
- 50: 50
- 80: 255That would also easily allow us to add an "up" option in the future. |
|
The name shouldn't really matter, so I am also fine with hysteresis if you prefer that name. |
|
I think that's it for now, looks great! |
Add a fan curve similar to linear, but without linear interpolation (i.e. it looks for the highest step that is still below or equal the sensors value and takes the fan speed as-is). The threshold prevents a "start-stop-start-stop" as described in #444 .
This fan curve prevents the fan from constantly ramping up. It usually is quieter and "less annoying".