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

Change config_to_json.py to Reconfigure Config Files (Fixes #10) #20

Merged
merged 2 commits into from
Oct 15, 2021

Conversation

JoshIles
Copy link
Contributor

These changes fix #10 after running the xml/Makefile with the transform_to_json argument. Tested on an HP EliteBook 8560p, which had the same issue before these changes. I didn't include all the reconfigured scripts as I didn't want to bloat this request and make it difficult to review, so you will have to do this and then push the updated config files yourself.

Original problem:

The logic in NBFC Linux that handles TemperatureThresholds differs to that of the original NBFC service:

  • The original service selects a TemperatureThreshold when the temperature exceeds its UpThreshold.
  • NBFC Linux will select the next upper TemperatureThreshold when the temperature exceeds the current UpThreshold.

We can bring the behaviour in line with the original service by making a small change to:

while (i < size - 1 && temperature >= my.thresholds.data[i].UpThreshold) ++i;

(by simply changing it to while (i < size - 1 && temperature >= my.thresholds.data[i+1].UpThreshold) ++i;)

However, I prefer the current logic of NBFC Linux, as the handling of UpThreshold is consistent with that of DownThreshold, and is more intuitive. With that said, it appears that the community sourced config files were not reconfigured to account for this, and as such, they currently do not behave the same way as they do in the original service. I have made changes to tools/config_to_json.py to account for this, and have used it to reconfigure the config file for my notebook, which I have included as an example (tested and works for all config files). In addition, I have added these details to the README so that new users can get up to speed quickly.

As for #10, several config files had a TemperatureThresholds entry where FanSpeed, UpThreshold, and DownThreshold were all 0. This entry was designed to keep the fans turned off to preserve power and make less noise when cooling is unnecessary. However, in NBFC Linux, when the UpThreshold of the current entry is 0, the queried temperature always exceeds it and so the FanSpeed of the next entry is selected instead. This is why the desired FanSpeed of 0 was never being set.

Solution:

The new logic in tools/config_to_json.py simply shifts each UpThreshold value down an entry (after sorting in ascending order, of course), and sets the highest UpThreshold to the CriticalTemperature value (a change to src/model_config.c was required to allow for this). From there, there is no next TemperatureThresholds entry to move to, and so NBFC Linux will simply enable critical mode if the temperature ever exceeds the CriticalTemperature value, and thus the desired behaviour is preserved.

With that said, I'd like to say thank you for the hard work on this project, I really do appreciate and prefer this lightweight implementation of NBFC :).

@JoshIles
Copy link
Contributor Author

Updated my addition to the README for clarity. I also added a new error check to src/model_config.c, to ensure that UpThreshold values are not 0, and the error message instructs users to consult the README to see how NBFC Linux differs to the original service.

@braph braph merged commit f94348e into nbfc-linux:main Oct 15, 2021
@jwhendy
Copy link

jwhendy commented Oct 15, 2021

Wow, killer work and much appreciated. Also, apologies if I missed this nuance, as the 0 entry, I believe, is explicitly called out in the original NBFC variants (as you mention).

I'd shifted back to NBFC-revive, but after digging around in some of the mono files... ick. This seems way cleaner and I'd love to transition here. I'll try this out and report back. Thanks again!

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

Successfully merging this pull request may close these issues.

Temp below lowest TemperatureThreshold but fan target > 0 anyway
3 participants