🐛 | PlayerMPD.prev/next crash at the end of the playlist

[hoffie]

Version

v3.5.2

Branch

future3/main

OS

Raspberry Pi OS Bookworm Lite - 32bit

Pi model

3B

Hardware

No response

What happened?

• Press the configured Next button when playing the last song of a playlist, or
• Press the configured Prev button when the last song of a playlist has played.

jukebox-daemon logs the below exception and stops reacting to music play requests until restarted.

Logs

Mar 15 22:32:56 phoniebox bash[739]: 15.03.2024 22:32:56 -  337:__init__.py        - jb.PlayerMPD         - Thread-2        - DEBUG    - Prev
Mar 15 22:32:56 phoniebox bash[739]: Exception in thread Thread-2:
Mar 15 22:32:56 phoniebox bash[739]: Traceback (most recent call last):
Mar 15 22:32:56 phoniebox bash[739]:   File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
Mar 15 22:32:57 phoniebox bash[739]:     self.run()
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/lgpio.py", line 554, in run
Mar 15 22:32:57 phoniebox bash[739]:     cb.func(chip, gpio, level, tick)
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/pins/lgpio.py", line 248, in _call_when_changed
Mar 15 22:32:57 phoniebox bash[739]:     super()._call_when_changed(ticks / 1000000000, level)
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/pins/local.py", line 111, in _call_when_changed
Mar 15 22:32:57 phoniebox bash[739]:     super()._call_when_changed(
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/pins/pi.py", line 618, in _call_when_changed
Mar 15 22:32:57 phoniebox bash[739]:     method(ticks, state)
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/input_devices.py", line 179, in _pin_changed
Mar 15 22:32:57 phoniebox bash[739]:     self._fire_events(ticks, bool(self._state_to_value(state)))
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/mixins.py", line 385, in _fire_events
Mar 15 22:32:57 phoniebox bash[739]:     self._fire_activated()
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/mixins.py", line 431, in _fire_activated
Mar 15 22:32:57 phoniebox bash[739]:     super()._fire_activated()
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/gpiozero/mixins.py", line 348, in _fire_activated
Mar 15 22:32:57 phoniebox bash[739]:     self.when_activated()
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/src/jukebox/components/playermpd/__init__.py", line 339, in prev
Mar 15 22:32:57 phoniebox bash[739]:     self.mpd_client.previous()
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 478, in mpd_command
Mar 15 22:32:57 phoniebox bash[739]:     return wrapper(self, name, args, callback)
Mar 15 22:32:57 phoniebox bash[739]:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 541, in _execute
Mar 15 22:32:57 phoniebox bash[739]:     return retval()
Mar 15 22:32:57 phoniebox bash[739]:            ^^^^^^^^
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 463, in command_callback
Mar 15 22:32:57 phoniebox bash[739]:     res = function(self, self._read_lines())
Mar 15 22:32:57 phoniebox bash[739]:           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 396, in _parse_nothing
Mar 15 22:32:57 phoniebox bash[739]:     for line in lines:
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 595, in _read_lines
Mar 15 22:32:57 phoniebox bash[739]:     line = self._read_line()
Mar 15 22:32:57 phoniebox bash[739]:            ^^^^^^^^^^^^^^^^^
Mar 15 22:32:57 phoniebox bash[739]:   File "/home/phoniebox/RPi-Jukebox-RFID/.venv/lib/python3.11/site-packages/mpd/base.py", line 584, in _read_line
Mar 15 22:32:57 phoniebox bash[739]:     raise CommandError(error)
Mar 15 22:32:57 phoniebox bash[739]: mpd.base.CommandError: [55@0] {previous} Not playing

Configuration

GPIO config

input_devices:
  Prev:
    type: Button
    kwargs:
      pin: 19
    actions:
      on_press:
        alias: prev_song

  Next:
    type: Button
    kwargs:
      pin: 26
    actions:
      on_press:
        alias: next_song

More info

A stop gap solution which works for me would be something like this:

diff --git a/src/jukebox/components/playermpd/__init__.py b/src/jukebox/components/playermpd/__init__.py
index 05dff82b..f34e479c 100644
--- a/src/jukebox/components/playermpd/__init__.py
+++ b/src/jukebox/components/playermpd/__init__.py
@@ -335,16 +335,22 @@ class PlayerMPD:
     @plugs.tag
     def prev(self):
         logger.debug("Prev")
-        with self.mpd_lock:
-            self.mpd_client.previous()
+        try:
+            with self.mpd_lock:
+                self.mpd_client.previous()
+        except mpd.base.CommandError:
+            logger.debug('Failed to go to previous song, ignoring')
         self.play_position_tracker.flush()
 
     @plugs.tag
     def next(self):
         """Play next track in current playlist"""
         logger.debug("Next")
-        with self.mpd_lock:
-            self.mpd_client.next()
+        try:
+            with self.mpd_lock:
+                self.mpd_client.next()
+        except mpd.base.CommandError:
+            logger.debug('Failed to go to next song, ignoring')
         self.play_position_tracker.flush()
 
     @plugs.tag

Obviously, it doesn't cover all cases and might be suboptimal. Maybe prev/next should rather wrap around?
I'm happy to submit a PR once it is clear how it should behave and where the fix should be located.

[pabera]

Good catch. Thanks!

Your solutions looks already good. It would basically not do anything once the end of the playlist has been reached.

Most Phonieboxes don't have a visual interface. So if a user tries to press to get to the next song and nothing happens, it's not intuitive nor accessible.

1. In case it's the last song, pressing "Next" would go back to the beginning of the playlist.
2. When it's the first song, pressing "Previous" would go to the end (last song) of the playlist

This behviour could be configurable, meaning an option in the julebox.yaml would either enable or disable this behaviour

Thoughts?

[s-martin]

I agree at the end going to start again with NEXT and vice versa would be the most intuitive. 👍

Making it configurable (other option "do nothing") would cover most cases and could be extended later, if we come up with more use cases in the future.

[AlvinSchiller]

Default behavior of v2 is "do nothing" until play is pressed or a card is swiped again.
So +1 for configurable option.

[hoffie]

Some corrections/additions:

• I initially wrote that prev() on the first song of a playlist is a problem. It isn't. It's problematic at the end of a playlist when MPD is stopped.
• I initially thought that the problem was only related to reaching the end of the playlist. However, the root cause is that MPD is stopped. This state can probably be reached through other ways as well (rpc, buttons, web UI) and should therefore be handled.

In essence, we need to be clear about the behavior in the following cases. I'm no longer sure if it makes sense to make all of these configurable?

• Action, when prev() is run on a stopped MPD. Sane actions: stop, rewind, previously played song. I'd argue for rewind as the static default.
• Action, when next() is run on a stopped MPD. Sane actions: stop, rewind, the song one after the last played song. I'd also argue for rewind as the static default.
• Action, when next() is run during playing the last song of a playlist. I guess this might make sense to be configurable as either stop or rewind.

Independent of those specified cases, prev() and next() should never crash the player thread (this is handled by the above Exception handling example).

That's what I've implemented here and what I'll submit as a PR soon: hoffie@903e8d1

#2324 might also be relevant in the considerations.

[varac]

@hoffie I encountered this bug as well, after using you fork and the hoffie-future-main branch it working fine again 👍

[varac]

So please create a PR, I can confirm your changes are working.

[s-martin]

#2326 has been merged to future3/develop