Bug 8593 - Sync'd booms should temp unsync for alarms
: Sync'd booms should temp unsync for alarms
Status: RESOLVED WONTFIX
Product: Logitech Media Server
Classification: Unclassified
Component: Alarm
: unspecified
: All All
: P3 normal with 1 vote (vote)
: Future
Assigned To: Alan Young
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-27 14:59 UTC by James Richardson
Modified: 2010-03-08 04:42 UTC (History)
7 users (show)

See Also:
Category: Feature


Attachments
Semi-working attempt to treat synced players as one during an alarm (10.84 KB, patch)
2008-07-28 14:46 UTC, Max Spicer
Details | Diff
Temporarily unsync at start of alarm and restore at end (753 bytes, patch)
2008-08-07 15:14 UTC, Max Spicer
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Richardson 2008-06-27 14:59:53 UTC
When syncing 2 booms, and setting an alarm on 1, it does not set a alarm on the synced boom.
Comment 1 James Richardson 2008-06-27 15:03:22 UTC
However, when the alarm goes off, it plays on all synced devices.  I.E. the 2nd boom will start playing the alarm.

The problem is that you can not turn off the alarm from the 2nd boom, only the 1st boom that has the alarm associated with it.

While you can pause or stop the stream from playing, the alarm is still flashing on the 1st boom
Comment 2 Chris Owens 2008-07-14 10:31:10 UTC
Alan notes this might be related to the sync power on-power off setting, which he notes has other problems.

Do we want only the alarmed boom to play?

Right now snooze only affects the original player.  Synced player continue playing.

Should the Alarm setting be synced between synced alarms?

Alan notes this behavior will be different on the 7.3 new streaming.

cc'ing Dean in case he has a clear idea of what to do.
Comment 3 Chris Owens 2008-07-21 10:28:40 UTC
Dean notes he thinks that if players are synced they should act as one player.

Therefore, we need to enable turning off the alarm from the synced boom.

Assigning to QA to see what the behavior is with new streaming before we pursue that.
Comment 4 Max Spicer 2008-07-21 16:16:49 UTC
I doubt the new streaming will change this as the current subscription for alarm off events is per client.  Off the top of my head, the only way to handle this would be to subscribe to the relevant events for all clients and then manually check if a received event is for a client that is in the same sync group as the alarm client.  Shouldn't be too bad.  I'm happy to take this, but any input appreciated.
Comment 5 Max Spicer 2008-07-28 14:38:06 UTC
I've been looking at doing this and have got it so that synced players display the datetime screensaver with flashing bell icon and also receive the appropriate show briefly messages.  Additionally, pressing play/pause etc on the synced players stops/snoozes/etc the alarm as appropriate.  However, I've hit a problem with the way Slim::Player::Sync reports synced players.  Synced players are only reported as such if they are on, regardless of whether they are in the same sync group.  When receiving notifications and deciding if they should cause the alarm to stop, I need to look at whether the request's client is synced to the alarm client.  However, for a power event that turned the player off, by the time the notification is received, the client is no longer reported as being synced with the alarm client.  This means you can't stop an alarm by turning a synced player off.  There's probably also a few other similar situations that will get broken by this.

I'll attach a patch of what I've done so far on this.  This patch also modifies Slim::Player::Sync so the currentAlarm state is copied to a player when it syncs with another player.  This is useful as the display code will not cancel the datetime screensaver if there's a current alarm.  Without copying the current alarm state, the display code would have to know about synced players to do this.  Copying the state also makes the alarm sync code simpler.  An alternative might be to subscribe to sync notifications and do the copy within the alarm code.  I don't know if such notifications exist.

Help needed if this is to make 7.2!
Comment 6 Max Spicer 2008-07-28 14:46:22 UTC
Created attachment 3700 [details]
Semi-working attempt to treat synced players as one during an alarm
Comment 7 Chris Owens 2008-08-04 09:47:04 UTC
Mickey to talk to Dean about this bug.
Comment 8 Mickey Gee 2008-08-05 14:51:09 UTC
I took a poll within the SW engineering team. Everyone thinks that one Boom should control on its own alarm, regardless of whether it's sync'd with another Boom. Someone might want a plug-in that syncs a Boom's alarm with other Boom alarms, but one doesn't exist at this time.

Of course Dean still wants one Boom to control many Boom alarms as a default behavior, but given the feedback it's not going to be implemented as a standard feature.

Reassigning to Max to tie up any loose ends on the code for this.
Comment 9 Blackketter Dean 2008-08-05 16:32:58 UTC
I'm good with the consensus.  Let's make the alarm affect only the one boom.

Comment 10 Max Spicer 2008-08-06 01:05:10 UTC
So how should this be implemented?  It sounds like the player should temporarily remove itself from any sync group during an alarm, or is there an easier way?  I can foresee several pitfalls with trying to temporarily alter sync groups.
Comment 11 Max Spicer 2008-08-06 07:11:31 UTC
I think this would prove pretty complicated.  You'd probably have to temporarily unsync a player during an alarm.  However, at the moment it's not that easy to find which players are synced without importing a lot of sync code (I think), especially if the players could potentially be turned off but be syncing power state.  The problem is that the syncedWith query only returns players that are on, but any one of those players could turn on when you turn on the master.  Also, you'd have to worry about how to restore sync state in such a way that intermediate changes are preserved.  And this is before you consider the changing sync code that's on the horizon.

I could be wrong about a lot of this - I'm no sync guru!  It sounds like a real can of worms though.

I don't think I'm going to be able to do this one, I'm afraid.
Comment 12 Blackketter Dean 2008-08-06 13:25:28 UTC
Max:

Player.pm has an example of how temporary sync handling works in power().

Alan should probably comment, but the key commands appear to be just:

	Slim::Player::Sync::unsync($client, 1);
and
	Slim::Player::Sync::restoreSync($client);

Doesn't look _too_ complicated... :)

Comment 13 Max Spicer 2008-08-07 08:42:02 UTC
You guys think of everything.  ;-)  Will have a go at using that.
Comment 14 Max Spicer 2008-08-07 15:14:34 UTC
Created attachment 3760 [details]
Temporarily unsync at start of alarm and restore at end

This patch is all that's needed.  However, there's a catch.  When you call unsync, it stops the player and sends out a stop notification.  This notification causes the alarm to immediately stop.  I don't think there's a way of detecting that the notification came from the sync call, and therefore ignoring it.  Triode did experiment with always setting the request's source a while back, but the patch either wasn't committed or was on the new sync branch.  The problem is at Slim::Player::Sync 241.
Comment 15 Max Spicer 2008-08-08 00:53:13 UTC
Alan, do you have any comments on this?
Comment 16 Alan Young 2008-08-12 12:00:09 UTC
[apart from disagreeing with the idea that the alarm should only sound on one player in a sync-group ...]

This patch could be made to work by the simple expedient of adding another "dont_stop" parameter to Sync::unsync(). Then Alarm would not get the extra and unwanted stop notification.

But that probably would not solve the whole problem. A few lines later on in Alarm.pm, it calls '$client->execute(['power', 1])'. If a player has the Synchronize Power preference set, then it will come on at that time and much the same issue will ensue. 

I get the impression that the mechanism by which alarms are canceled by listening for stop notifications is somehow a bit fragile; such notifications might be issued as part of playing the track associated with the alarm, is something was already playing when the alarm fires, for example.
Comment 17 Max Spicer 2008-08-12 12:27:07 UTC
Thanks, Alan.  The main problem is that I can't identify that notifications have originated from the sync code.  Within the alarm code I explictly set the source of requests so I don't react to my own.  Unfortunately, I don't think there's a mechanism for doing this when using notifyFromArray.  The mechanism is indeed fragile, but it's the only one there is at the moment.  Triode experimented with a modification to the control code that automatically filled in the source of a request, which was a good thing, in my opinion.

I don't understand what you mean about the power(1) bit.  If we've unsynced, why would a synced player turn on?
Comment 18 Alan Young 2008-08-12 13:32:53 UTC
Because we have only temporarily unsynced. We still have the same syncgroup-id as other players. This enable the power-on-sync code to work and would cause it to work in this case. Also any players that reconnect would do a RestoreSync.

Probably the only safe option is to do a permanent unsync, and then a restore-sync later will be pretty difficult: if there are any other "on" players in the (old) sync-group, you need to sync with one of them (in which direction?), but if there are no other "on" players in the old sync-group, then you just need to reset the 'syncgroupid' for the client.

Even then, restore-sync seems problematic: (1) the alarm goes off and player-A in the bedroom starts playing, having unsynced from player-B in the kitchen which was off; (2) you get up and go into the kitchen and turn on the player and play something; (3) the alarm timeout goes off and the sync-group is reestablished causing behaviour that will certainly be unexpected in some circumstances. A real mess.

But not restoring the sync-group will certainly lead to bug reports. As will the lack of sync between kitchen and bedroom (in the scenario above) when you turn the Kitchen on.
Comment 19 Max Spicer 2008-08-13 07:08:44 UTC
Retargeting to 7.3 as per discussion with Alan: https://slimdevices.campfirenow.com/room/147609/transcript/message/78951744.

Current thinking is that the best way to do this is to make alarms sound on all synced players (provided they are on or have the sync power setting on) and make them fully controllable from there.  I think I can now move this forward using my old patch to this bug, but it's risky for 7.2.
Comment 20 James Richardson 2008-08-22 15:25:12 UTC
Why not just remove the "alarm'd player" from the Sync Group like we do when plugging in a Line In ? bug 8981

Currently, when the alarm is triggered in a sync'd group, you have to turn the alarm off on each player.
Comment 21 Alan Young 2008-08-22 22:58:06 UTC
Because it will take less than a week before we get the following bug report: New alarm functionality breaks synchronization. It used to work fine in SC 7.1.
Comment 22 Max Spicer 2008-08-23 02:10:37 UTC
And we can't just temporarily remove it during the alarm either - see earlier comments.
Comment 23 Alan Young 2008-09-04 23:52:46 UTC
Max, I'm assigning this to you for 7.3. Send it back if you are not happy with that. Alan.
Comment 24 Chris Owens 2008-11-12 14:04:44 UTC
Changed summary to reflect the current angle on this bug.
Comment 25 Jim McAtee 2008-12-08 16:09:46 UTC
Why not add a player level 'Synchronize Alarms' option, so you could have it either way?  The way it would work is that if players A, B, and C were in a sync group and you set an alarm on player A, whether or not the alarm would sound on B or C would be controlled by this option for each of those players.
Comment 26 Tom Wynne 2008-12-20 01:43:22 UTC
Not sure if it should be raised as a separate bug, but this is completely broken in 7.3 (ie. sync and alarms cannot be used together as the alarm sets off all synced players).  Synced players are ignoring the 'power on sync' settings altogether when it comes to alarms.

I'm sure it would be nice for some to have the option to wake players together, but this shouldn't be the default behaviour.

Sadly it seems to be getting pushed further and further to the back of the queue.  Is there some way to prioritise it or bring it forward?
Comment 27 Alan Young 2008-12-21 01:31:46 UTC
Separate bug 10406 has been opened.
Comment 28 Tom Wynne 2008-12-23 04:41:59 UTC
This appears to have been resolved in the 12/22 nightly build.  It may have been the fix to 10406 that did it, but not sure.
Comment 29 Alan Young 2008-12-23 04:44:20 UTC
I do not think that the original intent of this bug has been resolved, but the problem with off players automagically coming (half) on with alarms has been fixed.
Comment 30 Tom Wynne 2008-12-23 04:47:36 UTC
Brilliant - I can use sync and alarms together again.  Merry Christmas!  :-)
Comment 31 James Richardson 2009-01-08 09:38:20 UTC
Moving to future for now, further discussion is needed
Comment 32 Alan Young 2009-09-29 05:03:29 UTC
Temp unsync is not an option - it is just not part of the architecture.

Forced full unsync is a bad option - see comment #21.

The fix from bug 10406 covers most of the bad side-effects that (I believe) caused this bug to be opened in the first place.

The remaining issue is that the active alarm cannot managed (snoozed, cancelled) from anything other than the alarmed player. Someone can open another bug for that if they wish.

I am going to close this with WONTFIX.
Comment 33 Alan Young 2010-03-08 04:42:21 UTC
*** Bug 15850 has been marked as a duplicate of this bug. ***