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

Add new MQTT Playlist topics #505

Open
cpinkham opened this Issue Mar 15, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@cpinkham
Copy link
Contributor

cpinkham commented Mar 15, 2019

Add the following new topic handlers in Playlist::MQTTHandler()
playlist/${PLAYLISTNAME}/start
playlist/${PLAYLISTNAME}/next
playlist/${PLAYLISTNAME}/prev
playlist/${PLAYLISTNAME}/repeat
playlist/${PLAYLISTNAME}/startPosition
playlist/${PLAYLISTNAME}/stop/now
playlist/${PLAYLISTNAME}/stop/graceful
playlist/${PLAYLISTNAME}/stop/afterloop
playlist/ALLPLAYLISTS/stop/now
playlist/ALLPLAYLISTS/stop/graceful
playlist/ALLPLAYLISTS/stop/afterloop

The following should eventually be deprecated:
playlist/name/set
playlist/repeat/set
playlist/sectionPosition/set

@ghormann

This comment has been minimized.

Copy link
Contributor

ghormann commented Mar 15, 2019

To go along with this.... Is there a spot where API documentation is being maintained? https://legacy.gitbook.com/@falcon-player seems a bit dated....

Greg.

@ghormann

This comment has been minimized.

Copy link
Contributor

ghormann commented Mar 16, 2019

While working on this, it occured to me that we don't have a clean divide between our "status" commands and our "set" ones. (It noticed when watching the debug logs and every "playlist" message published by FPP was also being consumed by Playlist::MQTTHandler since fpp subscribes to itself. )

We could leave it and just waste the bandwith, but the "correct" solution would be to have have a "status" and "set" level after the FALCON_TOPIC level. This would be a change to every topic. Instead of totally breaking backwards comparability, I'm proposing we keep the publish tags where they are, but change all the "set" tags to have a "set" level. Thus we would have

/set/playlist/ALLPLAYLIST/....
/set/playlist/${PLAYLISTNAME}/....

We would then only subscribe to the /set level, avoiding unnecessary network traffic (and cpu for checking) on the bounce back. This proposal would mean moving /event and /set to under the the /set branch as well to be clean.

Thoughts?

@cpinkham

This comment has been minimized.

Copy link
Contributor Author

cpinkham commented Mar 16, 2019

I'm on the fence, but would lean in this direction of having a 'set' level rather than having to have 'set' on the end of each topic. We can always respond to both for a short period of time and then in a future version (3.x later this year) drop support for any sets not under /set

I think that for simple devices handling one single piece of a tree, having set at the end makes more sense, but for our structure with fpp having lots of branches, having a set level makes more sense.

@ghormann

This comment has been minimized.

Copy link
Contributor

ghormann commented Mar 21, 2019

@cpinkham would appreciate a review. Does this patch seem like a reasonable approach given that multiple concurrent playlist are not currently supported?

@cpinkham

This comment has been minimized.

Copy link
Contributor Author

cpinkham commented Mar 21, 2019

I think the patch looks good. Are you going to modify Playlist::MQTTHandler() to handle the ALLPLAYLISTS or playlist name part of the topic? If not, we should probably leave all that out of the doc file for now until it is supported. I think we can go ahead and add support in the topic even if we don't have support in the player yet. That way topics won't have to change later when more users have tied into the MQTT support. I may be playing with some of this soon because I want to tie my FPP-based picture frame into my Home Assistant install so HA can either pause the playlist or turn off the monitor when I am not home.

@ghormann

This comment has been minimized.

Copy link
Contributor

ghormann commented Mar 21, 2019

Thanks for the review. @cpinkham

Based on my testing, MQTTHandler() will respond to ALLPLAYLISTS/... and $PLAYLIST/.... the same way. (See lines 1755 though 1781 in patch for ALLPLAYLISTS support. The playlist specific versions are right after so that we can provide different behavior in the future.

I would recommend also including PR #507 otherwise the /next /prev MQTT calls don't work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.