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 #32 received at 76289 <at> debbugs.gnu.org (full text, mbox):
From: Roman Scherer <roman <at> burningswell.com> To: Roman Scherer <roman <at> burningswell.com> Cc: Ludovic Courtès <ludo <at> gnu.org>, 76289 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, efraim <at> flashner.co.il Subject: Re: [bug#76289] [PATCH 2/2] services: Add speakersafetyd service. Date: Sat, 15 Feb 2025 10:25:55 +0100
[Message part 1 (text/plain, inline)]
And I just send a v3, because I exported the wrong symbol.
Roman Scherer <roman <at> burningswell.com> writes:
> 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.