Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alsa-utils: restore default card state handling #8662

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpapavas
Copy link
Contributor

@dpapavas dpapavas commented Feb 25, 2024

This is a first iteration of the PR regarding ALSA card state handling discussed here. The preexisting behavior, was to either restore a custom, ad-hoc state to each sound card, as it was loaded, or to restore a state saved by the user, if one existed.

The latter functionality did not work (at least not reliably; see forum thread for details) and, given the nature of the issue (briefly the udev rule runs the soundconfig script which then forks a background shell to do its work, but according to udev's manpage, any such background processes are killed once the main action finishes, so that the background shell is killed after doing whatever it managed to do, until the main script finished, which happens immediately), thefore I assume that the card resetting part of soundconfig, i.e. what it tries to do if no saved state file exists, probably doesn't work correctly either.

This PR restores the default functionality shipped with alsa-libs. Briefly this uses the alsa-restore systemd service to save the state on shutdown and restores it on next boot. The effect therefore is that the state is retained across boots. The user is then responsible for setting the mixer to the desired values, which are then retained.

This behavior works well on my setup and use-case. Nevertheless, assuming the existing functionality did work, to some extent at least and further assuming that the ad-hoc state set by the soundconfig script was in fact necessary in certain cases (where presumably the dafault state restored by ALSA on first encounter of the card, i.e. when no previously saved state exists, was not "correct" in some way), such cases may need to be fixed.

The PR is created against the 11.0 branch, as that is what I'm using. It can be rebased to other branches as needed.

@heitbaum heitbaum changed the base branch from libreelec-11.0 to master February 25, 2024 21:51
@heitbaum heitbaum changed the base branch from master to libreelec-11.0 February 25, 2024 21:52
@heitbaum
Copy link
Contributor

Hi @dpapavas - can you please rebase this against :master as the update will be to master.

@dpapavas dpapavas changed the base branch from libreelec-11.0 to master February 25, 2024 22:43
@dpapavas
Copy link
Contributor Author

Done. Note that I haven't tried a build of this, as I'm currently using LE 11 on my device, but I don't see why it would work any differently than the version I've tried. If required, I can try to build and install it on a separate card, in order to try it out.

Copy link
Contributor

@heitbaum heitbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial look at the change. it compiles - I havent tested.


# remove default udev rule to restore mixer configs, we install our own.
# so we avoid resetting our soundconfig
rm -rf ${INSTALL}/usr/lib/udev/rules.d/90-alsa-restore.rules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file has the following contents - which are not on the LE device

ACTION=="add", SUBSYSTEM=="sound", KERNEL=="controlC*", KERNELS!="card*", TEST=="/usr/sbin", TEST=="/usr/share/alsa", GO
TO="alsa_restore_go"
GOTO="alsa_restore_end"

LABEL="alsa_restore_go"
TEST!="/etc/alsa/state-daemon.conf", RUN+="/usr/sbin/alsactl restore $devnode"
TEST=="/etc/alsa/state-daemon.conf", RUN+="/usr/sbin/alsactl nrestore $devnode"

LABEL="alsa_restore_end"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is directed to me. If it is, I'm not sure if I understand. How are they not on the device?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing here - where is /etc/alsa/state-daemon.conf being created?

LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg$ find alsa-utils-1.2.11/
alsa-utils-1.2.11/
alsa-utils-1.2.11/.noinstall
alsa-utils-1.2.11/.noinstall/arecord
alsa-utils-1.2.11/.noinstall/aseqdump
alsa-utils-1.2.11/.noinstall/aplaymidi
alsa-utils-1.2.11/.noinstall/arecordmidi
alsa-utils-1.2.11/.noinstall/aseqnet
alsa-utils-1.2.11/.noinstall/aconnect
alsa-utils-1.2.11/.noinstall/amidi
alsa-utils-1.2.11/.libreelec-package
alsa-utils-1.2.11/usr
alsa-utils-1.2.11/usr/sbin
alsa-utils-1.2.11/usr/sbin/alsa-info.sh
alsa-utils-1.2.11/usr/sbin/alsactl
alsa-utils-1.2.11/usr/bin
alsa-utils-1.2.11/usr/bin/nhlt-dmic-info
alsa-utils-1.2.11/usr/bin/amixer
alsa-utils-1.2.11/usr/bin/alsaucm
alsa-utils-1.2.11/usr/bin/aplay
alsa-utils-1.2.11/usr/bin/speaker-test
alsa-utils-1.2.11/usr/bin/alsamixer
alsa-utils-1.2.11/usr/bin/alsatplg
alsa-utils-1.2.11/usr/bin/axfer
alsa-utils-1.2.11/usr/bin/iecset
alsa-utils-1.2.11/usr/lib
alsa-utils-1.2.11/usr/lib/systemd
alsa-utils-1.2.11/usr/lib/systemd/system
alsa-utils-1.2.11/usr/lib/systemd/system/alsa-state.service
alsa-utils-1.2.11/usr/lib/systemd/system/sound.target.wants
alsa-utils-1.2.11/usr/lib/systemd/system/sound.target.wants/alsa-state.service
alsa-utils-1.2.11/usr/lib/systemd/system/sound.target.wants/alsa-restore.service
alsa-utils-1.2.11/usr/lib/systemd/system/alsa-restore.service
alsa-utils-1.2.11/usr/lib/alsa-topology
alsa-utils-1.2.11/usr/lib/alsa-topology/libalsatplg_module_nhlt.la
alsa-utils-1.2.11/usr/lib/alsa-topology/libalsatplg_module_nhlt.so
alsa-utils-1.2.11/usr/lib/udev
alsa-utils-1.2.11/usr/lib/udev/rules.d
alsa-utils-1.2.11/usr/lib/udev/rules.d/90-alsa-restore.rules
alsa-utils-1.2.11/usr/share
alsa-utils-1.2.11/usr/share/sounds
alsa-utils-1.2.11/usr/share/sounds/alsa
alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Right.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Left.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Center.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Center.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Noise.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Side_Left.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Left.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Right.wav
alsa-utils-1.2.11/usr/share/sounds/alsa/Side_Right.wav
alsa-utils-1.2.11/usr/share/alsa
alsa-utils-1.2.11/usr/share/alsa/init
alsa-utils-1.2.11/usr/share/alsa/init/test
alsa-utils-1.2.11/usr/share/alsa/init/default
alsa-utils-1.2.11/usr/share/alsa/init/info
alsa-utils-1.2.11/usr/share/alsa/init/00main
alsa-utils-1.2.11/usr/share/alsa/init/hda
alsa-utils-1.2.11/usr/share/alsa/init/ca0106
alsa-utils-1.2.11/usr/share/alsa/init/help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't realize you were referring to state-daemon.conf. This file doesn't exist by default (note the conditionals testing for its existence in the udev rule, as well as in alsa-state.service). It is created by the user to switch state handling to daemon-mode. See here for more information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be addressed/adjusted /tuned with our planned use of alsa? (As previously this rules file was removed from the build). So RUN+="/usr/sbin/alsactl restore $devnode" is the command being run - no need for the TEST?

when does the udev get used, versus the systemd?

@@ -26,20 +26,8 @@ post_configure_target() {
}

post_makeinstall_target() {
rm -rf ${INSTALL}/lib ${INSTALL}/var
rm -rf ${INSTALL}/usr/share/alsa/speaker-test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this wasnt doing anything

@@ -26,20 +26,8 @@ post_configure_target() {
}

post_makeinstall_target() {
rm -rf ${INSTALL}/lib ${INSTALL}/var
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this wasnt doing anything

@@ -26,20 +26,8 @@ post_configure_target() {
}

post_makeinstall_target() {
rm -rf ${INSTALL}/lib ${INSTALL}/var
rm -rf ${INSTALL}/usr/share/alsa/speaker-test
rm -rf ${INSTALL}/usr/share/sounds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want these WAV files?

build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Right.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Left.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Rear_Center.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Center.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Noise.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Side_Left.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Left.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Front_Right.wav
build.LibreELEC-Generic.x86_64-12.0-devel/install_pkg/alsa-utils-1.2.11/usr/share/sounds/alsa/Side_Right.wav

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're used by speaker-test, which can be useful when testing your card. For instance, when trying to get the driver for my DAC board working, I had to look for and download wav files from the Internet in oder to test it. It would have been convenient, if they were already available.
The wav files are not absolutely necessary, as speaker-test can work without them, either by sending noise to the card, or user-specified wavs, but these files are the default ones used with the -t switch, so they're necessary if we don't want speaker-test to appear broken. They're also useful for testing speaker connections i.e. whether the left-right speakers haven't been connected the other way round etc.
So I would argue that they're useful as debugging tools that a desktop user might already be familiar with and expect to find. I don't see how they hurt either, except for taking up space. If using the least possible space is considered important, they can be skipped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferrable to include a single .wav sample (a reduced-in-size one if possible; doesn't need to be one from the original package) and then create symlinks that recreate the appearance of having the standard files, but avoiding the bloat of actually including them. It might sound daft, but that's the ethos of a minimalist distro :)

# SPDX-License-Identifier: GPL-2.0-or-later

d /storage/.cache/alsa 0755 root root - -
L /var/lib/alsa/ - - - - /storage/.cache/alsa
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do, but why? This approach, besides being simple enough, has the added benefit of retaining the standard setup as much as possible. A user familiar with ALSA on desktops for instance, needing to find the state file for whatever reason (say removing it, or trying to figure out why state is not retained), would know where to look for it. This would not be the case, if we reconfigured alsactl to place it directly inside the non-standard .cache/alsa location. Is there some other benefit to this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a look at prior art, e.g. openssh, where the configure option is applicable it is being used. There are not many links from / into .cache find . -xdev -type l | xargs ls -l | grep cache

@chewitt
Copy link
Member

chewitt commented Feb 26, 2024

If we embed alsamixer by default there also needs to be a change to remove it from the "Multimedia Tools" add-on.

@dpapavas
Copy link
Contributor Author

Sorry, I forgot I had skipped removal of some utilities etc. Regarding alsamixer, I restored it, because I found it useful and, having no idea that it was available via multimedia tools, had resorted to using amixer, which is cumbersome. We can remove it again, but it is a basic tool for setting up the sound card that I imagine many users might need to use and think is not available. Again, it depends on how important minimization of used space on the device is.

@chewitt
Copy link
Member

chewitt commented Feb 26, 2024

it depends on how important minimization of used space on the device is.

LE/OE has managed without alsamixer for 12+ years so I think we can safely continue without it; we do strive to minimise the growth of images and it's available from the tools add-on should people really need it.

@HiassofT
Copy link
Member

Some general comments:

When I sait the sound configuration should be redone in the forum I didn't mean to nuke everything and completely switch over to alsa state handling etc, but to try to move away from soundconfig, where possible, and let alsactl restore/init and ucm handle that.

In particular: we will have soundcards that can't be initialized by alsactl / UCM yet and for those we need to maintain a way to manually configure them. One such example is the Cirrus Logic Audio Card on RPi, the routing capabilities are far too complex to be mapped to UCM - see it's config scripts and (to-be-rewritten) udev initialization here https://github.com/LibreELEC/LibreELEC.tv/tree/master/packages/audio/rpi-cirrus-config

This can be easily done with udev, order the soundcard specific rules before 90-alsa-restore, let them set a marker property (eg ENV{.le_soundcard_initialized}=1 and then perform restore/init in 90-alsa-restore only if the marker isn't set. This means we should not ship alsa's default udev rules but we have to use our own.

We also don't need the alsa state daemon and I think it's better to not install the alsa systemd service to automatically store the current settings on shutdown/reboot as well. This is something users can manually do if they really want to make their configuration persistent.

@dpapavas
Copy link
Contributor Author

The desired direction was described in the forum as "completely nuking [the whole soundcard/soundconfig handling] and replacing it with the current ALSA default config/state handling", but from what I see I have misunderstood and this is neither feasible, nor desirable.

As far as I can see from the various comments, the current setup seems to suit us mostly fine. I understand there may be issues with some cards, but I cannot submit PRs for those, since I do not have the hardware in question. What I could do instead, is convert this PR to a simple change that would fix the current setup in the following respects:

  • Fix the soundconfig script, by removing the & backgrounding the subshell, since that doesn't work.
  • Fix the issues in the udev rule described in the forum thread (i.e. that invalid NAME action, adding perhaps SUBSYTEM="sound", etc.)

How does that sound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants