[PATCH] services: Add fancontrol-service-type

Open

Details

4 participants

Denis 'GNUtoo' Carikli
Juliana Sims
Adrien 'neox' Bourmault
Adrien 'neox' Bourmault

Owner: unassigned

Submitted by: Adrien 'neox' Bourmault

Severity: normal

Adrien 'neox' Bourmault wrote on 9 May 17:37 +0200

Recipients:(address . guix-patches@gnu.org)(name . Adrien 'neox' Bourmault)(address . neox@gnu.org)

Message-ID:20240509154032.5047-1-neox@gnu.org

Hi! I've created a fancontrol service for my own use on a KGPE-D16
workstation, and wanted to share it with GNU Guix. The configuration
has to be generated upstream with pwmconfig (lm-sensors package)
and you just have to tell the service where it is for it to work.

Change-Id: I120e54cbf849eebd088be2a4d0a0113ffcdfcd84
Signed-off-by: Adrien 'neox' Bourmault <neox@gnu.org>
---
 gnu/services/pm.scm | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Toggle diff (49 lines)diff --git a/gnu/services/pm.scm b/gnu/services/pm.scm
index 3daf484cc1..1b305e76a6 100644
--- a/gnu/services/pm.scm
+++ b/gnu/services/pm.scm
@@ -31,7 +31,9 @@ (define-module (gnu services pm)
             tlp-configuration
 
             thermald-configuration
-            thermald-service-type))
+            thermald-service-type
+            
+            fancontrol-service-type))
 
 (define (uglify-field-name field-name)
   (let ((str (symbol->string field-name)))
@@ -466,3 +468,31 @@ (define thermald-service-type
    (default-value (thermald-configuration))
    (description "Run thermald, a CPU frequency scaling service that helps
 prevent overheating.")))
+
+;;;
+;;; fancontrol
+;;;
+;;; This service implements fan control in conjunction with the tools in the
+;;; lm-sensors package (pwmconfig/fancontrol).
+
+(define (fancontrol-shepherd-service config)
+  (shepherd-service
+   (documentation "Run the fancontrol daemon (fancontrol-daemon)." )
+   (provision '(fancontrol))
+   (requirement '(udev user-processes))
+   (start #~(make-forkexec-constructor
+             (list #$(file-append lm-sensors "/sbin/fancontrol")
+                   #$config)
+             #:user "root" #:group "root"
+             #:log-file "/var/log/fancontrol.log"))
+   (stop #~(make-kill-destructor))))
+
+(define fancontrol-service-type
+  (service-type
+   (name 'fancontrol)
+   (description
+    "Run fancontrol as a daemon.")
+   (extensions
+    (list (service-extension shepherd-root-service-type
+            (compose list fancontrol-shepherd-service))))))
+
-- 
2.41.0

Denis 'GNUtoo' Carikli wrote on 9 May 19:39 +0200

Recipients:(address . 70845@debbugs.gnu.org)

Message-ID:20240509193942.20e6ec8e@primary_laptop

Hi,

Thanks a lot for the patch.

For the record I'm not a Guix maintainer / committer, and that I

didn't look at everything but I spotted something that could be

improved: the user has no way to override the lm-sensors package being

used.

Most services provide a way to do that kind of override, so it would

make sense to add that at some point.

Denis.

Adrien 'neox' Bourmault wrote on 9 May 21:28 +0200

Recipients:(address . 70845@debbugs.gnu.org)(name . Denis 'GNUtoo' Carikli)(address . GNUtoo@cyberdimension.org)

Message-ID:c299fc11cae331bb3a2b014c6e321390c84bafc9.camel@a-lec.org

Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit�:

Toggle quote (14 lines)> Hi,
> 
> Thanks a lot for the patch.
> 
> For the record I'm not a Guix maintainer / committer, and that I
> didn't look at everything but I spotted something that could be
> improved: the user has no way to override the lm-sensors package being
> used.
> 
> Most services provide a way to do that kind of override, so it would
> make sense to add that at some point.
> 
> Denis.

Hi and thanks for you feedback. Not every service I saw allows such override,
but it seems indeed a good idea. However, it would complicate a bit this patch
as it would require to create a proper configuration type for the service and I
also did not dig into that (did not have much time). Someone else might be able
to do it.

Happy hacking!
-- 
Adrien Bourmault
Maintainer, GNU Boot project
Associate member, Free Software Foundation
GPG : 393D4CC68136F39799DA75F295F65F55F682A17A

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQ5PUzGgTbzl5nadfKV9l9V9oKhegUCZj0jvQAKCRCV9l9V9oKh

emnAAQCKZsuPXxvAHUTyOasO6pPsABwyWiIk1oiyrxN5mK4i3wD+OlXilD4SYdjW

yV3V4L+9DyUYsbh+Zmz+EZtelQ5SQwk=

=OUk2

-----END PGP SIGNATURE-----

Adrien 'neox' Bourmault wrote on 9 May 21:30 +0200

Recipients:(address . 70845@debbugs.gnu.org)(name . Denis 'GNUtoo' Carikli)(address . GNUtoo@cyberdimension.org)

Message-ID:452c2d78e57c54b313decb40dbfeb59315e63131.camel@gnu.org

Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit�:

Toggle quote (14 lines)> Hi,
> 
> Thanks a lot for the patch.
> 
> For the record I'm not a Guix maintainer / committer, and that I
> didn't look at everything but I spotted something that could be
> improved: the user has no way to override the lm-sensors package being
> used.
> 
> Most services provide a way to do that kind of override, so it would
> make sense to add that at some point.
> 
> Denis.

Hi and thanks for you feedback. Not every service I saw allows such override,
but it seems indeed a good idea. However, it would complicate a bit this patch
as it would require to create a proper configuration type for the service and I
also did not dig into that (did not have much time). Someone else might be able
to do it.

Happy hacking!
-- 
Adrien Bourmault
Maintainer, GNU Boot project
Associate member, Free Software Foundation
GPG : 393D4CC68136F39799DA75F295F65F55F682A17A

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQ5PUzGgTbzl5nadfKV9l9V9oKhegUCZj0kQgAKCRCV9l9V9oKh

ei1/AP9xElgZxK2ktrKwcNtCS8z6vMU5qbgzlgxkInkfgJYX5gD/Vn7Vq3dAtRgU

6DTfn76VNp0b6ozu+Bvr2Nd4yRtrjg8=

=lBEY

-----END PGP SIGNATURE-----

Denis 'GNUtoo' Carikli wrote on 12 May 17:17 +0200

Recipients:(name . Adrien 'neox' Bourmault)(address . neox@gnu.org)(address . 70845@debbugs.gnu.org)

Message-ID:20240512171749.201e713a@primary_laptop

Hi again,

While looking at the patches I interacted with on issues.guix.gnu.org, I

noticed a small issue: the commit message summary lacks a dot at the

end.

Denis.

Adrien 'neox' Bourmault wrote on 13 May 16:06 +0200

[PATCH v2] services: Add fancontrol-service-type.

Recipients:(address . 70845@debbugs.gnu.org)(name . Adrien 'neox' Bourmault)(address . neox@gnu.org)

Message-ID:b8ea66dd0b5edd9e8d345a0beda3198049ab6231.1715609187.git.neox@gnu.org

Hi! I've created a fancontrol service for my own use on a KGPE-D16

workstation, and wanted to share it with GNU Guix. The configuration

has to be generated upstream with pwmconfig (lm-sensors package)

and you just have to tell the service where it is for it to work.

This is the second version of this patch, fixing the lack of copyright

notice and dot at the end of the commit message.

Change-Id: I120e54cbf849eebd088be2a4d0a0113ffcdfcd84

Signed-off-by: Adrien 'neox' Bourmault <neox@gnu.org>

---

gnu/services/pm.scm | 33 ++++++++++++++++++++++++++++++++-

1 file changed, 32 insertions(+), 1 deletion(-)

Toggle diff (58 lines)diff --git a/gnu/services/pm.scm b/gnu/services/pm.scm
index 3daf484cc1..2196673875 100644
--- a/gnu/services/pm.scm
+++ b/gnu/services/pm.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2024 Adrien 'neox' Bourmault <neox@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -31,7 +32,9 @@ (define-module (gnu services pm)
             tlp-configuration
 
             thermald-configuration
-            thermald-service-type))
+            thermald-service-type
+            
+            fancontrol-service-type))
 
 (define (uglify-field-name field-name)
   (let ((str (symbol->string field-name)))
@@ -466,3 +469,31 @@ (define thermald-service-type
    (default-value (thermald-configuration))
    (description "Run thermald, a CPU frequency scaling service that helps
 prevent overheating.")))
+
+;;;
+;;; fancontrol
+;;;
+;;; This service implements fan control in conjunction with the tools in the
+;;; lm-sensors package (pwmconfig/fancontrol).
+
+(define (fancontrol-shepherd-service config)
+  (shepherd-service
+   (documentation "Run the fancontrol daemon (fancontrol-daemon)." )
+   (provision '(fancontrol))
+   (requirement '(udev user-processes))
+   (start #~(make-forkexec-constructor
+             (list #$(file-append lm-sensors "/sbin/fancontrol")
+                   #$config)
+             #:user "root" #:group "root"
+             #:log-file "/var/log/fancontrol.log"))
+   (stop #~(make-kill-destructor))))
+
+(define fancontrol-service-type
+  (service-type
+   (name 'fancontrol)
+   (description
+    "Run fancontrol as a daemon.")
+   (extensions
+    (list (service-extension shepherd-root-service-type
+            (compose list fancontrol-shepherd-service))))))
+

base-commit: 7b0f145802f0c2c785014293d748721678fef824
-- 
2.41.0

Juliana Sims wrote on 16 May 00:16 +0200

Recipients:(address . neox@gnu.org)(address . 70845@debbugs.gnu.org)

Message-ID:DJSJDS.VLIJN2A399JX2@incana.org

Hi Adrien,

Thanks for this patch! It looks pretty good, though I do have a few

small bits of feedback.

First and foremost, this service needs documentation. Could you add

that as well? Speaking of documentation, the 'documentation' field of

your Shepherd service has an extraneous bit of whitespace.

Is it absolutely vital to use root for this service? Could you instead

create a user and usergroup with only the privileges required to run

fancontrol? You may need to do something with udev so that works. I'm

not sure exactly what privileges are required, but avoiding root seems

like a good idea.

That's the only real critique I have here. Consider the rest of this

email fun ideas rather than review per se :)

We had an out-of-band exchange about this patch that I'll summarize

here for the record. I echoed the sentiments of the reviewer who

suggested exposing the fancontrol package so that users could change

it. Your response was that the configuration is generated by pwmconfig

and therefore it wouldn't make sense to provide a Scheme interface to

configuration.

I don't know this package or how it works, but would it be possible for

this service to generate that config automatically when it's first

started? If the config is customizable about generation, you could then

expose various settings through the Guix service interface for users to

modify and rewrite the file for them. That would make using

define-configuration worthwhile for more than simply the ability to

change the package.

All that aside, you should be able to expose the package setting to

users without using define-configuration.

Best,

Juli

Adrien 'neox' Bourmault wrote on 16 May 11:27 +0200

Recipients:(address . 70845@debbugs.gnu.org)(name . Juliana Sims)(address . juli@incana.org)

Message-ID:3c931f339af9e0fbe0ea2110cb253e95a70d972e.camel@gnu.org

Le mercredi 15 mai 2024 à 18:16 -0400, Juliana Sims a écrit�:

Toggle quote (2 lines)

> Hi Adrien,

Hi Juliana and thanks a lot for your review. I'm now working on a v3 ;)

Toggle quote (4 lines)

> First and foremost, this service needs documentation. Could you add

> that as well? Speaking of documentation, the 'documentation' field of

> your Shepherd service has an extraneous bit of whitespace.

Ok, seems sensible to do, I'm working on it and fixing the whitespace issue.

Toggle quote (7 lines)

> Is it absolutely vital to use root for this service? Could you instead

> create a user and usergroup with only the privileges required to run

> fancontrol? You may need to do something with udev so that works. I'm

> not sure exactly what privileges are required, but avoiding root seems

> like a good idea.

I don't actually know if this is vital, but it was the only way to make it work

properly. I did not think about udev though, so I'll try to test things. I agree

that avoiding root is a good idea.

Toggle quote (8 lines)

> I don't know this package or how it works, but would it be possible for

> this service to generate that config automatically when it's first

> started? If the config is customizable about generation, you could then

> expose various settings through the Guix service interface for users to

> modify and rewrite the file for them. That would make using

> define-configuration worthwhile for more than simply the ability to

> change the package.

Okay, perhaps more details are needed then. The fancontrol software is made to
control the speed of the fans (CPU heatsinks for example) based on a
configuration that is proper to the fans and that depends on many physical
parameters that might include, sometimes, the environment where the computer is
used. This configuration has to be generated with the pmwconfig software, which
will ask questions to the users (for example "Should we check I2C bus?" or "Did
you hear a fan stopping?"). Since the configuration process is both critical and
interactive, it appears difficult to me to do that automatically. One more thing
is that users should recreate a configuration when the use environment has
changed.

Toggle quote (4 lines)

> All that aside, you should be able to expose the package setting to

> users without using define-configuration.

Ok, thanks for the information. I'll work on that.

Have a nice day everyone and happy hacking!

Adrien Bourmault

Maintainer, GNU Boot project

Associate member, Free Software Foundation

GPG : 393D4CC68136F39799DA75F295F65F55F682A17A

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQ5PUzGgTbzl5nadfKV9l9V9oKhegUCZkXRjQAKCRCV9l9V9oKh

ekNgAP9B9cPpTGsFdWDdto6YMMZBjH+rHtr+YgGLs5qBurUciwD7B20iSTpIvojB

on2hHvjkngazIGKvMcvt4Hy783LzigE=

=SMTk

-----END PGP SIGNATURE-----

Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 70845@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 70845

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date