TamtamHero / fw-fanctrl

A simple systemd service to better control Framework Laptop's fan(s)
BSD 3-Clause "New" or "Revised" License
178 stars 33 forks source link

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

Closed leopoldhub closed 3 months ago

leopoldhub commented 3 months ago
leopoldhub commented 3 months ago

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

TamtamHero commented 3 months ago

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 commented 3 months ago

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

TamtamHero commented 3 months ago

After a week running it, LGTM

dariov1988 commented 3 months ago

Hi @TamtamHero and @leopoldhub just to let you know that I install the latest version using AUR and I can confirm the issue https://github.com/TamtamHero/fw-fanctrl/issues/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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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