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

Rewrite of the fanctrl.py script to improve security, compatibility, modularity and maintainability #29

Merged
merged 13 commits into from
May 16, 2024

Conversation

leopoldhub
Copy link
Collaborator

  • Rewrite of the fanctrl.py script to improve security, compatibility, modularity and maintainability

  • Removing watchdog as a dependency.
    We now use real unix sockets to communicate with the running service

  • We now use the embedded ectool command to fetch temperatures

  • The sleeping behavior will now put the service in pause (keep the active strategy) and use the default computer fan management to avoid draining battery

  • On crash/stop, the service will reactivate the default computer fan management to avoid damaging hardware

…, modularity and maintainability

> Removing watchdog as a dependency.
We now use real unix sockets to communicate with the running service

> We now use the embedded ectool command to fetch temperatures

> The sleeping behavior will now put the service in pause (keep the active strategy) and use the default computer fan management to avoid draining battery

> On crash/stop, the service will reactivate the default computer fan management to avoid damaging hardware
@leopoldhub
Copy link
Collaborator Author

I kept existing commands the same as to avoid breaking existing systems

fanctrl.py Show resolved Hide resolved
fanctrl.py Outdated Show resolved Hide resolved
fanctrl.py Outdated Show resolved Hide resolved
fanctrl.py Show resolved Hide resolved
@TamtamHero
Copy link
Owner

Looks good to me now, though I'll run this branch for a few days before merging it
Thanks again for your hard work @leopoldhub !

@leopoldhub
Copy link
Collaborator Author

ok, I take back what I said about mixing the two temperature computing strategies.
It was a terrible idea... fans can go up pretty quickly and drop dead seconds after.
To mitigate this, I have to decrease the importance of the current temp in the ratio so much that I could pretty much delete it entirely.
I will put back the original strategy

fanctrl.py Outdated Show resolved Hide resolved
@TamtamHero
Copy link
Owner

After a week running it, LGTM

@dariov1988
Copy link

Hi @TamtamHero and @leopoldhub just to let you know that I install the latest version using AUR and I can confirm the issue #35. If you try to start the tool using systemctl you get:

❯ sudo systemctl start fw-fanctrl.service
❯ sudo systemctl status fw-fanctrl.service
× fw-fanctrl.service - FrameWork Fan Controller
     Loaded: loaded (/usr/lib/systemd/system/fw-fanctrl.service; disabled; preset: disabled)
     Active: failed (Result: exit-code) since Thu 2024-05-16 21:15:53 EDT; 4s ago
   Duration: 27ms
    Process: 2913 ExecStart=/usr/bin/python3 /usr/bin/fw-fanctrl --config /etc/fw-fanctrl/config.json --no>
    Process: 2914 ExecStopPost=/usr/bin/ectool --interface=lpc autofanctrl (code=exited, status=0/SUCCESS)
   Main PID: 2913 (code=exited, status=1/FAILURE)
        CPU: 32ms

May 16 21:15:53 silver systemd[1]: fw-fanctrl.service: Scheduled restart job, restart counter is at 5.
May 16 21:15:53 silver systemd[1]: fw-fanctrl.service: Start request repeated too quickly.
May 16 21:15:53 silver systemd[1]: fw-fanctrl.service: Failed with result 'exit-code'.
May 16 21:15:53 silver systemd[1]: Failed to start FrameWork Fan Controller.
...skipping...

And if I try run manually the tool to see the output like the previos version I get the follow:

sudo /usr/bin/python3 /usr/bin/fw-fanctrl --config /etc/fw-fanctrl/config.json
Traceback (most recent call last):
  File "/usr/bin/fw-fanctrl", line 320, in <module>
    main()
  File "/usr/bin/fw-fanctrl", line 301, in main
    client_socket.connect(COMMANDS_SOCKET_FILE_PATH)
FileNotFoundError: [Errno 2] No such file or directory

I hope this can help. Thanks!

@leopoldhub
Copy link
Collaborator Author

leopoldhub commented May 17, 2024

Hi @dariov1988 ,

there is no such thing as /etc/fw-fanctrl/config.json at the moment (it will change in #34 ), the configuration is located here instead: ~/.config/fw-fanctrl/config.json.
also, the AUR version is not uploaded by us, so we cannot guarantee it will work, please use the install.sh script for the time being (if you have problems with pip, use the --no-requirements argument).

@dariov1988
Copy link

Hi @leopoldhub, thanks for answer. The fw-fanctrl-git package is pointing to this git repository. I check when install that checkout e97f4b3 so is up to date. Your assumption that I don't have the config in etc/fw-fanctrl/config.json is not true the file exist there. The --config parameter was deprecated? But nerveless I will do the manual install and let you know...

@leopoldhub
Copy link
Collaborator Author

My bad, it seems that the AUR package created it without using the install script.
The issue seems related to the COMMANDS_SOCKET_FILE_PATH (/etc/fw-fanctrl/.fw-fanctrl.commands.sock) which wasn't created as the fw-fanctrl service didn't start correctly.
as said in #35 , please use the script while waiting for the AUR maintainer to update the package.
Have a nice day

@dariov1988
Copy link

Hi @leopoldhub, thanks for your quick response. I was reading the install script and I figured out that you have a binary in your repository (ectool) redistribute packaged by yourself binaries from other people is not good for several reasons (not only security). The previous AUR approach was nice, not only because they update it automatically, and I do not have to remember from time to time to go to my local git repo, pull, and install. They have a dependency fw-ectool-git and pull it directly from their source, so all these benefits apply to this dependency too. The original author of ectool has a pipeline that built the tool himself https://gitlab.howett.net/DHowett/ectool/-/artifacts. If you want to continue providing the binary for an easy install user experience could you please evaluate the possibility of using the latest artifact provided by the original author? Thanks again. Have a nice day...

@leopoldhub
Copy link
Collaborator Author

Hi @dariov1988

Yes, you are right I will work on this as soon as I have time for it.

Thank you for making this issue known to us

@leopoldhub leopoldhub mentioned this pull request May 22, 2024
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.

None yet

3 participants