GNU bug report logs - #76289
[PATCH 0/2] Add speakersafetyd system service.

Previous Next

Full log

Message #14 received at 76289 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Roman Scherer <roman <at> burningswell.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 76289 <at> debbugs.gnu.org, efraim <at> flashner.co.il
Subject: Re: [bug#76289] [PATCH 2/2] services: Add speakersafetyd service.
Date: Sat, 15 Feb 2025 10:18:10 +0900

Hi Roman,

Roman Scherer <roman <at> burningswell.com> writes:

> * gnu/services/sound.scm (speakersafetyd-service-type) New variable.
> * doc/guix.texi: Document the speakersafetyd service.

Interesting!

> Change-Id: Ib8fa19b056a2036019ae7c199d81e1139664e951
> ---
>  doc/guix.texi          | 41 ++++++++++++++++++++++++++++
>  gnu/services/sound.scm | 61 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 101 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index bd66adf326..3b82df5196 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -26575,6 +26575,47 @@ Sound Services
>
>  @end defvar
>
> +@subsubheading Speaker Safety Daemon System Service
> +
> +@uref{https://github.com/AsahiLinux/speakersafetyd, Speaker Safety
> +Daemon} is a userspace daemon that implements an analogue of the Texas
> +Instruments Smart Amp speaker protection model.  It can be used to
> +protect the speakers on Apple Silicon devices.
> +
> +@defvar speakersafetyd-service-type
> +This is the type for the @code{speakersafetyd} system service, whose
> +value is a @command{speakersafetyd-configuration} record.
> +
> +@lisp
> +(service speakersafetyd-service-type)
> +@end lisp
> +
> +See below for details about @code{speakersafetyd-configuration}.
> +@end defvar
> +
> +@deftp {Data Type} speakersafetyd-configuration
> +Data type representing the configuration for @code{speakersafetyd-service}.
> +
> +@table @asis
> +@item @code{blackbox-path} (default: @code{"/var/lib/speakersafetyd/blackbox"})
> +The path to a directory to which "blackbox" files are written when the
> +speakers are getting too hot.  The blackbox files contain audio and
> +debug information which the developers of @code{speakersafetyd} might
> +ask for when reporting bugs.
> +
> +@item @code{config-path} (default: @code{(file-append speakersafetyd "/share/speakersafetyd")})
> +Path to the base directory as a G-expression (@pxref{G-Expressions})
> +that contains the configuration files of the speaker models.
> +
> +@item @code{max-reduction} (default: @code{7})
> +Maximum gain reduction before panicing, useful for debugging.
> +
> +@item @code{package} (default: @var{speakersafetyd})
> +The Speaker Safety Daemon package to use.
> +
> +@end table
> +@end deftp
> +
>  @node File Search Services
>  @subsection File Search Services
>
> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
> index 8ca7acd737..fb8a8d3d17 100644
> --- a/gnu/services/sound.scm
> +++ b/gnu/services/sound.scm
> @@ -35,6 +35,7 @@ (define-module (gnu services sound)
>    #:use-module (gnu packages audio)
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages pulseaudio)
> +  #:use-module (gnu packages rust-apps)
>    #:use-module (ice-9 match)
>    #:use-module (srfi srfi-1)
>    #:export (alsa-configuration
> @@ -56,7 +57,15 @@ (define-module (gnu services sound)
>              ladspa-configuration
>              ladspa-configuration?
>              ladspa-configuration-plugins
> -            ladspa-service-type))
> +            ladspa-service-type
> +
> +            speakersafetyd-configuration
> +            speakersafetyd-configuration-blackbox-path
> +            speakersafetyd-configuration-config-path
> +            speakersafetyd-configuration-max-reduction
> +            speakersafetyd-configuration-package
> +            speakersafetyd-configuration?
> +            speakersafetyd-service-type))
>
>  ;;; Commentary:
>  ;;;
> @@ -263,4 +272,54 @@ (define ladspa-service-type
>     (default-value (ladspa-configuration))
>     (description "Configure LADSPA plugins.")))
>
> +
> +;;;
> +;;; Speaker Safety Daemon
> +;;;
> +
> +(define-record-type* <speakersafetyd-configuration>
> +  speakersafetyd-configuration
> +  make-speakersafetyd-configuration
> +  speakersafetyd-configuration?
> +  (blackbox-path speakersafetyd-configuration-blackbox-path
> +                 (default "/var/lib/speakersafetyd/blackbox"))

Since these values are not serialized, we are free to name them the way
we like; so they should follow the GNU and Guix conventions, namely:

I'd use blackbox-directory.

> +  (config-path speakersafetyd-configuration-config-path
> +               (default (file-append speakersafetyd
> "/share/speakersafetyd")))

I'd use configuration-directory.

> +  (max-reduction speakersafetyd-configuration-max-reduction
> +                 (default 7))

I'd use maximum-gain-reduction

> +  (package speakersafetyd-configuration-package
> +           (default speakersafetyd)))

I'd use the more conventional
speakersafetyd-configuration-speakersafetyd (using the name of the
package as the field name).

Our related conventions here are roughly:

1. Prefer full name instead of abbreviation, especially in public APIs
2. Path always used to denote a multi-entries search path like 'PATH'
and friends.  Use 'file name', 'file' or 'directory' instead.

It's also more conventional to use the package name for the package
object field, so here I'd use
'speakersafetyd-configuration-speakersafetyd'.

Could you please also use 'define-configuration/no-serialization' from
(gnu services configuration) ?  It validates the type of each field and
produces useful user error messages in case these are incorrect.

> +(define (speakersafetyd-shepherd-service config)
> +  (let ((blackbox-path (speakersafetyd-configuration-blackbox-path config))
> +        (config-path (speakersafetyd-configuration-config-path config))
> +        (max-reduction (speakersafetyd-configuration-max-reduction config))
> +        (package (speakersafetyd-configuration-package config)))

nitpick: I'd unbox each value using match-record; which will make things a bit
more concise.

> +    (shepherd-service
> +     (documentation "Speaker saftey daemon")

s/saftey/safety/

> +     (provision '(speakersafetyd))
> +     (requirement '(udev))
> +     (start #~(make-forkexec-constructor
> +               (list #$(file-append package "/bin/speakersafetyd")
> +                     "--config-path" #$config-path
> +                     "--blackbox-path" #$blackbox-path
> +                     "--max-reduction" (number->string #$max-reduction))))
> +     (stop #~(make-kill-destructor)))))
> +
> +(define speakersafetyd-service-type
> +  (service-type
> +   (name 'speakersafetyd)
> +   (description "Speaker Saftey Daemon")
>
s/Saftey/Safety/ The project has a better description, which can be
adapted to something like "@command{speakersafetyd} is a user space
daemon that implements an analogue of the Texas Instruments Smart Amp
speaker protection model."

> +   (extensions
> +    (list (service-extension
> +           profile-service-type
> +           (compose list speakersafetyd-configuration-package))
> +          (service-extension
> +           shepherd-root-service-type
> +           (compose list speakersafetyd-shepherd-service))
> +          (service-extension
> +           udev-service-type
> +           (compose list speakersafetyd-configuration-package))))
> +   (default-value (speakersafetyd-configuration))))

Nitpick, but I'd order these from most critical to less critical
ordering, such that te root service type is extended first, then the
udev-service-type, then the profile.

The rest LGTM.  Could you please send a v2?

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.