Logical Device Support

[stephen]

This is not ready for merge!

Implements logical devices with volume support.

While this works, it's incredibly slow, considering the number of callbacks/network requests at work. To get around this, we'll likely need to maintain client-side state for volumes and use event listeners to keep up to date.

[bencevans]

Awesome! 😸

Defo need to keep state. There's a way of using NOTIFY requests to get Sonos to ping webhooks on changes etc. but haven't been able to look into it a huge amount

[stephen]

@bencevans, @mrose17 - some things that probably need to be thought about:

• Where to keep state? Currently, I'm using a state property on the Sonos object (https://github.com/stephen/node-sonos/blob/logical-devices/lib/sonos.js#L69).
• I think it makes sense to have the Sonos device implement EventEmitter, so that we can do things like:

device.on('volumeChange', function() { ... });

• Currently, the implementation require a call to initialize() to set up event hook handlers. Should we require this moving forwards? i.e. usage now requires:

var device = new Sonos(...);
device.initialize(function() {
  // whatever code
});

This would also impact how things like the current getVolume() works, i.e. should it return the cached state volume (which requires initialize() to have happened), or do an entire request to get it as currently implemented, or should it selectively do one/the other based on whether or not startListeners() is called.

I'm partial to the first solution, though we would consider it a breaking change to how the API works. We can also have search() automatically call initialize() before returning callbacks to make things smoother.

Edit: It occurs to me the above notes are all about adding better eventing support, but that's necessary to get logical device support working well.

[stephen]

Up to this point - logical devices currently work for volume changing (example in examples/logicalDeviceVolume.js), but impl may need review on the above issues.

Before merge, we should also implement a logicalSearch() or similar to return logical grouping in the same way that search() does.

Tricky thing about this is how to know how long to wait before grouping things... i.e. since current search emits events for each new device to crop up, logicalSearch() would need to wait until all devices are found before grouping anything. Not sure how to do that apart from doing a timeout.

Perhaps an alternative is to get a single device, and getZoneTopology() from there? Since any given device should have consensus on who the other devices are...

[bencevans]

@bencevans, @mrose17 - some things that probably need to be thought about:

Where to keep state? Currently, I'm using a state property on the Sonos object (https://github.com/stephen/node-sonos/blob/logical-devices/lib/sonos.js#L69).

I'm happy with that.

I think it makes sense to have the Sonos device implement EventEmitter, so that we can do things like:

device.on('volumeChange', function() { ... });
Currently, the implementation require a call to initialize() to set up event hook handlers. Should we require this moving forwards? i.e. usage now requires:
var device = new Sonos(...);
device.initialize(function() {
// whatever code
});
This would also impact how things like the current getVolume() works, i.e. should it return the cached state volume (which requires initialize() to have happened), or do an entire request to get it as currently implemented, or should it selectively do one/the other based on whether or not startListeners() is called.

I'm partial to the first solution, though we would consider it a breaking change to how the API works. We can also have search() automatically call initialize() before returning callbacks to make things smoother.

Edit: It occurs to me the above notes are all about adding better eventing support, but that's necessary to get logical device support working well.

Personally I prefer the first option too, also I think it's a much friendlier interface for new comers (especially Express lovers!)

Tricky thing about this is how to know how long to wait before grouping things... i.e. since current search emits events for each new device to crop up, logicalSearch() would need to wait until all devices are found before grouping anything. Not sure how to do that apart from doing a timeout.

Hmm, I wonder what the Sonos Controller apps work on that one. For now a timeout would be great!

Trying to getZoneTopology() from all available nodes then building up a state store I think will be the fastest option as I've found the Sonos devices can be a little bit slow to respond to some requests

[stephen]

@bencevans - I've written up a search function that uses getZoneTopology from the first device it finds (assumes that a given device will have consensus on all other devices) which is pretty fast. Would it make sense as a separate PR after this merges, or all together here?

Further - I think we should also consider the idea of responding to changing topology, i.e. emitting an event when groups change or devices enter/exit. It might make sense to have a... SonosDeviceManager or that emits state changes and exposes the search function. This is because it wouldn't make sense to do global topology change events from a single device (or even logical device). Thoughts?

[mrose17]

hi. i agree that implementing EventEmitter is a good idea. i am very concerned about breaking existing behavior though. if that's necessary, then we need to bump the version to 1.0.x

overall, these are very positive changes to the package, so i'm happy to help out as needed.

[bencevans]

@bencevans - I've written up a search function that uses getZoneTopology from the first device it finds (assumes that a given device will have consensus on all other devices) which is pretty fast. Would it make sense as a separate PR after this merges, or all together here?

Oh yeah that method defo sounds best. Shall we make a 1.x branch and submit pull requests to that?

hi. i agree that implementing EventEmitter is a good idea. i am very concerned about breaking existing behavior though. if that's necessary, then we need to bump the version to 1.0.x

Definitely needs to be a new major version, I think the incentives for people to upgrade to use the new version would be quite beneficial

[stephen]

Yeah if you could open a 1.0.0 relase branch, it'd be good. Will close this and open a new PR against 1.0.0 when that's happened.

The search stuff needs more thought, so I'll open two separate PRs for it.

[stephen]

Closing in favor of #53

[bencevans]

Shiny new 1.x branch! https://github.com/bencevans/node-sonos/tree/1.x Awesome