hwmon drivers with long update period lead to malfunction

[desowin]

Describe the bug
fan2go assumes that pwm value update period is 5 milliseconds. This is definitely not true for https://github.com/Fred78290/nct6687d driver where the cache update period is 1 second.

To Reproduce
Steps to reproduce the behavior:

1. Trigger fan2go map computation (e.g. delete database) on nct6687d driver running on MSI Z790-P WIFI DDR5 motherboard
2. Wait for the map to be computed
3. Observe that the map is vastly incorrect
4. Notice frequent "changed by third party" warnings

Expected behavior
PWM map is correctly computed. While this might not be feasible, some sanity check (and appropriate warning message if sanity check failed) on computed PWM map would be helpful in troubleshooting.

Screenshots

 INFO  Loading fan curve data for fan 'front_top'...
 WARNING  Fan 'front_top' has not yet been analyzed, starting initialization sequence...
 INFO  Computing pwm map...
  DEBUG   Setting Fan PWM of 'front_top' to 255 ...
...
  DEBUG   Setting Fan PWM of 'front_top' to 2 ...
  DEBUG   Saving pwm map to fan...
  DEBUG   Distinct PWM value targets of fan front_top: [0 3 8 12 21 25 30 34 52 56 61 65 92 96 114 119 137 141 146 150 155 159 163 167 171 176 180 185 189 194 198 203 225 230 247 251]
 INFO  Measuring RPM curve...
  DEBUG   Waiting for fan front_top to settle (current RPM max diff: 40.000000)...
...
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...
  DEBUG   Fan front_top: Actual PWM value differs from requested one, skipping: requested: 251, expected: 2, actual: 0
 INFO  FanController: Using saved value for pwm map of Fan 'front_top'
  DEBUG   Distinct PWM value targets of fan front_top: [0 3 8 12 21 25 30 34 52 56 61 65 92 96 114 119 137 141 146 150 155 159 163 167 171 176 180 185 189 194 198 203 225 230 247 251]
  DEBUG   PWM map of fan 'front_top': map[0:0 1:0 2:0 3:8 4:8 5:8 6:8 7:8 8:13 9:13 10:13 11:13 12:0 13:0 14:0 15:0 16:0 17:0 18:0 19:0 20:0 21:25 22:25 23:25 24:25 25:30 26:30 27:30 28:30 29:30 30:34 31:34 32:34 33:34 34:0 35:0 36:0 37:0 38:0 39:0 40:0 41:0 42:0 43:0 44:0 45:0 46:0 47:0 48:0 49:0 50:0 51:0 52:56 53:56 54:56 55:56 56:60 57:60 58:60 59:60 60:60 61:65 62:65 63:65 64:65 65:0 66:0 67:0 68:0 69:0 70:0 71:0 72:0 73:0 74:0 75:0 76:0 77:0 78:0 79:0 80:0 81:0 82:0 83:0 84:0 85:0 86:0 87:0 88:0 89:0 90:0 91:0 92:98 93:98 94:98 95:98 96:0 97:0 98:0 99:0 100:0 101:0 102:0 103:0 104:0 105:0 106:0 107:0 108:0 109:0 110:0 111:0 112:0 113:0 114:120 115:120 116:120 117:120 118:120 119:0 120:0 121:0 122:0 123:0 124:0 125:0 126:0 127:0 128:0 129:0 130:0 131:0 132:0 133:0 134:0 135:0 136:0 137:142 138:142 139:142 140:142 141:146 142:146 143:146 144:146 145:146 146:150 147:150 148:150 149:150 150:154 151:154 152:154 153:154 154:154 155:159 156:159 157:159 158:159 159:163 160:163 161:163 162:163 163:0 164:0 165:0 166:0 167:170 168:170 169:170 170:170 171:177 172:177 173:177 174:177 175:177 176:181 177:181 178:181 179:181 180:186 181:186 182:186 183:186 184:186 185:190 186:190 187:190 188:190 189:195 190:195 191:195 192:195 193:195 194:199 195:199 196:199 197:199 198:203 199:203 200:203 201:203 202:203 203:0 204:0 205:0 206:0 207:0 208:0 209:0 210:0 211:0 212:0 213:0 214:0 215:0 216:0 217:0 218:0 219:0 220:0 221:0 222:0 223:0 224:0 225:230 226:230 227:230 228:230 229:230 230:0 231:0 232:0 233:0 234:0 235:0 236:0 237:0 238:0 239:0 240:0 241:0 242:0 243:0 244:0 245:0 246:0 247:255 248:255 249:255 250:255 251:2 252:2 253:2 254:2 255:2]
 INFO  PWM settings of fan 'front_top': Min 0, Start 255, Max 255
 INFO  Starting controller loop for fan 'front_top'
 WARNING  PWM of front_top was changed by third party! Last set PWM value was: 2 but is now: 0
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...
 WARNING  PWM of front_top was changed by third party! Last set PWM value was: 2 but is now: 0
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...
 WARNING  PWM of front_top was changed by third party! Last set PWM value was: 2 but is now: 0
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...
 WARNING  PWM of front_top was changed by third party! Last set PWM value was: 2 but is now: 251
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...
 WARNING  PWM of front_top was changed by third party! Last set PWM value was: 2 but is now: 251
  DEBUG   Setting Fan PWM of 'front_top' to 251 ...

Desktop (please complete the following information):

• Distro: Arch Linux
• uname -a: Linux starfighter 6.7.5-arch1-1 #1 SMP PREEMPT_DYNAMIC Sat, 17 Feb 2024 14:02:33 +0000 x86_64 GNU/Linux
• sensors -v: sensors version 3.6.0+git with libsensors version 3.6.0+git
• fan2go version: 0.8.1

Additional context
I can get correct map by increasing pwmSetGetDelay in

fan2go/internal/controller/controller.go

Line 21 in dfa4364

const pwmSetGetDelay time.Duration = 5 * time.Millisecond

from 5 to 1500 milliseconds (with 1000 milliseconds I was still getting wrong map, didn't try values in between due to pretty long time the computation takes).

While it can be argued that the problem is really in nct6687d driver, I think it would make sense to have some better sanity checks in fan2go to help explain weird behavior. Also it would be good to be able to specify direct 0-255 -> 0-255 map in config to workaround this sort of issues. See Fred78290/nct6687d#91 for a nct6687d driver workaround, but I believe such driver workaround is not the way to go.

[markusressel]

Hey @desowin , thx for opening an issue 👍

I think it would make sense to have some better sanity checks in fan2go to help explain weird behavior.

Do you have anything specific in mind?

Also it would be good to be able to specify direct 0-255 -> 0-255 map in config to workaround this sort of issues.

That's already possible. Please have a look at the example config:

fan2go/fan2go.yaml

Line 66 in dfa4364

pwmMap:

[desowin]

I think it would make sense to have some better sanity checks in fan2go to help explain weird behavior.

Do you have anything specific in mind?

Take a look at the PWM map of fan 'front_top': map in the log I posted. It is much easier to see it on graph:

I am not sure if there are genuine cases where there is non-monotonic pwmMap.

Also it would be good to be able to specify direct 0-255 -> 0-255 map in config to workaround this sort of issues.

That's already possible. Please have a look at the example config:

fan2go/fan2go.yaml

Line 66 in dfa4364

pwmMap:

Yes, I know, but I have to put 256 distinct lines after pwmMap for it to work as expected, i.e.

pwmMap:
  0: 0
  1: 1
  2: 2
  3: 3
  ...the list goes on and on...
  254: 254
  255: 255

[markusressel]

@desowin thx for posting the graph, makes it much easier to visualize.
What kind of check would you implement based on this?

I am not sure if there are genuine cases where there is non-monotonic pwmMap.

Neither am I. If working on fan2go has told me anyghing its that fan drivers are not all created equal 😄
The original case (for which the config was introduced) was to support fan drivers that do not allow the full 0-255 range to be used.

Yes, I know, but I have to put 256 distinct lines after pwmMap for it to work as expected, i.e.

So? I mean its ugly, sure, but its in a config file that has to be configured once to never be looked at again.
Do you have anything in mind to improve this?