combine-sinks (1)

Line range hint 8-9: Replace hardcoded user ID with dynamic user detection.

The environment variables use a hardcoded user ID (1000), while the script later dynamically detects the user with OVOS_USER. This inconsistency could cause issues on systems where the first non-system user has a different ID.

Apply this diff to use the dynamically detected user ID:

-export PULSE_RUNTIME_PATH="/run/user/1000/pulse/"
-export XDG_RUNTIME_DIR="/run/user/1000/"
+export PULSE_RUNTIME_PATH="/run/user/$(id -u "$OVOS_USER")/pulse/"
+export XDG_RUNTIME_DIR="/run/user/$(id -u "$OVOS_USER")/"

combine-sinks (2)

91-99: Make sleep duration configurable and implement PipeWire auto_null check.

The function uses a hardcoded sleep duration and lacks proper PipeWire implementation.

Apply this diff to improve the implementation:

+# Constants
+WAIT_DURATION=3  # Configurable sleep duration in seconds
+
 wait_for_auto_null() {
     if [ "$SOUND_SERVER" == "pulse" ] && pactl list short sinks | grep -q "auto_null"; then
         log_message "auto_null sink exists, still booting? Sleeping for 3 seconds..."
-        sleep 3
+        sleep "$WAIT_DURATION"
     elif [ "$SOUND_SERVER" == "pipewire" ]; then
-        echo "TODO"
-        sleep 3
+        if wpctl status | grep -q "auto_null"; then
+            log_message "auto_null sink exists in PipeWire, still booting? Sleeping for $WAIT_DURATION seconds..."
+            sleep "$WAIT_DURATION"
+        fi
     fi
 }

---

Line range hint 1-174: Consider implementing sound server abstraction.

While the script's modularity has improved, the sound server-specific logic is scattered throughout the code. Consider implementing a more structured approach:

1. Create separate functions for each sound server operation
2. Use a configuration file for system-specific settings
3. Implement proper service detection and fallback mechanisms

Example structure:

# Sound server interface functions
pulse_get_sinks() { ... }
pipewire_get_sinks() { ... }
pulse_create_combined_sink() { ... }
pipewire_create_combined_sink() { ... }

# Generic interface
get_sinks() {
    case "$SOUND_SERVER" in
        pulse) pulse_get_sinks ;;
        pipewire) pipewire_get_sinks ;;
        *) return 1 ;;
    esac
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

combine-sinks (1)

Line range hint 8-9: Use OVOS_USER variable for environment paths.

The environment variables are hardcoded with user ID 1000, which might not match the actual user. Consider using the OVOS_USER variable consistently.

Apply this diff to make the paths dynamic:

-export PULSE_RUNTIME_PATH="/run/user/1000/pulse/"
-export XDG_RUNTIME_DIR="/run/user/1000/"
+export PULSE_RUNTIME_PATH="/run/user/$(id -u "$OVOS_USER")/pulse/"
+export XDG_RUNTIME_DIR="/run/user/$(id -u "$OVOS_USER")/"

combine-sinks (3)

19-33: 🛠️ Refactor suggestion

Enhance sound server detection reliability.

The current detection only checks if commands exist but doesn't verify if the services are actually running.

Apply this diff to improve detection reliability:

 detect_sound_server() {
     # Check if PipeWire is installed
-    if command -v pipewire > /dev/null; then
+    if pgrep -x pipewire >/dev/null; then
         echo "pipewire"
     # Check if PulseAudio is installed
-    elif command -v pulseaudio > /dev/null; then
+    elif pgrep -x pulseaudio >/dev/null; then
         echo "pulse"
     # Check if ALSA is available
     elif command -v aplay > /dev/null && command -v amixer > /dev/null; then
         echo "alsa"
     else
         echo "No sound server detected"
         exit 1
     fi
 }

---

68-75: ⚠️ Potential issue

Implement PipeWire sink detection.

The get_sinks function returns "TODO" for PipeWire, which will break the sink combination logic.

Apply this diff to add PipeWire support:

 get_sinks() {
     if [ "$SOUND_SERVER" == "pulse" ]; then
         pactl list short sinks | awk '{print $2}' | grep -v 'auto_combined' | grep -v 'auto_null' | tr '\n' ',' | sed 's/,$//'
     elif [ "$SOUND_SERVER" == "pipewire" ]; then
-        echo "TODO"
+        wpctl status | grep -A1 "Sinks:" | tail -n1 | awk '{print $2}' | tr '\n' ',' | sed 's/,$//'
     fi
 }

---

149-151: ⚠️ Potential issue

Implement PipeWire combined sink creation.

The PipeWire implementation for creating combined sinks is missing, which makes the script non-functional for PipeWire users.

Apply this diff to add PipeWire support:

     elif [ "$SOUND_SERVER" == "pipewire" ]; then
-        echo "TODO"
+        # Remove existing combined sink if present
+        wpctl destroy auto_combined 2>/dev/null || true
+        
+        # Create new combined sink
+        if ! wpctl create-sink auto_combined slaves="$SINKS"; then
+            log_message "Failed to create combined sink"
+            exit 1
+        fi
+        
+        # Set as default if creation successful
+        if wpctl status | grep -q "auto_combined"; then
+            wpctl set-default-sink auto_combined
+            log_message "Combined sink created with outputs: $SINKS"
+        else
+            log_message "Combined sink creation failed"
+            exit 1
+        fi
     fi

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro