Eraden / amdgpud

MIT License
191 stars 11 forks source link

Fanspeeds as floats, 0 is lowest speed, bugfixes #9

Closed BoostCookie closed 3 years ago

BoostCookie commented 3 years ago
  1. The fanspeeds can now be specified as floats for better granularity
  2. Removed 4 as the lowest possible speed. Lowest possible value is now 0.0
  3. min and max values are now correctly read for pwm1 and not the rpm values of fan1 anymore
  4. Percentage speed now gets correctly converted to the pwm values (usually between 0 and 255)
  5. Before the update of the pwm-value was skipped when the current speed was below minimum or above maximum. This got removed. Why was that there in the first place?
  6. Simpler algorithm for speed_for_temp
  7. in monitor the min value is now actually below MIN and the max value below MAX. Before it was swapped
  8. Fixed grammar error "can's" instead of "can't"

Resolves #8 Resolves #4

BoostCookie commented 3 years ago

This fixes both https://github.com/Eraden/amdgpud/issues/8 and https://github.com/Eraden/amdgpud/issues/4 and also a few other bugs/mistakes.

BoostCookie commented 3 years ago

Now I've also noticed that the variable last_temp in CardController has no purpose. https://github.com/Eraden/amdgpud/blob/0794605006e0888875d90c098b29c8215af7b79e/src/main.rs#L147-L159 https://github.com/Eraden/amdgpud/blob/0794605006e0888875d90c098b29c8215af7b79e/src/main.rs#L299 These are all the occasions where it is used => it only gets written and is never read. I think this makes the whole CardController struct a bit obsolete.

BoostCookie commented 3 years ago

To be honest, I don't get why you even merged https://github.com/Eraden/amdgpud/pull/7. This pull request here solves every issue https://github.com/Eraden/amdgpud/pull/7 solves and more. Now because you've merged https://github.com/Eraden/amdgpud/pull/7 first it was possible to make mistakes when resolving the unnecessary merge conflicts.

Nothing against https://github.com/Eraden/amdgpud/pull/7, but it was a rather quick and dirty solution (hardcoding the boundaries to 0 and 255) while this pull request here correctly interpolates between pwm_min() and pwm_max().

BoostCookie commented 3 years ago

BTW, the problem persists in the current version.

https://github.com/Eraden/amdgpud/blob/2c7dd81a304dc69362825c33d6219d666d987734/src/main.rs#L311

I just linked to the version of the original merge where it was introduced. Will you revert the changes or should I create another PR that fixes it again?

Eraden commented 3 years ago

I'll fix it