search

mcgillij / amdfan

Updated AMD Fan control utility forked from amdgpu-fan and updated.
https://mcgillij.dev/pages/amdfan.html
GNU General Public License v2.0
33 stars 7 forks source link

add better daemon support #37

Closed mazunki closed 1 month ago

mazunki commented 1 month ago

As I mentioned in issue #36 , I wanted to add support for running the service in the background. This PR does just that, and also adds some cleaner shutdown to the controller. I've added some better logging statements and better --help documentation.

Keep in mind the logger is using the same logger as it used to on the terminal, meaning the rich colours we see may be interpreted weirdly by some editors when viewing them. That said, I feel like if that's an issue for some people, that's the user's editor's issue, and not really our fault. If this is a problem, we may choose to add some configuration to it (as we might want to do with the verbosity, fwiw).

The main changes is that amdfan daemon by default now redirects the output to /var/log/amdfan.log. This can be reverted with --no-logfile or --stdout, or a manual logfile can be set with --logfile=.

After some discussion with some init-smart people, I decided that it's better to explicitly request the daemon to run in the background with --background. By default it will stay running in the foreground, as expected by most init systems.

Some commits include more details

Closes: #36

mcgillij commented 1 month ago

Looks good, I pushed up a couple minor changes to deal with the flake8 issues being reported: https://github.com/mcgillij/amdfan/actions/runs/10153195863/job/28076076158#step:5:12

There was a pretty significant bug though with the order of the parameters preventing any of the daemon commands from working.

image

So I just changed the order so it would start up.

mazunki commented 1 month ago

nice :)

anything else we should change before updating the version? maybe the min_speed thingie, or the QA issue we briefly talked about? once the version is released i'll bump the ebuild on gentoo, as the current version there is broken

mcgillij commented 1 month ago

I have a local branch started to cut a new release, but if you want to get anything else prior lemme know. Otherwise I'll push it up tonight if I have time, after work.

mazunki commented 1 month ago

I think that's fine. I kind of want to modify how we load the logger and add to the monitor output, so to make it more configurable (also including an option to test the config file before attempting to reload the service), but I think we can do that at another time

mcgillij commented 1 month ago

yeah being able to validate the config would be nice