Beta test and vote: Notification queue

svrooij commented 3 years ago

Since 2.4.0-beta.2 we support a second implementation of notification queues #89

I added both so everybody can check them out. This issue described the differences and I'm curious to what version works best.

To just vote for an implementation, react with:

🚀 for PlayNotification
🎉 for PlayNotificationTwo by @theimo1221

But it's more helpful to reply and explain what you've found.

Pros and cons

The list of pros and cons might not be complete, please reply and I'll edit this issue

Pros and cons for PlayNotification

Pros:

Using calling thread to play notification
Added two public methods on SonosDevice GetState and RestoreState, to build your own app around getting and restoring state.
Better maintainable
Less complex
...

Cons:

The second call to PlayNotification returns immediately instead of waiting for the notification to actually play
...

Pros and cons for PlayNotificationTwo

Pros:

Returns only when the notification actually played
...

Cons:

Less maintainable (more code, that is also more complex)
...

Details

PlayNotification

This method uses the calling thread of the first PlayNotification call. The first call will return when all notifications have played. The calls while currently playing a notification will return when that notification is added to the array and will be played in the thread of the first call.

Each notification can still use the timeout (in seconds) to stop the playback if the event isn't received or if you're playing a stream.

This implementation uses about 120 lines of code and the complexity is moderate.

https://github.com/svrooij/node-sonos-ts/blob/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6/src/sonos-device.ts#L351-L469

https://github.com/svrooij/node-sonos-ts/blob/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6/src/sonos-device.ts#L548-L583

PlayNotificationTwo

This implementation uses several timers to disconnect the thread, but each call to PlayNotificationTwo returns when that specific notification has played.

This implementation uses about 250 lines of code and the complexity is high.

https://github.com/svrooij/node-sonos-ts/blob/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6/src/sonos-device.ts#L485-L497

https://github.com/svrooij/node-sonos-ts/blob/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6/src/sonos-device.ts#L1107-L1392

https://github.com/svrooij/node-sonos-ts/blob/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6/src/models/notificationQueue.ts#L1-L93

hklages commented 3 years ago

I have already a solution in place (currently based on node-sonos):

group.create.snap creates as snapshot of the group (all player: volumes, mutestate; coordinator: playbackstate, avTransport, track, etc, and also the list of members)
group.play.snap, reverting everything to the snapshot
group.save.queue to save the current SONOS queue (which can be restored with mysonos.queue.item )

and I leave the queuing of notifications in the responsibility of the user. In Node-RED you can do that with a simple "Queue" node in front of a player. So init, refresh, reset, and the queuing itself of notification is in his responsibility :-)

Anyway: The issue with "notification queuing" is that usually there are several people in a household and they (can) use different apps to send commands to the SONOS player. So the system can be easily messed up.

Bottom line: PlayNotification would be my preferred solution as I might reuse getState/restoreStore methods and replace my group.create/play.snap.

svrooij commented 3 years ago

Closing stale issue, lets keep the new notification way in the code for now.

theimo1221 commented 3 years ago

@svrooij just as a short update from my side:

I retried using your solution and can't run it in my enviroment due to the following:

System starts playing a weather report with TTS split in several pieces (to allow using cached versions for already spoken messages, e.g. "It won't rain today")
System doesn't recieve tts end, thus not starting the item in 2nd position
System will start speaking 2nd messages after the default 30 minutes

I can't set timeout because, I would need to set it considering the other items in queue. So in my case I'm kinda dependent on using the option specificTimeout from my variant.

Shall I take a look to implement the specificTimeout in your variant as well?

svrooij commented 3 years ago

I guess the PlayTtsTwo doesn't work correctly because I rushed it. This should be modified to use the send notification method.


public async PlayTTSTwo(options: PlayTtsOptions): Promise<boolean> {
    this.debug('PlayTTSTwo(%o)', options);

    const notificationOptions = await TtsHelper.TtsOptionsToNotification(options);

    return await this.PlayNotification(notificationOptions);
  }

I guess if you fix this method, the regular notification method wouldn't need any changes.

svrooij / node-sonos-ts