Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

siberex
Copy link

@siberex siberex commented May 21, 2024

Current implementation is a bit flawed in terms of PWM signal frequency. Measured ~300 kHz PWM signal when launched like this:

kvmd-fan --verbose --pwm-pin 12 --speed-const 50

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 (unlike pigpio 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).

@mdevaev
Copy link
Member

mdevaev commented May 21, 2024

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.

@siberex
Copy link
Author

siberex commented May 21, 2024

Existing configs are not going to be broken, but --pwm-low and --pwm-high values will be ignored if set (because range is depending on the target frequency now, to align with the specs).
I don't think anybody could practically rely on --pwm-low/--pwm-high settings (with the existing implementation they just produce unpredictable results, depending on the board oscillator). Still, if you feel those values should not be ignored, I will return them back

src/fan.c Show resolved Hide resolved
@@ -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
Copy link
Member

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.

@mdevaev
Copy link
Member

mdevaev commented May 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants