Control loop logic does not really approach target values #266
Comments
jwefers
changed the title
|
Currently debugging the code, and three observations so far: 1) - id: gpu
cmd:
exec: /usr/bin/bash
args:
- "-c"
- 'echo "$(( $(/usr/bin/nvidia-smi --query-gpu=temperature.gpu --format=csv,noheader) * 1000 ))"'and my steps to full degrees. Contrary to the impression i get looking at I'll hope i find time to contribute to the docs to highlight that. 2) To me, this looks like an attempt to "scale the desired percentage of maxPwm to the configured pwm window". I think this is a bad idea, as you let the user explicitly state their desired step points (as i did), already including min and max values. And the results unfortunately leave the user-defined step boundaries, defying (my) user expectation. I commented it out for the next point. 3) I am looking at iterations of the PidLoop to understand what it does, but once at 129 (where it shouldn't be), it does not step down to 128 . indicating that there is an issue with small distances between current and desired. I'll continue investigating, and already thanks for even writing this software. Abandoning Windows, i had to leave FanControl behind and your SW is the closest to it there is :) |
|
I added a debug line in LinearSpeedCurve.Evaluate and observe behaviour at idle temps. See |
|
EDIT: i have probably misunderstood the purpose of this, so ignore critic undertone. I have added constructive comments in PR. To demonstrate that the "scaling-to-min-max" adjustment of observation 2) is wrong, here is a log with it turned on, resulting in speeds just go up: |
jwefers
changed the title
jwefers
changed the title
|
Hey @jwefers , sorry for not getting back to you sooner. First of all: Thank you for the time and energy you already invested into this. There are definitely things about the current logic that are not optimal. I would also love to improve this, as the whole PID thing has caused issues and confusion in the past, but unfortunately I do not have the time to deep dive into this. I wasn't even able to read all of your comments yet, I will try to get a read this week. About the linear approach: Yes, this is much simpler, and possibly the right approach, however we have to be careful about changing such integral behavior at this point, as it might break existing users fan control. Dealing with this probably requires implementing some kind of system to let the user choose between different approaches (pid vs. linear) to be able to default to the old one. This is the biggest reason why I wasn't able to "just merge" your PR. When coming up with the PID loop logic, the main goal I had in mind was proving a flexible way to let the user specify how fan2go should approach its internally computed target pwm, with a default that provides a reasonably fast but also "smooth" behavior. I have since then had my doubts whether a PID loop was the right choice for this, especially since there are currently two separate PID loops acting at the same time. In hindsight, I think one of them is a bad idea (which might be what you are also saying here), but changing it requires a lot of care, which means time, which I didn't have (and still don't have much of). But maybe we can figure something out through a discussion here and within the PR 🤞 , so the first step is for me to read up on it. There is a lot to unpack here so I will followup with a comment later, but feel free to share you thoughts in the meantime, if you want. Thx. |
|
Hi Markus, thanks for replying. I agree with everything you said, and in fact continued my PR on my machine, introducing the linear approach as a configurable alternative to the PID default. Prob. same as you, life hit and i left it alone, did not get around to game anyway. Let me try to push my work-in-progress on this within the next days/weeks. At current speed, within a year i'll have a fully fleshed configurable new feature, that does not break any existing setups 😆 |
|
Note:
Yes. This is also documented in the README. I can understand the confusion though, its not consistent everywhere. Changing this would also be a breaking change though, so I am not sure how we could improve this. Maybe adding an optional unit parameter?
Same thing here, changing it would be a breaking change, but for the second we have at least full control, so adding a unit directly to the value would be an option (something like
I am not sure if the current implementation does what I want, but to explain what I tried to achieve with it: All curves are expected to return a value in [0..255] when calling fan2go/internal/curves/curve.go Line 13 in e197de0 Since the user has defined hard limits on min and max values for the same [0..255] range, this code is suppsed to map the [0..255] range the curve operates on to the [minPwm..maxPwm] range specified by the user for the relevant fan. This is supposed to ensure that the target pwm is always within the user defined [minPwm..maxPwm] bounds, but that obviously doesn't seem to be working correctly, and maybe the approach is not the best.
Thats not totally unexpected since thats how PID loops operate. The mapping mentioned above is supped to ensure the actual fan pwm is still within min/max bounds. Having pid loops for both the curve as well as the fan controller makes the whole system pretty hard to follow though, but I don't think many people are using a PID based curve either. In the worst case, there are multiple things adding delay:
All of them have the same goal: smoothing out the fan speed transition. This might be too much on too many levels. I am not sure what is best to keep though. The sensor averaging is useful for removing random outliers, but since it can be smoothed out lateron too I think this can be removed. |
|
Just to give an update on this issue: I have made a lot of adaptations and improvements, including a boatload of tests in #267 which will hopefully be merged relatively soon. This should fix the issues of the set boundaries beeing exceeded as well as the control logic failing to approach the target. |
Describe the bug
I set a config including a 'cpu' sensor and a 'gpu' sensor, reading from hwmon and nvidia-smi respectively. The sensors work fine apparently,
fan2go sensor -i cpu|gpuspit out the expected temps, without any newline though.I set linear curves for two groups of fans, depending on these sensors:
and finally associated them to the fans:
running as
sudo fan2go --verbose, i can observe:Expected behavior
fans adhere to curves and temp changes
Screenshots
Desktop (please complete the following information):
Distro: Arch Linux
uname -a: Linux endeavour-desktop 6.6.2-arch1-1 SMP PREEMPT_DYNAMIC x86_64 GNU/Linuxsensors -v: sensors version 3.6.0+git with libsensors version 3.6.0+gitfan2go version: 0.8.1Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: