[bug#70845] [PATCH] services: Add fancontrol-service-type

[bug#70845] [PATCH] services: Add fancontrol-service-type

[Adrien 'neox' Bourmault]

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(-)

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

[bug#70845] [PATCH] services: Add fancontrol-service-type

[Denis 'GNUtoo' Carikli]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#70845] [PATCH] services: Add fancontrol-service-type

[Adrien 'neox' Bourmault via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit :
> 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









[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[bug#70845] [PATCH] services: Add fancontrol-service-type

[Adrien 'neox' Bourmault]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit :
> 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









[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[bug#70845] [PATCH] services: Add fancontrol-service-type

[Denis 'GNUtoo' Carikli]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Adrien 'neox' Bourmault]

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(-)

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

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

[Juliana Sims via Guix-patches via]

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

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

[Adrien 'neox' Bourmault]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

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

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

> 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.

> 
> 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.

> 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.

> 
> 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









[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]