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

Support for Soundtouch multiroom #165

Merged
merged 4 commits into from
Mar 22, 2020
Merged

Support for Soundtouch multiroom #165

merged 4 commits into from
Mar 22, 2020

Conversation

kalkih
Copy link
Owner

@kalkih kalkih commented Aug 23, 2019

Soundtouch multiroom requires some additional calls compared to most other platforms.
DOCS

@kalkih kalkih added the enhancement New feature or request label Aug 23, 2019
@kalkih
Copy link
Owner Author

kalkih commented Aug 23, 2019

@mkoninkx & @da-anda, this is what I came up with after reading the docs carefully, can't really test it in real life so would be great if you could give it a shot!
Compiled code can be found here (gist).
Thanks!

@da-anda
Copy link
Collaborator

da-anda commented Aug 23, 2019

doesn't work yet. I get the following error:

2019-08-23 16:52:55 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection.1838022736] value should be a string for dictionary value @ data['slaves']
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/websocket_api/commands.py", line 128, in handle_call_service
    connection.context(msg),
  File "/usr/src/homeassistant/homeassistant/core.py", line 1213, in async_call
    processed_data = handler.schema(service_data)
  File "/usr/local/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 267, in __call__
    return self._compiled([], data)
  File "/usr/local/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 589, in validate_dict
    return base_validate(path, iteritems(data), out)
  File "/usr/local/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 427, in validate_mapping
    raise er.MultipleInvalid(errors)
voluptuous.error.MultipleInvalid: value should be a string for dictionary value @ data['slaves']

edit: calling the service manually via the HA service panel works just fine when using an [] array as part of the JSON data object. So no idea what's wrong here. Is entity actually a string here or might it already be an array when multiple speakers are selected? Didn't read the entire code of this component yet.

@kalkih
Copy link
Owner Author

kalkih commented Aug 23, 2019

Alright, thanks for the detailed error!
iirc, entity should be a string (the entity_id), but I might be wrong, will double check later.
If it's indeed a string, I guess we could try sending the string directly and see if that works, as we never actually add multiple slaves at a time. I know sonos and other components accepts both a string and a list/array, might be similar here.

@da-anda
Copy link
Collaborator

da-anda commented Aug 23, 2019

if sonos etc do accept both, a list and an array, why do you loop over each speaker and send it individually? Or is this service only triggered once? In that case you would have to just omit the wrapping ' []' in the JSON data. Will give it a quick shot when I can find the correct spot in the compiled code.

@da-anda
Copy link
Collaborator

da-anda commented Aug 23, 2019

just did some more testing. entity is a string when you tick one of the speakers and is an array when you click "group all". So in the later case we would end up with an array inside an array, which would be wrong. The service seems to also handle strings as slaves value, so just remove the [] in the service call and it should work just fine. (just tested)

edit: ungrouping didn't work yet though, will investigate some more

@kalkih
Copy link
Owner Author

kalkih commented Aug 23, 2019

if sonos etc do accept both, a list and an array, why do you loop over each speaker and send it individually?

We actually never loop through them, and you are right, it's an array when using the group all button, totally forgot about that feature (not using multiroom myself) 😆

The service seems to also handle strings as slaves value, so just remove the [] in the service call and it should work just fine. (just tested)

Great, then it function just like the other multiroom platforms, I'll make the change when I get back to my comp later, thank you.

@da-anda
Copy link
Collaborator

da-anda commented Aug 23, 2019

We actually never loop through them

yeah, figured it out myself. I thought that you have to first select the speakers and then click "group all" to start the grouping process. After adding some console.log(...) calls here and there I noticed that the grouping is triggered the second you toggle the speaker.

What doesn't seem to work yet is SOUNDTOUCH_REMOVE_ZONE_SLAVE. Nothing seems to happen when unticking the speaker. And when clicking the "ungroup" button, the list of entities is empty, which also causes an issue with the soundtouch service since it requires a list.

@kalkih
Copy link
Owner Author

kalkih commented Aug 23, 2019

Okay, what attribute does the soundtouch component use to expose the current group? Could be that it doesn't use the same as other components.

@kalkih
Copy link
Owner Author

kalkih commented Aug 23, 2019

Btw, I've added you as a collaborator, feel free to push commits to this branch if you want.

@da-anda
Copy link
Collaborator

da-anda commented Aug 25, 2019

Btw, I've added you as a collaborator, feel free to push commits to this branch if you want.

thanks. I'll investigate into the ungroup issue and PR the required changes. Might take a few days though since I'm pretty busy over the next days (even on weekends)

@kalkih
Copy link
Owner Author

kalkih commented Aug 25, 2019

Awesome, no hurry.
Could be that the card doesn't pick up the group correctly, if it's exposed differently than other plarforms

get group() {
const groupName = `${this.config.speaker_group.platform}_group`;
return this.attr[groupName] || [];
}

@da-anda
Copy link
Collaborator

da-anda commented Aug 25, 2019

it's likely called zone instead of group, but I'll check

edit: from a quick glance there is absolutely no grouping/zone info expose in the mediaplayer attributes - I at least couldn't see anything in the devtools states view. In doubt I might have to extend the soundtouch component to expose this info.
But as said, will investigate once I have a bit more time again.

@kalkih
Copy link
Owner Author

kalkih commented Aug 26, 2019

Ah, that's unfortunate.
I had a peek at the source and you are most likely right, the zone/group doesn't seem to be exposed.

@da-anda
Copy link
Collaborator

da-anda commented Oct 28, 2019

argh, so I just started to work on this and spent around 2 hours only to find this PR again which already had all the changes I had just been working on :(

What I just noticed is, that for some reason the checked status is not updated in the JS component in lovelace. So even though I see the check mark in the UI, the internal state of the group item is still unchecked. Any idea why that could be?

edit: nvm, seems it's all related to this missing group attribute

@kalkih
Copy link
Owner Author

kalkih commented Oct 28, 2019

Yes, and I think @jpearce73 has a working component implementation with the group attribute (see #155) but it requires changes to upstream repo which seems to be dead/inactive.

@da-anda
Copy link
Collaborator

da-anda commented Oct 28, 2019

yeah, seen his comments over there. But since his solution seems to take a bit longer I'm currently hacking in the soundtouch_group property in the current soundtouch component. From a brief look this component could use an overhaul in order to update state information instantly and not wait for HA to poll the info etc (libsoundtouch would have events for this).

Also, on the long run IMO it would be best if there would be a standard for multiroom handling in HA instead of components having to do their own thing. Maybe I can spend some time on it during the Christmas holidays.

@kalkih
Copy link
Owner Author

kalkih commented Oct 28, 2019

I've not looked into it but you say it would be possible to implement without changes to libsoundtouch?

Indeed a standard would be great, I've pointed people to the Sonos implementation when they've asked for requirements, since that implementation imo works well.

I think @lyghtnox is looking to implement the group attribute in the Snapcast component, and I know @hcoohb has a working yamaha musiccast implementation (waiting for upstream as well I think).

@jpearce73
Copy link

Hi guys

Sorry I have been traveling for work and not had much spare time.
I did review my changes a few weeks ago and its definitely possible to move a couple of things I did in libsoundtouch into soundtouch component so that it could be released. I think this is the way to go.

Seperately we can get another branch up and see about having libsoundtouch upgraded.

Just a warning. In my experience libsoundtouch 0.7.0 is less reliable than 0.8.0. I found some strange and inconsistent behavior with 0.7.0. My libsoundtouch changes have heaps of logging added to try and figure this out, and I found 0.8.0 (which never got released into HA) is much better.

@da-anda - The group property in my uploaded file definitely works, and you are welcome to take it and anything else and make a PR request out of it. I'll get to it one day but if you have time I don't care how it ends up getting shared.

@da-anda
Copy link
Collaborator

da-anda commented Oct 28, 2019

@kalkih yes, it works by simply extending the HA soundtouch component.
@jpearce73 mind pointing me to the file you uploaded? But I have it 90% working already (still need to learn Python which is why I'm a bit slow on things here)

@jpearce73
Copy link

@da-anda - The zip is posted at the bottom of this comment #155 (comment)

@da-anda
Copy link
Collaborator

da-anda commented Oct 28, 2019

thanks, just had quick a look and I solved it quite similar to your code. Preparing the PR now.

You can have a look at my take on it here: https://github.com/da-anda/home-assistant/commits/soundtouch

@da-anda
Copy link
Collaborator

da-anda commented Oct 30, 2019

I just tried to push to this branch, but got an access denied error from GIT. Am I still a contributor to this branch? I used the SSH URL of your repo for my push attempt, which I think is the correct way, or do I have to use the http one? Haven't contributed to a PR yet - only created my own so far.

@kalkih
Copy link
Owner Author

kalkih commented Oct 30, 2019

@da-anda Says it's still waiting for your confirmation, re-sent the invite now.
If you can't find the invite feel free to create a new PR and I'll close this one.

@da-anda
Copy link
Collaborator

da-anda commented Oct 30, 2019

sorry, had missed the invitation mail. All good now :)

src/model.js Outdated
Comment on lines 313 to 316
// if no entity is specified, remove self (leave group)
if (!entity || !entity.length) {
entity = this.id;
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to do this, should be handled already

@click=${e => this.player.handleGroupChange(e, isMaster ? group : this.player.entity_id, false)}>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't work for me when I tried. Can give it another try tough.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let me know, it's no biggie either way.

src/model.js Outdated
Comment on lines 317 to 320
// don't send master entity as slave to remove
if (typeof entity == "object" && entity.indexOf(this.master) > -1) {
entity.splice(entity.indexOf(this.master), 1);
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this?

Copy link
Collaborator

@da-anda da-anda Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still debugging this. For some reason it doesn't seem to work when I send multiple slaves to the remove_zone_slave service/method, even though the HA component as well as libsoundtouch seem to support this. When I click the button in the player, nothing happens. When I trigger the service manually with the according payload (containing all slaves) just one is actually removed. The other one won't ever get removed no matter how often I click or regardless which order they have in the payload. Something really odd seems to be going on here.
I'll try to send the HTTP POST request to the master manually in the next days (that's all the BOSE API seems to require) to see if the issue is in the API of BOSE or somewhere in our call chain

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works for me. I'll have to double-check tonight when I am home.
But I am using libsoundtouch 0.8.0 base rather than what you have, and I do think that the 0.7.2 has some issues like this. This might be a known issue to fix, or you might want to call the libsoundtouch routine repeatedly for each slave to work around it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just cheked.
Group All does work.
Ungroup does not work.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the debugging in my logs, which I would need to compare to the BOSE API

2019-10-30 20:28:06 INFO (SyncWorker_12) [custom_components.soundtouch.media_player] Removing slaves media_player.bose_stairs, media_player.bose_lounge, media_player.bose_kitchen from zone with master media_player.bose_kitchen
2019-10-30 20:28:06 DEBUG (SyncWorker_12) [libsoundtouch.device] request.post url=http://192.168.2.180:8090/removeZoneSlave, payload=<?xml version="1.0" encoding="UTF-8" ?><zone master="000C8AC06D11"><member ipaddress="192.168.2.182">A81B6A53F41B</member><member ipaddress="192.168.2.181">08DF1F0DC6E0</member><member ipaddress="192.168.2.180">000C8AC06D11</member></zone>, response=<?xml version="1.0" encoding="UTF-8" ?><status>/removeZoneSlave</status>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I got the UNGROUP button working.
Here is my code for soundtouch remove_zone_slave.
Note that the commented code seems correct to me, BOSE API acknowledges it but does not execute it.
It was important not to call remove slave on the master, and mini-card does return the master in the list of things to ungroup. So I filter that out.

It tests well for me.

 def remove_zone_slave(self, slaves):
        """
        Remove slave(s) from and existing zone (multi-room).

        Zone must already exist and slaves array can not be empty.
        Note: If removing last slave, the zone will be deleted and you'll have
        to create a new one. You will not be able to add a new slave anymore

        :param slaves: slaves to remove from the zone. Slaves here are of class SoundTouchDevice(MediaPlayerDevice)

        """
        if not slaves:
            _LOGGER.warning("Unable to find slaves to remove")
        else:
            # Underlying BOSE API does not seem to work corrently when removeslave command is sent
            # with multiple slaves. API responds with an OK, it just doesn't do it.
            # So unroll the command into multiple API calls, one for each slave
            #slave_names = [s.entity_id for s in slaves if (slave.entity_id != self.entity_id) ]
            #_LOGGER.info("Removing slaves %s from zone with master %s",
            #             ", ".join(slave_names), self.entity_id)
            #self._device.remove_zone_slave([slave.device for slave in slaves if (slave.entity_id != self.entity_id)])
            for s in slaves:
                if (s.entity_id != self.entity_id):
                    # Only remove slaves, do not call remove removeSlave on itself
                    _LOGGER.info("Removing slave %s from zone with master %s",s.entity_id, self.entity_id)
                    self._device.remove_zone_slave([s.device])

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. IIRC I had already tested removing the slaves one by one, but also didn't work. Will give it another try then

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, not working here. I see in the log that it tries to remove one out of two radios, but nothing actually happens, which is super weird since the "LEAVE" button is working just fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, its definitely working for me with the code above.
Here are my detailed debug logs
You are filtering out the master? Calling remove slave with the master as the data caused Bose to do some weird things.

2019-11-03 09:52:12 DEBUG (SyncWorker_10) [custom_components.soundtouch.media_player] service_handle <ServiceCall media_player.soundtouch_remove_zone_slave (c:e4cf862899a74c1aad3a253c188245e1): master=media_player.bose_kitchen, slaves=['media_player.bose_kitchen', 'media_player.bose_stairs', 'media_player.bose_lounge', 'media_player.bose_family_room']> with master=media_player.bose_kitchen, slave_ids=media_player.bose_kitchen ,media_player.bose_stairs ,media_player.bose_lounge ,media_player.bose_family_room
2019-11-03 09:52:12 INFO (SyncWorker_10) [custom_components.soundtouch.media_player] Removing slave media_player.bose_stairs from zone with master media_player.bose_kitchen
2019-11-03 09:52:12 DEBUG (SyncWorker_10) [libsoundtouch.device] request.post url=http://192.168.2.180:8090/removeZoneSlave, payload=<?xml version="1.0" encoding="UTF-8" ?><zone master="000C8AC06D11"><member ipaddress="192.168.2.182">A81B6A53F41B</member></zone>, response=<?xml version="1.0" encoding="UTF-8" ?><status>/removeZoneSlave</status>
2019-11-03 09:52:12 INFO (SyncWorker_10) [custom_components.soundtouch.media_player] Removing slave media_player.bose_lounge from zone with master media_player.bose_kitchen
2019-11-03 09:52:13 DEBUG (SyncWorker_10) [libsoundtouch.device] request.post url=http://192.168.2.180:8090/removeZoneSlave, payload=<?xml version="1.0" encoding="UTF-8" ?><zone master="000C8AC06D11"><member ipaddress="192.168.2.181">08DF1F0DC6E0</member></zone>, response=<?xml version="1.0" encoding="UTF-8" ?><status>/removeZoneSlave</status>
2019-11-03 09:52:13 INFO (SyncWorker_10) [custom_components.soundtouch.media_player] Removing slave media_player.bose_family_room from zone with master media_player.bose_kitchen
2019-11-03 09:52:13 DEBUG (SyncWorker_10) [libsoundtouch.device] request.post url=http://192.168.2.180:8090/removeZoneSlave, payload=<?xml version="1.0" encoding="UTF-8" ?><zone master="000C8AC06D11"><member ipaddress="192.168.2.183">B0D5CCD8F1C5</member></zone>, response=<?xml version="1.0" encoding="UTF-8" ?><status>/removeZoneSlave</status>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like I had a bug in my player JS code. Cleaned everything up and it's working now.

@jpearce73
Copy link

@da-anda - I am a little confused by the pull requests. I am expecting to see a PR for mediaplayer/soundtouch and I was going to ask what the best way for me to test it.

Can I build a HA install with your changes and give it a true clean vanilla test?
Or should I just copy your soundtouch files into custom_components folder (I'd also restore libsoundtouch back to 0.7.2)?

@da-anda
Copy link
Collaborator

da-anda commented Nov 6, 2019

I know it's failing and probably also missing documentation changes,, but I can't run this friggin tox thing on Windows to get CI green, and I don't have the time to setup a Linux build env since I'm super stressed out at work in the next couple weeks (16+ h shifts atm to hopefully get the project done in time :( ). Also, without any feedback from the maintainers if this change is welcome at all I won't spend more time on it.

@frodoberlin
Copy link

Hi all, what is the current status of this implementation? I'm really looking forward to using it for my Bose setup. Best regards.

@kalkih
Copy link
Owner Author

kalkih commented Feb 16, 2020

Hello, waiting for home-assistant/core#28298 to be merged, as soon as that's merged and the functionality has made it into a HA release we should be good to go on this 👍🏼

@frodoberlin
Copy link

Ok, thanks!

@da-anda
Copy link
Collaborator

da-anda commented Mar 6, 2020

it finally has been merged into master 🎉

@kalkih
Copy link
Owner Author

kalkih commented Mar 6, 2020

Great!
Rebased this branch on dev to get it up to date, could you double check and make sure everything is implemented and working correctly?

Thanks!

@kalkih
Copy link
Owner Author

kalkih commented Mar 18, 2020

@da-anda Had any time to test?

@Krocko
Copy link

Krocko commented Mar 18, 2020

@kalkih how can I test this?

@schneekluth
Copy link

schneekluth commented Mar 18, 2020

Hey guys,
I have just updated hass to 0.107 and sadly have to report an issue because the current master is not identified correctly. I have tested with a zone of 4 speakers. 3 Soundtouch 10 and 1 Soundlink adapter and although my media_player.soundtouch_kuche is the master the soundlink adapter media_player.soundtouch_wohnzimmer and also another Soundtouch 10 media_player.soundtouch_room2 is also shown as master:

Master:
st1
Bose API of Master:
image

Slave but shown as master:
st2
Bose API for Slave:
image

Guess we are not done yet.

@schneekluth
Copy link

schneekluth commented Mar 19, 2020

@da-anda @Krocko Can you please check if your multi room state attributes are correct? What's really strange is that one of my slaves has the attibute is_master: true but lists itself as slave.

image

@da-anda
Copy link
Collaborator

da-anda commented Mar 19, 2020

@da-anda Had any time to test?

sorry, was waiting for the changes to be part of an actual release. Can test now that 0.107 is out. Will do so tonight.

@kalkih
Copy link
Owner Author

kalkih commented Mar 19, 2020

@kalkih how can I test this?

Follow the development instructions but instead of checking out the dev branch, check out this one (soundtouch_multiroom).
Can be a bit tricky if you no prior dev experience, but if you have time you'll learn a lot + it's fun! 😄

Hey guys,
I have just updated hass to 0.107 and sadly have to report an issue because the current master is not identified correctly. I have tested with a zone of 4 speakers. 3 Soundtouch 10 and 1 Soundlink adapter and although my media_player.soundtouch_kuche is the master the soundlink adapter media_player.soundtouch_wohnzimmer and also another Soundtouch 10 media_player.soundtouch_room2 is also shown as master:

Strange, if this is the case you should consider opening an issue in the official home assistant repo.

@da-anda
Copy link
Collaborator

da-anda commented Mar 19, 2020

@schneekluth the issue you mentioned is not related to the mini-media-player. For some odd reason the first radio that joins the group is getting the master assigned correctly. Any subsequent radio doesn't. No idea yet as to why that is the case yet.

@kalkih current version does not work since the services moved from the media_player namespace to the soundtouch namespace IIRC. Will fix later and commit.

@schneekluth
Copy link

@da-anda I know that this issue is not related to mini-media-player but @kalkih explicitly asked if someone could test the new multi room features. What exactly is causing the wrong attributes? Is the error on Bose's side? I have an account in their support forum and could address the issue over there but I need to know what exactly causes the wrong attributes.

Anyway I opened an issue in hass/core and made a reference to this thread: home-assistant/core#32976

@da-anda
Copy link
Collaborator

da-anda commented Mar 19, 2020

@schneekluth no idea yet why this error occurs. Maybe we have to send the existing slaves along with the API call or it's simply a bug in libsoundtouch or even the BOSE API. Requires further testing. What is working correctly though is the "group all" button - there all slaves have the correct master set.

Unfortunately I'm quite busy/stressed out atm, so I can't do a lot of testing. If you want you could test calling the soundtouch service manually via the dev tools in HA and see if you manage to find a way to add each radio one by one and still end up with the correct master value.

@da-anda
Copy link
Collaborator

da-anda commented Mar 19, 2020

@kalkih is the "leave" button supposed to be clickable on the master device itself? It's at least not disabled for me on a quick test.

@schneekluth
Copy link

schneekluth commented Mar 19, 2020

@da-anda I tried different multi room scenarios with various master/slave configuration and also provided additional infos in the issue over at hass/core home-assistant/core#32976 (comment)
I do not think Bose API is the problem. Master is always correctly identified in the XML output. @Krocko What about you? Have you found the time to test this?

@Krocko
Copy link

Krocko commented Mar 19, 2020

If @kalkih could make a beta release, I can test.
But at the moment I don‘t have a chance to compile the plugin myself.

@kalkih
Copy link
Owner Author

kalkih commented Mar 19, 2020

@kalkih is the "leave" button supposed to be clickable on the master device itself? It's at least not disabled for me on a quick test.

Yes, I added the ability to leave with the master a few releases back per request #213 (comment)

@da-anda
Copy link
Collaborator

da-anda commented Mar 22, 2020

@kalkih is the "leave" button supposed to be clickable on the master device itself? It's at least not disabled for me on a quick test.

Yes, I added the ability to leave with the master a few releases back per request #213 (comment)

ok, not sure this will ever work with soundtouch though. Anyways, I gave it a testrun on my setup today (including my fixup commits I already added to this branch and the PR with changes to the HA component) and it seems to work fine for me now (apart from the leave button on master devices). So this PR is IMO good to go.

@kalkih
Copy link
Owner Author

kalkih commented Mar 22, 2020

Great, thanks!
I'll merge this, we could always fix the leave button (master) in the future (if it's possible).
Expect a new release in the upcoming days.

Thanks everyone!

@kalkih kalkih merged commit 12179a5 into dev Mar 22, 2020
@kalkih kalkih deleted the soundtouch_multiroom branch January 4, 2021 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants