Adds boot sound playback during system startup#99
Conversation
WalkthroughA boot sound feature was introduced by adding a new systemd service ( Changes
Sequence Diagram(s)sequenceDiagram
participant systemd
participant boot-sound.service
participant livecd-sound
participant sound_card
systemd->>boot-sound.service: Start after sysinit.target, before basic.target
boot-sound.service->>livecd-sound: Execute with --unmute
livecd-sound->>sound_card: Unmute and set volume on first valid card
livecd-sound->>sound_card: Play startup sound
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
configs/airootfs/etc/systemd/system/multi-user.target.wants/boot-sound.service (1)
1-1: Enable under the correct target
The PR goal is to play the boot sound early (beforebasic.target), yet this link lives undermulti-user.target.wants. Consider moving (or duplicating) it intobasic.target.wantsand removing it here to ensure proper boot ordering.configs/airootfs/usr/local/bin/livecd-sound (2)
163-174: Optimization with a potential edge caseThe change optimizes the sound initialization process by focusing on essential controls ("Master" and "PCM") for just the first valid sound card instead of applying numerous settings to all cards. This is a good performance improvement for boot time.
However, there's a potential issue: the function assumes the first numeric card ID is the correct one for audio playback. On systems with multiple sound cards, this might not select the card connected to speakers.
Consider adding a fallback mechanism if the first card doesn't produce audible output. For example:
unmute_all_cards() { # Set only the essential controls for sound playback for card in $(list_non_pcsp_cards); do if is_numeric "$card"; then unmute_and_set_level "$card" "Master" "80%" unmute_and_set_level "$card" "PCM" "80%" # Play sound immediately after minimal setup - play_on_card "$card" /usr/share/livecd-sounds/startup.wav & - break + if play_on_card "$card" /usr/share/livecd-sounds/startup.wav; then + # Successfully played on this card + break + fi + # If we reach here, try the next card fi done }
169-171: Consider adding error handling for sound playbackThe script plays a sound immediately but doesn't handle potential failures in the audio playback.
Add simple error handling to log issues if sound playback fails:
-# Play sound immediately after minimal setup -play_on_card "$card" /usr/share/livecd-sounds/startup.wav & +# Play sound immediately after minimal setup +if ! play_on_card "$card" /usr/share/livecd-sounds/startup.wav &> /dev/null &; then + systemd-cat -t "livecdsound" printf "Failed to play startup sound on card: %s\n" "$card" +fiscripts/build_iso.sh (1)
26-30: Add error checking for the boot sound configurationWhile the changes are good, consider adding error checking to verify that the boot sound components are properly set up.
Add verification similar to other checks in the script:
# Add this to your build_iso.sh before creating the symlink chmod +x configs/airootfs/usr/local/bin/livecd-sound # Change from multi-user.target.wants to basic.target.wants mkdir -p "$CONFIG_DIR/airootfs/etc/systemd/system/basic.target.wants" ln -sf /etc/systemd/system/boot-sound.service "$CONFIG_DIR/airootfs/etc/systemd/system/basic.target.wants/boot-sound.service" +# Verify boot sound setup +if [ ! -x "$CONFIG_DIR/airootfs/usr/local/bin/livecd-sound" ]; then + echo -e "${RED}Error: livecd-sound script is not executable${NC}" + exit 1 +fi + +if [ ! -f "$CONFIG_DIR/airootfs/etc/systemd/system/boot-sound.service" ]; then + echo -e "${RED}Error: boot-sound.service file is missing${NC}" + exit 1 +ficonfigs/airootfs/etc/systemd/system/boot-sound.service (1)
13-13: Consider a more precise sleep commandWhile the sleep command works, systemd provides a more precise timer unit that could be used instead.
You could replace the sleep command with a systematic delay:
-ExecStartPre=/usr/bin/sleep 0.5 +ExecStartPre=/bin/sh -c 'systemd-analyze log-level debug && echo "Waiting for sound hardware initialization..."'Or keep the sleep but add a descriptive message:
-ExecStartPre=/usr/bin/sleep 0.5 +ExecStartPre=/bin/sh -c '/usr/bin/sleep 0.5 && echo "Sound hardware initialization delay complete"'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
configs/airootfs/usr/share/livecd-sounds/startup.wavis excluded by!**/*.wav
📒 Files selected for processing (5)
configs/airootfs/etc/systemd/system/basic.target.wants/boot-sound.service(1 hunks)configs/airootfs/etc/systemd/system/boot-sound.service(1 hunks)configs/airootfs/etc/systemd/system/multi-user.target.wants/boot-sound.service(1 hunks)configs/airootfs/usr/local/bin/livecd-sound(1 hunks)scripts/build_iso.sh(1 hunks)
🔇 Additional comments (5)
configs/airootfs/usr/local/bin/livecd-sound (1)
170-170:Details
✅ Verification successful
Verify sound file path exists
The code references
/usr/share/livecd-sounds/startup.wav, but we should ensure this path exists and the file is available.
🏁 Script executed:
#!/bin/bash # Check if the sound file directory and startup.wav exist if [ ! -d "configs/airootfs/usr/share/livecd-sounds" ]; then echo "ERROR: Directory /usr/share/livecd-sounds does not exist in the build configuration" exit 1 fi if [ ! -f "configs/airootfs/usr/share/livecd-sounds/startup.wav" ]; then echo "ERROR: startup.wav sound file not found in the build configuration" echo "Please ensure the file is added at configs/airootfs/usr/share/livecd-sounds/startup.wav" exit 1 else echo "Found startup.wav sound file" # Check if it's a valid WAV file file "configs/airootfs/usr/share/livecd-sounds/startup.wav" fiLength of output: 350
No action needed: startup.wav is present
The directory
configs/airootfs/usr/share/livecd-soundsand the filestartup.wavboth exist in the build configuration, so the referenced path is valid.scripts/build_iso.sh (2)
26-26: Good addition of executable permissionSetting executable permissions on the livecd-sound script is essential for the boot-sound service to function correctly.
29-30: Appropriate target change for earlier boot soundChanging from multi-user.target.wants to basic.target.wants is appropriate for a boot sound service, as it will trigger the sound earlier in the boot process. This aligns well with the service's WantedBy=basic.target directive.
configs/airootfs/etc/systemd/system/boot-sound.service (2)
1-19: Well-structured service for early boot soundThe systemd service is well-designed for early boot sound playback:
- Proper dependency ordering ensures it runs after sound drivers are loaded but before basic.target
- DefaultDependencies=no allows it to start earlier in the boot process
- Pattern matching for sound devices is a good approach to handle various hardware configurations
- Timeout setting prevents hanging if there are issues
8-9: Robust sound card dependency handlingThe pattern matching approach for sound card devices is excellent, as it handles systems with different sound hardware configurations and ensures the service waits for sound drivers to be loaded without waiting for the full sound.target.
| @@ -0,0 +1 @@ | |||
| /etc/systemd/system/boot-sound.service No newline at end of file | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing service unit definition
This file appears to be only a symlink placeholder without the actual boot-sound.service unit file. The service cannot execute without a unit definition containing [Unit], [Service], and [Install] sections. Please add the full unit file at configs/airootfs/etc/systemd/system/boot-sound.service.
| @@ -0,0 +1 @@ | |||
| /etc/systemd/system/boot-sound.service No newline at end of file | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable boot-sound.service via a symlink instead of a plain file
Systemd expects entries in the *.wants directories to be symbolic links to the actual unit file. A plaintext file containing the path will be ignored. Replace this file with a symlink pointing to ../boot-sound.service.
Proposed fix (in your build script or overlay manifest):
# Remove the incorrect plaintext file
rm configs/airootfs/etc/systemd/system/basic.target.wants/boot-sound.service
# Create the correct relative symlink
ln -s ../boot-sound.service configs/airootfs/etc/systemd/system/basic.target.wants/boot-sound.service
1 participant
This change introduces a boot sound feature to provide auditory feedback when the system starts.
Key modifications include:
systemdservice (boot-sound.service) is created to manage and play the sound. This service is configured to run early in the boot process, after essential sound drivers are loaded but before the full desktop environment, ensuring timely audio feedback.livecd-soundscript is updated to unmute specific audio controls ("Master", "PCM") and play the startup sound (startup.wav) immediately after minimal sound card initialization.startup.wavsound file is added.build_iso.sh) is modified to include the new service inbasic.target.wantsand ensure thelivecd-soundscript has execute permissions.Summary by CodeRabbit