-
-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PWM frequencies and align with Intel/Noctua specs #2
base: master
Are you sure you want to change the base?
Conversation
…ned by pwm_high, which is sufficient enough. removed nonsensical value convertion: pwm / 1024.0 * fan->pwm_soft
… of pwm_min_duty --pwm-low and --pwm-high agrs are left in place to not break compatibility, values are ignored
I don't mind to extend functionality, but old behaviour should be saved for backward compatibility. Users should not have their configs broken due to an update. As for the rest of the logic, I'll ask my HW engineer to look at it. |
Existing configs are not going to be broken, but |
@@ -102,10 +128,11 @@ fan_s *fan_init(unsigned pwm_pin, unsigned pwm_low, unsigned pwm_high, unsigned | |||
goto error; | |||
} | |||
int flags; | |||
// It is safer to assume tacho signal requires to be pulled-up than leave the dangling wire |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler checks if I use all possible enum values in the switch, but I don't think anything needs to be changed here. I'm using a very paranoid set of gcc options.
Since I do not have any telemetry, I do not know what options users use. So I prefer to keep compatibility as much as possible. |
Current implementation is a bit flawed in terms of PWM signal frequency. Measured ~300 kHz PWM signal when launched like this:
Current implementation uses balanced PWM mode (aka MSEN=0, default one) instead of mark-and-sweep. Some fans will work fine with that, but that's not up to the specs.
Noctua PWM fan specification and the Intel PWM fan specification both are recommending mark-space encoding with base frequencies ranging from 21kHz to 28kHz.
This PR addresses frequency issues and aligns produced PWM signal with those specs. Tested on rPi 4B (BCM2711) and rPi 3B+ (BCM2835).
Measured frequencies are now 25 kHz and 23.69 kHz respectively. Small discrepancy is due to different oscillator clocks (rPi4 uses 54 MHz base clock for PWM unlike 19.2 MHz on rPI3) and
wiringPi
lib does not allow to set PWM clock register (unlikepigpio
lib, which allows to choose clock source and uses PLLD by default).Also introducing
--pwm-min-duty
setting, which defaults to 20% (Intel spec recommends this value).Some fans require it, some do not. For example, Noctua NF-A4x10 is spinning fine on lower end (<10%) while Elepeak PA4010S05HN2 fan needs at least around 18% minimal duty cycle on my tests.
As long as PWM upper bound is defined by PWM range now (which itself depends on the base clock) and lower bound is covered by
--pwm-min-duty
(set in percent of the upper bound), both--pwm-low
and--pwm-high
settings are no longer make sense and marked as deprecated (their values will be ignored if set).Fixed minor issues in the software PWM settings along the way, but sw PWM is unreliable as it was, mostly kept intact.
--pwm-soft
now defines wider range upper bound with allowed values up to 1024. Lower range is defined by--pwm-min-duty
(like in hw pwm).Tacho pin readout default bias is changed to pull-up (most of the fans are using open collector output for tachometer pin).