Simplifications

[coldfix]

Hi,

Your fan control script was really useful for me, but I still found some things to simplify and improve (some serious) :)

I realize that this PR changes many aspects at once and I can split it up if you prefer (although I think that wouldn't do much good either and we would have to merge sequentially).

Things that changed (also see individual commits):

• removed unused imports and fix flake8 style complaints (I can also add a .github action for style checks using flake8 if you want)
• removed noise due to comments that just repeated what the code was saying anyway
• fix shebang to use /usr/bin/env rather than point to a specific python interpreter using an absolute path directly; make the script actually executable
• fix atexit handling: previously only triggered (twice!) on KeyboardInterrupt, and in no other scenario; now works as was probably intended, even catching SIGTERM
• simplify code (unneeded return statements, redundant cast to float, inline oneshot functions)
• simplify and document the dead band logic (previously was somewhat hard to wrap hand around for me, now very easy to configure)
• skip redundant calls to GPIO library

Regarding the frequency: I checked that the API takes values in Hz (not kHz) by setting low frequency values (2, 1, 0.5, 0.1) which makes it easy to observe the cycle length. Also, it's documented here https://sourceforge.net/p/raspberry-gpio-python/wiki/PWM/. What I don't know is whether the optimal frequency for the Noctua fan is in Hz or kHz?

Best, Thomas

[DriftKingTW]

Hi,

Thanks for your help! I'm not that familiar with Python, glad that someone can point out these issues. I review the code and they looks good to me :) also the github action for style checks would be a good idea.

About the frequency: this official documentation says it's 25kHz, but there's someone on my blog pointed out that RPI.GPIO library can't produce frequency this high (looks like it can only do software PWM)

Unfortunately my blog's comment extension is currently broken so I can't show you the discussion. Seems that we need to use other library like pigpio to make hardware 25kHz PWM work. (currently don't have the fan and pi around, can't test either)

BTW I actually wrote this script with PWM_FREQ = 25000 at first, it works but CPU usage is weirdly high (10~20%). Then I might looked for the wrong documentation and thinks it's kHz, strangely after this change not only the CPU usage goes down to around 1% and the fan control still works fine.

Please let me know if you have any thoughts on this, thanks!

[coldfix]

Hi,

thanks for your reply:)

the github action for style checks would be a good idea.

I'm on it. Should I add it to this PR or make a new one?

About the frequency: this official documentation says it's 25kHz, but there's someone on my blog pointed out that RPI.GPIO library can't produce frequency this high (looks like it can only do software PWM)

Unfortunately my blog's comment extension is currently broken so I can't show you the discussion. Seems that we need to use other library like pigpio to make hardware 25kHz PWM work. (currently don't have the fan and pi around, can't test either)

BTW I actually wrote this script with PWM_FREQ = 25000 at first, it works but CPU usage is weirdly high (10~20%). Then I might looked for the wrong documentation and thinks it's kHz, strangely after this change not only the CPU usage goes down to around 1% and the fan control still works fine.

Ok. I'm pretty sure the value provided to RPi.GPIO is in Hz. The RPi.GPIO page says it can only do software PWM:

Note that the current release does not support SPI, I2C, hardware PWM or serial functionality on the RPi yet. This is planned for the near future - watch this space! One-wire functionality is also planned.

Although hardware PWM is not available yet, software PWM is available to use on all channels.

So that may be the reason that high frequencies don't work very well. I had the experience that setting high frequencies (> 1000 Hz) gives unhealthy sounding squeaking noise.

So, I've tried to do it with low frequencies around 25Hz and found that the experience is not optimal either (fan stumbles), so if they actually want kHz that would make sense.

This is also why I'm currently personally using a discrete on/off mode based on your script (which I can contribute if you like).

This page here has an example on how to do hardware PWM with wiringPi (only in C, but I have some experience with making and distributing C extension modules with cython, so that wouldn't be a show-stopper).

I suggest, I update the comment about kHz/Hz to be more accurate about the dilemma and we merge this PR before fixing the hardware/software PWM issue (since this PR doesn't change behaviour with regards to the frequencies anyway).

[coldfix]

I've added a simple action and the suggested comment for now.

While on the topic of actions/deployment:

If you're interested I could create a PR for publishing this as a package on PyPI similar to my cecmap package. My vision would be the following:

• make it installable via pip3 install pifancontrol (with a name to be suggested by you)
• also installs .service file, so that systemctl --user start pifancontrol can be used right after
• simple .cfg config file that reads the values for PWM_FREQ, FAN_PIN, etc
• config reloadable with systemctl --user reload pifancontrol
• automatic releases whenever a tag vX.Y.Z is pushed

Might be overkill for such a simple package, but on the other hand, I've done it enough that it's not too much effort and sometimes it's just nice for users to be able to set it up with 2 commands without having to git clone anything or manually manage the "installation" directory (keep it around forever in your home folder, or manually copy it to /etc? also don't forget to change the .service file accordingly, etc).

[DriftKingTW]

I'm on it. Should I add it to this PR or make a new one?

I'll merge them together in this PR.

This is also why I'm currently personally using a discrete on/off mode based on your script (which I can contribute if you like).

This page here has an example on how to do hardware PWM with wiringPi (only in C, but I have some experience with making and distributing C extension modules with cython, so that wouldn't be a show-stopper).

Cool. I'm interested in the discrete on/off version that you've mentioned, it would be nice if you can put it on Github gist or something like that :)

And yeah I think using wiringPi is more suitable for this project.

If you're interested I could create a PR for publishing this as a package on PyPI similar to my cecmap package.

Agreed on you and I think that's an awesome idea 👍 Install process is way more friendly for those who are new to the Pi or Linux.

And the package name, how about pipwmfancontrol? I feel that it's more distinguishable from DC control. The rest looks good enough for me, thanks again for your contribution!

[coldfix]

Just FYI, I've reconsidered and won't be adding packaging for now. The reason is that, first, it's a bit premature considering there's still that open issue regarding hardware vs software PWM, and second, a bit more emotional: all the setup and workflow files, metadata, config file reader, would deduct from the plain and awesome simplicity that the package is currently in. I might be coming back to this later if that hardware PWM issue is resolved, let's see.