Allow percent values for speed in linear curves

[DanielGibson]

.. in addition to the existing 0..255 system

Contributes to #387

Also allow setting useUnscaledCurveValues: true in fan configs, which prevents the scaling from 0..255 to MinPwm..MaxPwm. So with a PwmMap that maps 0: 0, 1: 1, ..., 255:255 the speed values from the curve are used (almost) unaltered. ("Almost" because if neverStop is set, they're clamped to MinPwm, 255)

---

TODOs from the discussion below:

• [x] Option to disable setting fan mode every cycle
• [ ] Periodic checks for whether the fan mode has been changed (and restoring it), every few seconds or so
  • [ ] Option to disable those as well
  • [ ] If not possible (can't read PWM) and not disabled, print a warning - once, at startup
• [x] Maybe ensure that MinPwm is 1% not 0% (unless NeverStop)?
• [x] Adapt LinearCurve steps mapstructure stuff
• [x] LinearCurve step translation (string->float) in LoadConfig()?
• [x] Rework pwmMap so it really always has 256 entries and doesn't need to call findClosestDistinctTarget() all the time anymore

[DanielGibson]

Let's discuss how this should really work.. like trying to look at the big picture and how it should work to be consistent and useful. TBH one of the aims of all the changes I did here was to enable users to do those two things (and maybe more) in curves, whichever they prefer:

• Define the speed as percentage between min speed and max speed (with special case 0% for "stop")
• Define the speed as raw PWM (by using useUnscaledCurveValues and a 1to1 Pwm Map, though the second part isn't implemented yet, I was thinking about your suggestions from.. err.. somewhere, with that linear: 0: 0 255: 255 thing)

While keeping backwards-compat with existing configs as far as possible

But for speed as percentage, is it better to do this between PWM 0 and MaxPwm or MinPwm and MaxPwm? Maybe the percent of Max RPM is better represented when starting at PWM 0 (even if for the first e.g. 10% the fan just doesn't spin)? Not sure, and of course this is hardware-dependent anyway

[DanielGibson]

I rebased this onto the branch for #396, assuming that it will be merged first.

I implemented the curve step validation with redundant code, but if you can tell me how to do the conversion from (string) Steps to FloatSteps in the config code I'm of course still willing to do that change.

I added a skipAutoPwmMap config option for fans - of course still open for a better name, just like in case of the fan init commandline option.

---

I have a question regarding using the pwmMap:
DefaultFanController.applyPwmMapToTarget() does

closestToTarget := f.findClosestDistinctTarget(target)
return f.pwmMap[closestToTarget]

Somehow findClosestDistinctTarget() feels redundant. Doesn't the pwmMap map every value between 0 and 255 to the appropriate target pwm/speed/whatever? At least the pwmMap doc says "This map always contains the full range of [0..255] as keys" - what would be the point of that if not mapping all values between 0 and 255 to the most appropriate output value?

So shouldn't return f.pwmMap[target] be sufficient?

[DanielGibson]

This fucking linter is driving me insane! First it complains about using just % in fmt.Errorf() - ok, fair point, that's a real bug.

Then it complains about punctuation at the end of an error string - in the same strings! And after fixing that it complains that "error strings should not be capitalized" - whyever the fuck not, an also, couldn't it have said so last time, because it's again about the same fmt.Errorf() calls?!

[markusressel]

Let's discuss how this should really work.. like trying to look at the big picture and how it should work to be consistent and useful. TBH one of the aims of all the changes I did here was to enable users to do those two things (and maybe more) in curves, whichever they prefer:

• Define the speed as percentage between min speed and max speed (with special case 0% for "stop")
• Define the speed as raw PWM (by using useUnscaledCurveValues and a 1to1 Pwm Map, though the second part isn't implemented yet, I was thinking about your suggestions from.. err.. somewhere, with that linear: 0: 0 255: 255 thing)

While keeping backwards-compat with existing configs as far as possible

Not sure about this one, though it sounds good to me at at first glance. The useUnscaledCurveValues option would allow a user to pick the behavior that suits them best, either the way that lights work (see below), or the way fan pwm values typically work, correct?

But for speed as percentage, is it better to do this between PWM 0 and MaxPwm or MinPwm and MaxPwm? Maybe the percent of Max RPM is better represented when starting at PWM 0 (even if for the first e.g. 10% the fan just doesn't spin)? Not sure, and of course this is hardware-dependent anyway

The comparison that comes to my mind is the lighting in my home. The brightness of all of them is controlled in a percentage from [0..100]%. All of these lights turn off at 0% and on at 1%. The light output between different lights at 1% varies, and 1% brightness is not always 1/100 of the light output at 100% (although thats difficult to say anyway because of light and especially brightness perception of the human eye). 1% is (more or less) the minimum brightness the light is able to achieve without turning off because of the voltage is so low that it powers off completely. I guess I would want to assume the same behavior on the fans of my devices.

[markusressel]

I have a question regarding using the pwmMap: DefaultFanController.applyPwmMapToTarget() does

closestToTarget := f.findClosestDistinctTarget(target)
return f.pwmMap[closestToTarget]

Somehow findClosestDistinctTarget() feels redundant. Doesn't the pwmMap map every value between 0 and 255 to the appropriate target pwm/speed/whatever? At least the pwmMap doc says "This map always contains the full range of [0..255] as keys" - what would be the point of that if not mapping all values between 0 and 255 to the most appropriate output value?

So shouldn't return f.pwmMap[target] be sufficient?

Good catch. The documentation states this, because I wanted it to be that way when I did the rework that introduced the setPwmToGetPwmMap. But although the values that are computed automatically by fan2go will always contain the full 0-255 range, the pwmMap specified by the user can have any values, and there is currently no validation for that. Now that there is a method to interpolate a given map using "steps" it would be possible to also "expand" the user input to the full range, but I didn't do that yet, since such an interpolation could potentially require users of fans with f.ex. only three states (1:0, 127:1, 255:3 or something like that) to make adaptations to their config. I didn't have the time to think this through, so I left the original code (mostly) as is, but the comment still got in.

There is also a check to that tries to "compress" the data in the pwmMap again, by ignoring entries that all lead to the same target PWM value. But I am not so happy about this method, as its behavior can be a bit unpredictable for a human. If the 0-255 guarantee were true, we wouldn't need to do this at all, and it would not be an issue if values are present multiple times (it would rather be a feature).

[DanielGibson]

since such an interpolation could potentially require users of fans with f.ex. only three states (1:0, 127:1, 255:3 or something like that) to make adaptations to their config

Why? If for every value between 0 and 255 the output-pwm value is calculated in the way it is now (with the users PWM map) and stored in a PWM map, you'd get the exact same behavior as now, but at runtime need only one lookup in the resulting PWM map instead of findClosestDistinctTarget()

[markusressel]

Why? If for every value between 0 and 255 the output-pwm value is calculated in the way it is now (with the users PWM map) and stored in a PWM map, you'd get the exact same behavior as now, but at runtime need only one lookup in the resulting PWM map instead of findClosestDistinctTarget()

Yes, but I am not sure what kind of interpolation corresponds to "finding the closest value". Or do you want to suggest something else?

I tried to look up issues where the pwmMap was relevant, and these two are probably the most interesting:

https://github.com/markusressel/fan2go/issues/161#issuecomment-1272551915

https://github.com/markusressel/fan2go/issues/311#issuecomment-2365265064

The first is a "three state" fan. The second is a "percentage based fan", similar to the Nvidia driver.

Looking at the pwmMaps in these issues, I think a "step interpolation" would be what is desired, but thats not what "findClosestDistinctTarget" does, which operates more like util.ExpandMapToFullRange() does.

Idk what is more intuitive/logical: This one:

pwmMap:
  0: 0
  128: 128
  255: 255

where the resulting pwmMap is interpolated like ExpandMapToFullRange, "switching" between output pwm values at halfway-point between keys?

Or:

pwmMap:
  0: 0
  64: 128
  195: 255

where the resulting pwmMap is interpolated stepwise, and the keys of the pwmMap provided by the user must already be the halfway-points?

Since the pwmMap is a very technical setting, I think I tend to say the second, which would also not require any breaking change. It would still behave differently though, as the "findClosestDistinctTarget" that would have chosen the key of 195 already at a target value of 160, while a stepped interpolated function would only switch to 195 at this exact value (195). While writing this sounds more reasonable to me though, the old behavior seems misleading.

Thoughts?

[DanielGibson]

Yes, but I am not sure what kind of interpolation corresponds to "finding the closest value". Or do you want to suggest something else?

I'm suggesting a loop iterating from 0 to 255 and doing the exact same calculation that findClosestDistinctTarget() does for each of them (with the user-supplied map), and storing the result in a map that gets actually used. The behavior would be 100% the same as currently, but without the overhead of calling findClosestDistinctTarget() each cycle

[DanielGibson]

Oh right, regarding the half-way points: I think in general the first version is more logical: The user maps values out of the 0..255 range to the appropriate values supported by their fan, and when mapping a value from a curve to the actually supported values, fan2go chooses the closest one. I think this makes most sense, as the values from the curves are also interpolated.

But I'm not entirely sure about the lower bound: 0: 0 128: 128 - this means that PWM target values up to 64 result in the fan not spinning. Is that a problem? IDK.. OTOH, what would MinPWM be in that case? 128 probably? In that case, all curve values (except for 0) make the fan spin at speed 128 (the lowest supported/defined speed), because curve value 1 is mapped to MinPWM. That's consistent with how "normal" fan(controllers), that take PWM values between 0 and 255, behave.

[DanielGibson]

The comparison that comes to my mind is the lighting in my home. The brightness of all of them is controlled in a percentage from [0..100]%. All of these lights turn off at 0% and on at 1%. The light output between different lights at 1% varies

I have (very) slightly tweaked the meaning of curve values once more, for the case that UseUnscaledCurveValues is not set. Now:

• 0 and 0% mean stop (PWM 0), unless NeverStop is set, then it's set to MinPwm
• 1 and 1% now both correspond to MinPwm (no matter if NeverStop is set or not)
• 255 and 100% correspond to MaxPwm, like before
• => Now 1% - 100% gets scaled to 1..255, instead of scaling 0% - 100% to 0..255

[DanielGibson]

I have a question about the following code in computePwmMap(): https://github.com/markusressel/fan2go/blob/007c90877588847d0e5fe58f85642d57b94602e1/internal/controller/controller.go#L706-L711

Why if ... && f.pwmMap != nil { ? Why does it matter if it's nil if it gets replaced in the next line anyway? When is it NOT nil anyway? It gets initialized to nil in NewFanController() and the only places where it gets set are in computePwmMap() (i.e. the function I'm asking about), and if it gets set there before that line (with configOverride), the function returns early (before the line in question).

[DanielGibson]

I implemented an AlwaysSetPwmMode option for fans. If set to true, it sets the PWM Mode / ControlMode each cycle (like before), otherwise (and by default) it doesn't.

Also added several tests for many of the new/changed features

[DanielGibson]

I added documentation for the new options and rebased to current master

[markusressel]

Hey @DanielGibson thx for the update. Sorry for the delay, lots going on right now. I will get back to this soon, hopefully on the weekend :crossed_fingers:

One quick note: Lets try to keep the scope of this PR as is, because it already makes it very hard for me to keep up with everything going on, especially when there are days between the sessions :sweat_smile: This in turn in turn makes it hard to review everything, merge it, and move on to the next bit. I guess this also partly contributes to the delay in response, because the time slot I need to thoroughly go through this gets longer and longer. I get that the changes are all related somehow, I just wanted to mention this to avoid misunderstandings. Thx again!

[DanielGibson]

Ok, then I'll keep those periodic checks for another PR and just do some more (manual) testing of this stuff.

I think it should be in a good shape generally, though I could squash some of the commits together if you want

[DanielGibson]

BTW, I think it would be good to have that periodic check (from a future PR) in version 1.0.0 - it could solve that potential "fan might not be in manual mode after suspend+resume" issue, by using real time: Even with only check once per minute, that minute would most probably be over after suspend+resume (though probably every 10 seconds or would be a saner default)

[markusressel]

Yes, but I am not sure what kind of interpolation corresponds to "finding the closest value". Or do you want to suggest something else?

I'm suggesting a loop iterating from 0 to 255 and doing the exact same calculation that findClosestDistinctTarget() does for each of them (with the user-supplied map), and storing the result in a map that gets actually used. The behavior would be 100% the same as currently, but without the overhead of calling findClosestDistinctTarget() each cycle

Ok thats a good suggestion, but lets leave optimization out of the equation in this PR. I am sure there are lots of things that can be improved about performance in fan2go, lets separate it to another PR.

[markusressel]

The comparison that comes to my mind is the lighting in my home. The brightness of all of them is controlled in a percentage from [0..100]%. All of these lights turn off at 0% and on at 1%. The light output between different lights at 1% varies

I have (very) slightly tweaked the meaning of curve values once more, for the case that UseUnscaledCurveValues is not set. Now:

• 0 and 0% mean stop (PWM 0), unless NeverStop is set, then it's set to MinPwm
• 1 and 1% now both correspond to MinPwm (no matter if NeverStop is set or not)
• 255 and 100% correspond to MaxPwm, like before
• => Now 1% - 100% gets scaled to 1..255, instead of scaling 0% - 100% to 0..255

Sounds good! I made a note about that for the release note draft.

[markusressel]

I will run the current state of the branch on two of my machines for a couple of hours or days to test things out, but first results look promising :+1:

This is my current changelog draft, did I miss anything?

# Noteworthy changes

- The default way to specify curves has now been changed to use percentages to make it more intuitive
  - the previous way (an int range in `[0..255]`) is still supported (see also `useUnscaledCurveValues`)
  - old configrations should be migrated to a float range in `[0.0 ... 100.0]`

- **Behavior Change** The way curve values are interpreted and mapped to a fan speed has changed:
  - `0` and `0%` mean stop (PWM 0), unless NeverStop is set, then it's set to MinPwm
  - `1` and `1%` now both correspond to MinPwm (no matter if NeverStop is set or not)
  - `255` and `100%` correspond to MaxPwm, like before
  - => Now `1%` - `100%` gets scaled to `1..255`, instead of scaling `0%` - `100%` to `0..255`
  - **This is a change in behavior, make sure to verify your setup is working as expected or make necessary adjustments**

- New fan config option `useUnscaledCurveValues: false`
  - defaults to `false`
  - if set to `true`, disables the curve value rescaling of `[0..255]` to `[MinPwm..MaxPwm]`. If active, curve values will be used as fan speed values (almost) unaltered (assuming a "standard" `pwmMap` of `0: 0, 1: 1, ..., 255:255`). "Almost", because if `neverStop: true` is set, clampinig of curve values to `[MinPwm, 255]` will still be applied.

- New CLI flag `--skip-auto-pwm-map` for the `fan2go init` command
  -  disables/skips the automatic detection of supported PWM values

- **Behavior Change** New fan config option `alwaysSetPwmMode: false`
  - defaults to `false`
  - if set to `true`, disables actively setting the pwm mode on each fan controller cycle
  - can be used to work around issues with suspend/resume as well as third party software which is constantly interfering with fan2go
  - **this was previously the hardcoded to `true`, but is now disabled by default**

- do we need to inform users about a change in how to specify pwmMap for fans with only three states?

[DanielGibson]

Yes, but I am not sure what kind of interpolation corresponds to "finding the closest value". Or do you want to suggest something else?

I'm suggesting a loop iterating from 0 to 255 and doing the exact same calculation that findClosestDistinctTarget() does for each of them (with the user-supplied map), and storing the result in a map that gets actually used. The behavior would be 100% the same as currently, but without the overhead of calling findClosestDistinctTarget() each cycle

Ok thats a good suggestion, but lets leave optimization out of the equation in this PR. I am sure there are lots of things that can be improved about performance in fan2go, lets separate it to another PR.

please don't make me remove this from the PR again, this is implemented with tests and everything

[DanielGibson]

I will run the current state of the branch on two of my machines for a couple of hours or days to test things out, but first results look promising 👍

This is my current changelog draft, did I miss anything?

# Noteworthy changes

- The default way to specify curves has now been changed to use percentages to make it more intuitive
  - the previous way (an int range in `[0..255]`) is still supported (see also `useUnscaledCurveValues`)
  - old configrations should be migrated to a float range in `[0.0 ... 100.0]`

Not [0.0 .... 100.0] but [0.0% ... 100.0%] - but both ways are still supported

- **Behavior Change** The way curve values are interpreted and mapped to a fan speed has changed:
  - `0` and `0%` mean stop (PWM 0), unless NeverStop is set, then it's set to MinPwm
  - `1` and `1%` now both correspond to MinPwm (no matter if NeverStop is set or not)
  - `255` and `100%` correspond to MaxPwm, like before
  - => Now `1%` - `100%` gets scaled to `1..255`, instead of scaling `0%` - `100%` to `0..255`
  - **This is a change in behavior, make sure to verify your setup is working as expected or make necessary adjustments**

could mention that the behavior only really changes if neverStop is false (in that case it already scaled 0..255 to MinPwm..MaxPwm before)

- New fan config option `useUnscaledCurveValues: false`
  - defaults to `false`
  - if set to `true`, disables the curve value rescaling of `[0..255]` to `[MinPwm..MaxPwm]`. If active, curve values will be used as fan speed values (almost) unaltered (assuming a "standard" `pwmMap` of `0: 0, 1: 1, ..., 255:255`). "Almost", because if `neverStop: true` is set, clampinig of curve values to `[MinPwm, 255]` will still be applied.

- New CLI flag `--skip-auto-pwm-map` for the `fan2go init` command
  -  disables/skips the automatic detection of supported PWM values

Should be "fan2go fan init". Could mention that it's worth trying if fan init fails or gives weird values, which especially is the case if the set PWM value isn't immediately applied.

- **Behavior Change** New fan config option `alwaysSetPwmMode: false`
  - defaults to `false`
  - if set to `true`, disables actively setting the pwm mode on each fan controller cycle
  - can be used to work around issues with suspend/resume as well as third party software which is constantly interfering with fan2go
  - **this was previously the hardcoded to `true`, but is now disabled by default**

- do we need to inform users about a change in how to specify pwmMap for fans with only three states?

Has that changed? I didn't change the example for that in fan2go.yaml and I think it works and behaves pretty much like before? Though if the values are consecutive (like {0, 1, 2}) then one can skip the pwmmap and just set maxPwm to 2 (same for {0, 1, 2, ..., 99, 100})