Subject: [bug#76289] [PATCH 2/2] services: Add speakersafetyd service.

From: Roman Scherer <[email-masked]>
Date: Sat, 15 Feb 2025 10:15:21 +0100

Hi Maxim,

thanks for your review. I hope I addressed your comments. Additionally:

- I used the configuration->documentation command to generate
  documentation from the configuration record

- I discovered match-record-lambda and use this instead of match-record

Could you have another look please?

Thank you, Roman.

Maxim Cournoyer <[email-masked]> writes:

> Hi Roman,
>
> Roman Scherer <[email-masked]> 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?