nfarina / homebridge-sonos

Sonos plugin for homebridge: https://github.com/nfarina/homebridge
153 stars 52 forks source link

Enhancements 0.0.4 #15

Closed fwboettger closed 8 years ago

fwboettger commented 8 years ago

Dear @nfarina - i’d like to propose this significant improvement to my prior submission (leading to version 0.0.3 and subsequently 0.0.4 with your package dependency correction - no this time didn’t include another new dependency :-)).

The changes to index.js with this Pull Request include:

Remaining “known issue” (further investigation required, anticipated for next pull request):

nfarina commented 8 years ago

Cool, I'll try this out when I get home! Can anyone else check out this branch and verify that it works for them?

Nousemusic commented 8 years ago

@fwboettger How can I use and test your new version? Do I just have to use the new index.js (https://raw.githubusercontent.com/fwboettger/homebridge-sonos/enhancements-0.0.4/index.js)?

fwboettger commented 8 years ago

@Nousemusic - yes that's correct. Just use the new index.js, nothing else changed, just "the internals in there" :-)

Nousemusic commented 8 years ago

@fwboettger I used the new index.js, homebridge starts and is running stable. All my players are found and can be controlled, even the connect "Wohnzimmer" which is grouped with the two paired play3 "Küche":

[Radio Wohnzimmer] Initializing Sonos accessory... [Radio Mobil] Initializing Sonos accessory... [Radio Kinderzimmer] Initializing Sonos accessory... [Radio Bad] Initializing Sonos accessory... [Radio Küche] Initializing Sonos accessory... [Radio Wohnzimmer] Found a playable coordinator device at 192.168.2.130 in zone 'Küche' for accessory 'Radio Wohnzimmer' in accessory room 'Wohnzimmer' [Radio Mobil] Found a playable coordinator device at 192.168.2.117 in zone 'Mobil' for accessory 'Radio Mobil' in accessory room 'Mobil' [Radio Kinderzimmer] Found a playable coordinator device at 192.168.2.136 in zone 'Kinderzimmer' for accessory 'Radio Kinderzimmer' in accessory room 'Kinderzimmer' [Radio Bad] Found a playable coordinator device at 192.168.2.133 in zone 'Bad' for accessory 'Radio Bad' in accessory room 'Bad' [Radio Küche] Found a playable coordinator device at 192.168.2.130 in zone 'Küche' for accessory 'Radio Küche' in accessory room 'Küche'

Is there something specific I should test for you in my environment?

fwboettger commented 8 years ago

@Nousemusic - thanks much for supporting with the verification! Great to hear that it's working flawlessly and stable also in your environment and now also for your "Wohnzimmer" connect! Could you also help to test the 'dynamic grouping' in your environment (besides the 'Wohnzimmer + Küche' group, also add/remove other groups (e.g. "Bad + Kinderzimmer" or similar) while HomeBridge is running, and verify that you can properly start/stop music via the homekit app for those groups respectively the individual devices both after adding and again after removing the group? Don't look into steering the volume of the group yet please (that's a known issue - only the volume of the coordinator of the group is affected, not the group volume, need to further look into this).

Nousemusic commented 8 years ago

@fwboettger As you wrote, I tested the funktion 'dynamic grouping': First I grouped the devicec "Bad" and "Kinderzimmer" while HomeBridge is running. I could properly start/stop music via the homekit app for the group. After removing the group I could properly start/stop music via the homekit app for the seperated players.

Seems to work like a charm and in my eyes is a huge step in the development of the plugin. Thank you very much.

I will do some more testing when I'm back home. If I discover issues by using the Plugin via the homekit app in daily life I will give you a note.

fwboettger commented 8 years ago

dear @Nousemusic, thanks so much for your support and feedback! Great to have this confirmed as such, sure let me know anytime please of course should you come across any issues using this in daily life :-)

gazzer82 commented 8 years ago

This looks amazing, @fwboettger do i still need to define each Sonos speaker in the my config.json, or are the speakers now manually found.

fwboettger commented 8 years ago

@gazzer82 - you solely need to define the zones/rooms you want to see in your HomeKit app in config.json, but not each individual speaker. E.g. if you have a "Living Room" with multiple paired or grouped speakers, just that one accessory with respective "room: Living Room" needs to be there, the plugin will discover the appropriate speakers and associate them accordingly automatically.

leoneleone commented 8 years ago

@fwboettger - I keep getting errors when using the index.js file from Enhancements 0.0.4 "Ignoring request; Sonos device has not yet been discovered."

My Sonos Speakers initialise without any problems, homebridge is still running, but I can't send command to any of the speakers.

Please help

fwboettger commented 8 years ago

@leoneleone - may i ask you to post your config.json and the results when you're accessing Sonos' topology service at http://:1400/status/topology and then drilling into "Zone Players" (replace with an IP address of one of your sonos devices).

These "ignoring request" messages aren't errors, they rather indicate that the coordinator device required for your HomeKit configuration as per config.json has not yet been found by the plugin. Could be something in your config.json, could be caused by some unknown (to me) behaviour in your Sonos network, could be a bug in my code, ... need some more details please to figure out what's going on in your situation.

leoneleone commented 8 years ago

I have my AirPlay output connected to a Sonos:Connect which then feeds the audio to my other Sonos Players. The Sonos:Connect isn't set as Group Coordinator. I suppose I could just remove it from config.json and still get have it play via the group.

Is that right?

fwboettger commented 8 years ago

@leoneleone - yes please remove the connect from your config.json. I don't have one myself, but we recently came accross similar issues with the connect and the plugin. Basically the connect isn't a speaker, the plugin is meant to manage speakers, at least currently.... Let me know please the results.

leoneleone commented 8 years ago

@fwboettger - Great. I'll test that.

One more question: Does volume control on a coordinator speaker change the Sonos Group volume or just the single coordinator speaker volume?

fwboettger commented 8 years ago

@leoneleone - Group Volume unfortunately isn't implemented yet. The library being used by the plugin (node-sonos) doesn't seem to support this yet out of the box, this needs to be looked into some more.... Currently, adjusting the volume for a group coordinator speaker does this just for this speaker.

fwboettger commented 8 years ago

@nfarina - found an issue (see also issue https://github.com/bencevans/node-sonos/issues/121) in the listener.js code of the node-sonos library this plugin is using. Unfortunately this might lead to an uncaught exception and thus homebridge dying eventually (have this myself once in a while, usually after a few days of homebridge running) if this happens. Will attempt to find a resolution with/for node-sonos first, and eventually submit either a respectively adjusted/updated or a new pull request for this homebridge-sonos plugin...

mattnewham commented 8 years ago

@nfarina @fwboettger Confirming this is working much better than previously. All my devices are correctly registered now.

mattnewham commented 8 years ago

Comments after further testing:

I don't agree with the dynamic topology changes as described in point 3.

I should retain "dumb" control over each speaker from within home kit, rather than overall group control which has been implemented with this pull request:

If I am listening to music from within the sonos app throughout my house, I will group all devices within the app. If I then want to stop playback in one room (via HomeKit) - (lets say I have a call), turning off (stopping) playback for the room speaker now stops ALL speakers (because they have been dynamically added to a group from the Sonos topology change).

The Sonos app has much more control over what is playing etc, so I believe we should keep simple control (play/pause/volume) tied to individual speakers within the home bridge plugin.

jasongold commented 8 years ago

not sure this is the right place to put this.....

If you change lines 260 and 261 from 'stop' to 'pause' it won't start the playlist all over when you resume.

doooh commented 8 years ago

Is there a reason, that this pull request is not yet merged into the official branch?

At the moment there are a lot of issues with detecting all speakers and especially grouping. These enhancements seem to fix this issues?

Thanks in advance.

nfarina commented 8 years ago

I'm not able to test it personally so I haven't merged - if the consensus from the participants on this thread are that it's good to merge then I will happily do so.

doooh commented 8 years ago

@nfarina I would say that is so?

Regarding the comment from @mattnewham: In my opinion the way @fwboettger realized this feature is a real gain. If I group diferrent speakers in the Sonos app, I would expect HomeKit to also recognize this grouping and thus control the whole group instead of a single speaker. Of course this is more of a philosophical discussion, as there is no wrong and right here. I go +1 for this feature.

nfarina commented 8 years ago

OK - @fwboettger good to merge?

fwboettger commented 8 years ago

Good for me. Apologies didn't have time to look into further improvements as originally intended....

Am 25.05.2016 um 02:05 schrieb Nick Farina notifications@github.com:

OK - @fwboettger good to merge?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

nfarina commented 8 years ago

Thanks all!

doooh commented 8 years ago

Thanks @nfarina ! Could you please also publish the new version on npm and do a version bump?

nfarina commented 8 years ago

@doooh Done, sorry for delay!