WIP: Fix/linear pwm adjustment #267
Conversation
|
Once i find time, i would like to change this PR to:
|
…ional new feature
| # simple: together with pwmChangePerSecond, fan speeds will approach target value | ||
| # with the given max speed. Default is 10 | ||
| controlAlgorithm: | ||
| alg: simple |
There was a problem hiding this comment.
Let's stay with how differentiation of different types is done elsewhere within the config, meaning the type should be the "key" here.
Also, I am not very fond of the name "simple", since it doesn't really explain anything about what its doing. Your initial name for it was "linear", but I don't think thats correct here either, since this only specifies how the pwm values of a fan are adjusted, not to what, so maybe "direct" would be a better fit here?
If so, changing the parameter name to "maxPwmChangePerCycle" could better indicate that this is a modification on the "direct" behavior, which applies once per control loop cycle and puts a "cap/max" on the pwm change from one to the next cycle. Since the length of a cyce is not constant, using "Second" here might not be optimal. Although for a user using a fixed amount of time for this property regardless of how fast the cycle rate is could also be of value, but would make it more difficult to implement the control algorithm, since it would have to transform the limit to multiples of its cycle time, which would not be possible in all configuration variations.... so I would like to stay away from that, at least for the initial implementation.
The "simplest" form of control algorithm should directly apply the calculated target pwm value for each cycle. No other logic involved. I am not even sure adding this "limiter" is a good idea for the simplest form, since its not really direct or linear anymore either.
Examples:
controlAlgorithm:
direct:
pwmChangePerSecond: 10controlAlgorithm:
pidLoop:
p: 0.03
i: 0.002
d: 0.0005There was a problem hiding this comment.
Thinking about it this might not work for types that don't have any configuration properties 🤔 But maybe we can support setting a string ("pidLoop") when using the default values and a map when not.
| @@ -228,21 +228,28 @@ func initializeObjects(pers persistence.Persistence) map[fans.Fan]controller.Fan | |||
| for config, fan := range initializeFans(controllers) { | |||
| updateRate := configuration.CurrentConfig.ControllerAdjustmentTickRate | |||
|
|
|||
| var pidLoop util.PidLoop | |||
| if config.ControlLoop != nil { | |||
There was a problem hiding this comment.
Technically we cannot remove this, but I am not very fond about keeping it forever if we change the structure now.
Maybe we can keep it and add a warning notification, so users are made aware that this is now deprecated.
| HwMon *HwMonFanConfig `json:"hwMon,omitempty"` | ||
| File *FileFanConfig `json:"file,omitempty"` | ||
| Cmd *CmdFanConfig `json:"cmd,omitempty"` | ||
| ControlLoop *ControlLoopConfig `json:"controlLoop,omitempty"` |
There was a problem hiding this comment.
lets mark this deprecated then, see: https://go.dev/wiki/Deprecated
| @@ -420,10 +439,11 @@ func (f *PidFanController) calculateTargetPwm() int { | |||
|
|
|||
| // map the target value to the possible range of this fan | |||
| maxPwm := fan.GetMaxPwm() | |||
| minPwm := fan.GetMinPwm() + f.minPwmOffset | |||
| // minPwm := fan.GetMinPwm() + f.minPwmOffset | |||
There was a problem hiding this comment.
The f.minPwmOffset property is used to make sure a "neverStop" constraint is actually met. If a fan stops spinning when at minPwm, the minPwmOffset should be increased gradually until the fan is spinning again.
Essentially, there is
- the value read from the sensor
- the value computed by the curve based on the sensor value
- the target pwm value computed by the fan controller based on the curve value
- the actually applied pwm value, whis is computed by "sanity checks" (like pwm map, neverStop constraints) based on the target pwm of the fan controller.
There was a problem hiding this comment.
Don't change the entire logic of the PidLoop type. If you want to implement a separate logic that is "simpler", please create a separate file for it. The changes you made here should not be part of the git history (of this file) either.
I think we will end up with supporting multiple fan control loops, so a separate package dedicated to exactly this is probably justified. The PidLoop class can then be used on one of the implementations for a fan control loop.
There was a problem hiding this comment.
guess that was lost in "commit half-assed state 5 months later, will clean up
Addressing #266
This throws out the PID approach, instead linearly approaches the desired fan speed, capped to x amount of change per second. On my system, it feelds solid and not unpleasant.
Work is needed in understanding why the scaling into a window of possible (still-rotating) fan speeds is needed, as this is not really hidden from the user when they are supposed to give a step function themselves.
I understand the idea of staying within supported min and max, but i think it defies expectations when still having to give 0-255 for desired values. Maybe percent is better then. Let the user give fan speed percentage values with internally map in between min and max supported. Maybe even raise the min boundary to lowest-still-spinning when neverStop is set. But it should be made transparent to the user what happens with their values internally.