Add resume to play_card

[votti]

Adds a resume flag to play_card to resume from position in case there is already playback information for a folder. This is important for audiobooks.

In case the resume fails (eg if the folder changed), a normal playback is done and the error logged.

Addreses issue #1945

[votti]

I was wondering, if the the annotation if a folder should be resumed or not shouldnt rather be a property of the folder instead of defined in the RPC linked to a card? I see that there is already a RESUME Status in playermpd but could it be that it is not used yet?

Maybe even a combination could make sense: allowing the play_card to have a resume argument with options True|False|None (default None). If set to True|False, this overwrites any property of the folder, giving the power to enforce resume or not for specific cards. In case of None, this resumes or not depending on the RESUME status (ON|OFF) of the folder.

[ghost]

I am trying to sharpen the use case a bit:

• Case 1: Play something (let's say an audiobook), pause and resume it all with the same card. We currently already have an option to toggle playing on repeating card swipes. This is done via the second swipe option.
• Case 2: Play an audiobook, interrupt it and play something different, the come back to the same spot where we left off.
• Case 3: Same as case 2, but allow to shut down the Box in between. The only different is that the resume information needs to saved to disc.

Case 2 and 3 is currently not implemented (bits and pieces are there). This is a feature that is available in 2.X and is planned for 3.X as well. But we were planning to rewrite the mpd interface module as there is a couple of issues with it. And then put this and a few other features in.

The way I read your comment, you would add to these cases to function to select on a per-card basis if the resume should actually be executed or not. Correct?

[votti]

Thanks for sharpening! I am going for Case 2 & 3. With my current implementation, these cases work now for me.

As I currently have it, it is configured in the the RPC call associated with a card if a folder is resumed. I.e. a card has an RPC command play_card associated with an argument resume=True. In such a case, a card would behave like Case 2 & 3.

What I am now also working on, is that the RESUME status of a folder honored. I.e. if RESUME=ON, it would behave like in Case 2&3 and attempt to resume when playing the folder. If RESUME=OFF not (i.e. the standard behaviour we currently have). The good thing about this would be, that we could add an RPC toogle_resume that switches the RESUME status. This could then be linked to a button/card/Web GUI button to set this option on a per folder basis.

Does this make sense?

[pabera]

@votti I am not entirely following your comment. We do not care about folders in V3 like we did in V2. It all comes down to the card and its assignment.. which can be a folder, an album or a single file. All of these possible options then would have to be supported by the "Resume" flag and especially for Option 3, this needs to be stored on disk.

[votti]

Ah thanks a lot for the pointer. I just was looking at the code and somehow assumed that play_card was the main function that needed to be supported. Eg for the second swipe functionality so far only this one works.

I actually did miss play_song and play_album and that they are not yet logged and persisted in the music_folder_status.json file.

So I guess the steps to go from here is to add status tracking for albums/songs first? (Does this already have an open issue?).

Would you think that in order to get a resume functionality merged it first needs to support all 3 things (songs, album, folder) or would it be OK as well if this would be folder specific first?

[pabera]

Would you think that in order to get a resume functionality merged it first needs to support all 3 things (songs, album, folder) or would it be OK as well if this would be folder specific first?

Yes! (sorry for the late answer :-))

[tutenchamun]

this is still in request state right ?

[DivineDominion]

I want to look into this feature. We very much need playback resume controls, otherwise the user (my grandmother :)) would have to listen to the same intros dozens of times 🤔 Second Swipe on future3 is also not implemented AFAICT, right? Or is it possible to use that in the config e.g. cards.yaml?

[pabera]

This is a great feature to have. But a few requirements will determine the way to solve the problem. Maybe we can first define the requirements in the format of "As a user, I want to ..." before we go heads down. Maybe we can review the architecture to solve this before someone spends time on it. Thoughts?

[DivineDominion]

Side note: I checked out v2.4, too, thinking that might "just solve it", but oh dear future3's RPC and config is much nicer :)

This PR is also probably not the best place to chat about what we (I) wish for.

Let's imagine the next step is to return the double swipe to pause/resume feature, so is there anything here to salvage that, you think? From my noob perspective it looks like the existing functions on future3/develop suffice, only the wiring in the RPC commands is missing?

[pabera]

I didn't quite remember which features had been implemented, there was a larger break since I last developed. If it's just to connect the existing feature to the RPC, the give it a try :-)

[Mannshoch]

If possible I would like to set the resume with a jump of 10 sec backwards. Like I could do in Antennapod

[hoffie]

I've just stumbled upon this PR after playing with code enhancements around this topic myself. My current code is here and has been tested for play_single for now: https://github.com/hoffie/RPi-Jukebox-RFID/commit/c4805cebb848e8aaafd69c43f497b09436e67169

• The code currently addresses the above mentioned cases 1, 2 and 3.
• I noticed that only the play_card method supports second swipe. play_card does not seem to be used out of the box in the current code base at all. I've added a comment regarding that and added tracking functionality to the more relevant play_single, play_album and play_folder methods.
• The tracking is implemented separately from playermpd in order to be able to re-use it in other/different implementations. It is currently implemented to handle mpd-specific data, but that could easily be made more generic.
• The tracking re-uses the existing status information from the mpd polling thread.
• The tracking only writes to SD card after some grace time or upon user interactions (e.g. stop/next) to keep SD wear low.
• The tracking does not act on a per-folder level (which would break for album/single or totally different players). Instead, it uses a play_target, which is basically a combination of method call (e.g. play_single) with its arguments (SomeSong.mp3). That makes it flexible and reliable.
• With this code in place, playermpd.status_file may become redundant (but I haven't removed it yet).

If there's any interest I'm happy to polish that commit further and submit it as a PR.

[s-martin]

@hoffie, sounds like a cool addition.

Right now we are also working on a multi player support #2164, maybe it’s worth considering.

[hoffie]

@s-martin What would be the best way forward? Should I wait for #2164 to be merged and rebase my code on that? Or should I rather submit a PR with my code right now?

[s-martin]

We should check with @Groovylein how far the Multiplayer PR is.

[pabera]

@s-martin What would be the best way forward? Should I wait for #2164 to be merged and rebase my code on that? Or should I rather submit a PR with my code right now?

@hoffie: Let's try to get this merged. We'll have to do an extra effort for #2164 anyways.

[pabera]

I've just stumbled upon this PR after playing with code enhancements around this topic myself. My current code is here and has been tested for play_single for now: hoffie@c4805ce

@hoffie I added a few comments to your code

[hoffie]

@pabera Thanks. I'll try to address your comments in my upcoming PR. I guess a PR will be easier to comment/review than a single commit (which can't change). Will probably get to it by Thursday.

(Just for reference and probably not worth commenting there again: dca9d1b3f635e99e30f67540425b6a502921be57 is what I currently use in "production" without issues, even for whole folders)

[hoffie]

I'll try to address your comments in my upcoming PR.

See #2331.

[s-martin]

Fixed a merge conflict.

There is a flake8 error, which lets the checks fail, see https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/8944194290/job/24570588323?pr=1946

[coveralls]

Pull Request Test Coverage Report for Build 8944306028

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 38.235%

---

Totals
Change from base Build 8775015840:	0.0%
Covered Lines:	494
Relevant Lines:	1292

---

💛 - Coveralls

[s-martin]

There was a question in the chat about the resume feature https://matrix.to/#/!keXiohOiVddsrZXwBu:gitter.im/$4AmKngehqk3CH9Uu1w1jpqL2rWNoZ-F7y4dRAy7cXGA?via=matrix.org&via=gitter.im&via=unsicher.net

Right now I'm not sure, if anything is missing so this PR could be merged? @votti @pabera

[votti]

I am using this "in production" for more than a year and by now have it enabled for almost any card. Really a useful feature in my view that I would not want to miss any more.

I think it should be straight forward to refactor this in the future to a more generalized play tracking mechanism (eg as implemented here: https://github.com/hoffie/RPi-Jukebox-RFID/commit/1ab43a53fbf726d8502643924fe93086d217a9f8 ). Thus I think it would be appropriate to merge now.

[s-martin]

Thanks for the feedback!

[plimptm]

I just got this branch working on a test pi, with the primary goal of providing basic audiobook resume functionality for my kids (ages 3-8).

• rpi zero w
• bookworm
• rc522 reader (with rpi-lgpio shim for bookworm)

For reference, here are two working cards.yml entries:

'2788200895':
  alias: play_card
  args:
    - <folder_name>
  kwargs:
    resume: true
'2788200896':
  alias: play_folder
  args:
    - <folder_name>
  kwargs:
    resume: true

From my observation, the only difference between the play_folder and play_card entries is the lack of second_swipe support for play_folder (as @pabera is working on in #2330). But the resume functionality introduced in this PR is already achieving its purpose when used with play_folder directly (I hope that is encouraging feedback!)

Since this is a relatively small code addition, I hope it can be merged without complicating the refactoring/removal of play_card #2330 too much.

[votti]

Something I noticed recently, is that if the kids leave the card on the device, with resume=True the song will sporadically stutter as the box triggers a resume. I am very much looking forward to have second_swipe fully supported as this would make it easy to prevent it.

[votti]

One workaround I implemented for myself was that I enabled play_folder to also support playing single files. Thus I can use play_cards (that ultivatively calls play_folders) for both single files and folders and the resume functionality also works for both: https://github.com/votti/RPi-Jukebox-RFID/commit/f8e9de729f6fab1028335118e119434de5815612

[coveralls]

Pull Request Test Coverage Report for Build 10222388596

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 38.14%

---

Totals
Change from base Build 9427235714:	0.0%
Covered Lines:	492
Relevant Lines:	1290

---

💛 - Coveralls