Package: guix-patches;
Reported by: Roman Scherer <roman <at> burningswell.com>
Date: Fri, 14 Feb 2025 13:56:01 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #23 received at 76289 <at> debbugs.gnu.org (full text, mbox):
From: Roman Scherer <roman <at> burningswell.com> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: Roman Scherer <roman <at> burningswell.com>, 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:15:21 +0100
[Message part 1 (text/plain, inline)]
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 <maxim.cournoyer <at> gmail.com> writes:
> 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?
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.