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

Add 2 parameters -u and -U, respectively Fan-OFF and Fan-ON temperature thresholds #7

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

araynard
Copy link

Add 2 parameters -u and -U, respectively Fan-OFF and Fan-ON temperature thresholds, to enable passive-cooling functionality.

When silence is an important criteria for the NanoPi application (in my case, NAS in my living room / home office), those parameter allow for passive cooling under a desired temperature. For any typical use-case, the thresholds can be set like so:
-U: Fan-ON threshold in °C, set slightly higher than the CPU temperature at IDLE without fan
-u : Fan-OFF threshold in °C, set to a slightly higher than the CPU temperature at IDLE with fan (using the regular PWM control)
Like so, the NanoPi remains silent when IDLE, the fan kicks in as soon as the CPU is under higher load, then the fan stops once the CPU returns to IDLE and is properly cooled. It is preferred to set the 2 thresholds with a sufficient Delta-T, e.g:10°C apart, so avoid toggling the FAN ON and OFF to frequently, which is the problem I encountered by using the current script with -d 0.

With the default values, the script behavior is unchanged.

…re thresholds, to enable passive-cooling functionality
Copy link
Owner

@cgomesu cgomesu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your submission, @araynard. I glanced over the changes and they look reasonable. however, I did notice a change to a default value that requires your attention (see my code review).

in addition, please let me know if you tested the changes before submitting this PR. if you did, inform the distro and kernel version.

I'll take a closer look at your changes tomorrow.

pwm-fan.sh Outdated Show resolved Hide resolved
Copy link
Owner

@cgomesu cgomesu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my second and final round of reviews for the first version of this PR. Please address each of my comments and suggested code changes.

pwm-fan.sh Outdated Show resolved Hide resolved
pwm-fan.sh Outdated Show resolved Hide resolved
pwm-fan.sh Outdated Show resolved Hide resolved
Copy link
Owner

@cgomesu cgomesu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My last two reviews were not included in the last one, so appending them to this one.

pwm-fan.sh Outdated Show resolved Hide resolved
@cgomesu
Copy link
Owner

cgomesu commented Jan 23, 2022

my last code review suggestion was incorrect, so I removed it from the current thread. (I got the -le and -ge confused.)

1. Fix the regex check for -t, -T and -u to behave has expected
2. Change variable name to a more descriptive one: FAN_ON_OFF_STATE and declare it in fan_run_thermal ()
3. Revert TIME_STARTUP to 60 seconds
4. Fix typo in help message
cgomesu added a commit that referenced this pull request Jan 23, 2022
Copy link
Owner

@cgomesu cgomesu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all comments were successfully addressed by the author and via 6589b99.

@cgomesu
Copy link
Owner

cgomesu commented Jan 23, 2022

@araynard, I'll run a few additional tests using the new arguments tomorrow (there's a power outage where my nanopi m4 is, so I cannot test it right now) and if everything looks good, will then merge this into master (along with #8, which is just an amend to README.md to include the new args). you were very thorough, so I do not anticipate any additional issues. again, thank you so much for spending your time to contribute to this project.

@cgomesu
Copy link
Owner

cgomesu commented Jan 24, 2022

there is a slight complication when -u is specified but -U is missing that may cause unwanted behavior. more specifically, if -u is specified but -U isn't, then once the temperature reaches the one specified in -u, the fan will never turn on anymore. this could be avoided by setting a higher default value for THERMAL_ABS_THRESH_ON instead of 1. (notice that the inverse is not so problematic because it keeps FAN_ON_OFF_STATE=1, unless the temp reaches below 0 C, in which case you do not need a fan anyway.)

I won't request additional changes though. I'm just noting this here for future changes. I'm planning on tightening up this script within the next few weeks and will introduce a fix for this issue then.

edited: on a second thought, my observation was incorrect. if temp < threshold_off is false, then temp > threshold_on should be true if -U is missing because the default value is 1. it's all good.

congrats on your first PR here, @araynard!

@cgomesu cgomesu merged commit f193513 into cgomesu:master Jan 24, 2022
cgomesu added a commit that referenced this pull request Jan 24, 2022
Update documentation per #7
@cgomesu cgomesu added the enhancement New feature or request label Jan 25, 2022
@araynard
Copy link
Author

Hi @cgomesu this has been a very positive experience for me and I am motivated to contribute more in the open-source community! In the meantime my NanoPi is always running at home, so do not hesitate to reach out to me if you need to test something.

@cgomesu
Copy link
Owner

cgomesu commented Jan 25, 2022

if you don't mind, take a look at #9. I've made several changes to the code, mostly to improve readability and make it easier for others to edit/customize it in the future by changing defaults right at the top. that PR will be merged into master by the end of this week if no one else chimes in.

in addition, I'm also writing a PID controller (https://en.wikipedia.org/wiki/PID_controller) to add as an alternative to the logistic model. I'm (right now) trying to figure out ways to auto tune a few parameters before submitting a new PR. if you want to receive a message when I submit such a PR (or any other repo related changes), you can select the appropriate watch option:

image

lastly, I noticed that you are not showing up as a contributor after merging your commits to master and I think it has to do with mismatching (or missing) user info between git and github. if you want to fix this (not required), check https://git.luolix.topmunity/t/added-collaborator-with-commits-not-showing-as-contributor/152690 .

@araynard
Copy link
Author

I'm also writing a PID controller

That's pretty cool ! You know, I happen to be a cybernetics engineer :D

Most common and simplest way to tune a PID is to record a "Step response" in order to create a simple model of the system.
You could proceed like so:

  1. run the system at IDLE until the temperature is steady
  2. start recording the CPU temperature, with a reasonable sampling rate, e.g: 500ms (doesn't need to be very fast, especially with the heat-sink installed)
  3. turn ON the fan at constant duty-cycle until the temperature reaches steady-state again
  4. plot the step response, measure the time constant and the delta-T between the start and the finish of the experiment, these parameters are enough to start tuning the PID.

Another interesting experiment would be staring form the CPU loaded 100% until the temperature is stable (and CPU throttling) then activate the FAN, then see how it compares to the first experiment.

Main limitation I see: the CPU load is not considered as an system state here, only the CPU temperature,. Therefore the PID controller must be tuned to reject the disturbance created by varying CPU load. But this is definitely a problem for a later stage !

@cgomesu
Copy link
Owner

cgomesu commented Jan 25, 2022

this feedback is pretty useful. I'm probably going to ping you once when I'm done with the first version of the PID controller then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants