More features for the Bluesound component #11450

thrawnarn · 2018-01-04T17:36:42Z

Description:

Added support for join and unjoin speakers together
Added support for sleep timer
Added support for shuffle
Added support for playing of media (for example for TTS)

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4340

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

houndci-bot · 2018-01-04T17:36:49Z

homeassistant/components/media_player/bluesound.py

+        """Increases sleep time on player"""
+        sleep_time = yield from self.send_bluesound_command('/Sleep')
+        if sleep_time is None:
+            _LOGGER.error('Error while increasing sleep time on player: %s', self.host)


line too long (87 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

+    def async_remove_slave(self, slave_device):
+        """Remove slave to master"""
+        return self.send_bluesound_command('/RemoveSlave?slave={}&port={}'
+                                           .format(slave_device.host, slave_device.port))


line too long (89 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

+    def async_add_slave(self, slave_device):
+        """Add slave to master"""
+        return self.send_bluesound_command('/AddSlave?slave={}&port={}'
+                                           .format(slave_device.host, slave_device.port))


line too long (89 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

@@ -647,6 +778,9 @@ def supported_features(self):
        if self._status is None:
            return None

+        if self.is_grouped and not self.is_master:
+            return SUPPORT_VOLUME_STEP | SUPPORT_VOLUME_SET | SUPPORT_VOLUME_MUTE


line too long (81 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

+                    # we will force an update if the player is grouped
+                    # this isn't a foolproof solution. A better solution would be to fetch
+                    # sync_status more often when the device is playing. This would solve
+                    # alot of problems. This change will be done when the communication


line too long (87 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

+                elif self.is_grouped:
+                    # when player is grouped we need to fetch volume from sync_status.
+                    # we will force an update if the player is grouped
+                    # this isn't a foolproof solution. A better solution would be to fetch


line too long (90 > 79 characters)

houndci-bot · 2018-01-04T17:36:50Z

homeassistant/components/media_player/bluesound.py

+                    yield from asyncio.sleep(1, loop=self._hass.loop)
+                    yield from self.async_trigger_sync_on_all()
+                elif self.is_grouped:
+                    # when player is grouped we need to fetch volume from sync_status.


line too long (86 > 79 characters)

houndci-bot · 2018-01-04T17:36:51Z

homeassistant/components/media_player/bluesound.py

+                if group_name != self._group_name:
+                    _LOGGER.debug('Group name change detected on device: %s', self.host)
+                    self._group_name = group_name
+                    # the sleep is needed to make sure that the devices is synced


line too long (81 > 79 characters)

houndci-bot · 2018-01-04T17:36:51Z

homeassistant/components/media_player/bluesound.py

+
+                group_name = self._status.get('groupName', None)
+                if group_name != self._group_name:
+                    _LOGGER.debug('Group name change detected on device: %s', self.host)


line too long (88 > 79 characters)

houndci-bot · 2018-01-04T17:36:51Z

homeassistant/components/media_player/bluesound.py

+        if master is not None:
+            self._is_master = False
+            master_host = master.get('#text')
+            master_device = [device for device in self._hass.data[DATA_BLUESOUND]


line too long (81 > 79 characters)

thrawnarn · 2018-01-04T19:18:57Z

Hmm, seems to be a problem with the canary test. Their systems seem to have run into some problems. At least I think this is causing the Travis problems...

thrawnarn · 2018-01-05T23:16:56Z

Now it's all working. Feels a bit "risky" to rely on 3rd-party API:s to be working for the tests to go through. :(

balloob · 2018-02-18T06:08:27Z

homeassistant/components/media_player/bluesound.py

+        if update_tasks:
+            yield from asyncio.wait(update_tasks, loop=hass.loop)
+
+    descriptions = yield from hass.async_add_job(


We don't do descriptions like this anymore. Please remove.

balloob · 2018-02-18T06:09:30Z

We fixed the canary issue. Tests should never rely on external APIs.

For future PRs, please limit each PR to 1 change.

balloob · 2018-02-18T06:09:37Z

Can you resolve the merge conflicts?

balloob · 2018-02-18T06:10:33Z

homeassistant/components/media_player/bluesound.py

+            yield from getattr(player, method['method'])(**params)
+
+        for player in target_players:
+            if player.should_poll:


Why check this? You know if bluesound entities should be polled or not.

Very good question. I can't remember why, I will remove this

balloob · 2018-02-18T06:12:16Z

homeassistant/components/media_player/bluesound.py

+        sleep = 0
+        if self._status is not None:
+            sleep = self._status.get('sleep', 0)
+
        return {
            ATTR_MODEL: self._model,


Please remove model and brand info. That info does not belong in the state machine.

balloob · 2018-02-18T06:12:22Z

homeassistant/components/media_player/bluesound.py

        return {
            ATTR_MODEL: self._model,
            ATTR_MODEL_NAME: self._model_name,
            ATTR_BRAND: self._brand,
+            ATTR_SLEEP: sleep


What is sleep?

If this is a value that is automatically reduced / increased based on time, it's not allowed in the state attributes.

Sleep is a sleep timer. The value is automatically reduced, where am I suppose to place this kind of information? What's the difference from for example 'media_position' ? 'media_position' too is automatically increased.

FYI - I haven't changed this yet, even though github seems to think I changed this.

media position should not be automatically updated on each update. We store a time once and store a progress_updated_at. The frontend calculates the right value.

Let's remove sleep from the attributes.

houndci-bot · 2018-02-18T12:10:46Z

homeassistant/components/media_player/bluesound.py


 import aiohttp
 from aiohttp.client_exceptions import ClientError
 from aiohttp.hdrs import CONNECTION, KEEP_ALIVE
 import async_timeout
 import voluptuous as vol

+from homeassistant.config import load_yaml_config_file


'homeassistant.config.load_yaml_config_file' imported but unused

houndci-bot · 2018-02-18T12:10:47Z

homeassistant/components/media_player/bluesound.py

@@ -8,22 +8,26 @@
 from asyncio.futures import CancelledError
 from datetime import timedelta
 import logging
+import os


'os' imported but unused

houndci-bot · 2018-02-18T16:12:13Z

homeassistant/components/media_player/bluesound.py

+            _LOGGER.error('Error while increasing sleep time on player: %s',
+                          self.host)
+            return 0
+


blank line contains whitespace

balloob · 2018-02-18T17:43:44Z

homeassistant/components/media_player/bluesound.py

-            ATTR_MODEL: self._model,
-            ATTR_MODEL_NAME: self._model_name,
-            ATTR_BRAND: self._brand,
+            ATTR_SLEEP: sleep


Let's remove it. We should not have automatic decreasing values in state attributes. Ok values would be a sleep started timestamp and total sleep seconds.

Ok, I'll remove this

balloob · 2018-02-18T17:44:08Z

homeassistant/components/media_player/services.yaml

@@ -323,7 +323,7 @@ squeezebox_call_method:

 yamaha_enable_output:
  description: Enable or disable an output port
-
+  


Please remove this white space.

Ok, I'll remove this, I did it first, but then I noticed that thats how it is in current dev branch and I didn't think it was for me to correct. But I will of course do it.

balloob

🎉 🎉 🐬 🌮 Awesome! Thanks

MartinHjelmare

Please make a new PR with those fixes.

MartinHjelmare · 2018-02-18T23:04:55Z

homeassistant/components/media_player/bluesound.py

+                    # communication is moved to a separate library
+                    yield from self.force_update_sync_status()
+
+                self.schedule_update_ha_state()


This should be self.async_schedule_update_ha_state().

Please also fix this below in the old code.

* Added support for join and unjoin * Added support for sleep functionality * Fixed supported features * Removed long lines and fixed documentation strings * Fixed D401, imperative mood * Added shuffle support * Removed unnecessary log row * Removed model, modelname and brand * Removed descriptions * Removed polling command on method run. This change is not needed * Fixed merge errors * Removed unused usings * Pylint fixes * Hound fixes * Remove attr Sleep and removed white space in services.xml

homeassistant added cla-signed platform: media_player.bluesound labels Jan 4, 2018

houndci-bot reviewed Jan 4, 2018

View reviewed changes

thrawnarn changed the title ~~Bluesound update 1~~ More features for the Bluesound component Jan 4, 2018

homeassistant added the cla-signed label Jan 4, 2018

balloobbot added the platform label Jan 4, 2018

homeassistant added the cla-signed label Jan 4, 2018

thrawnarn mentioned this pull request Jan 4, 2018

New service methods for the Bluesound component home-assistant/home-assistant.io#4340

Merged

2 tasks

balloob reviewed Feb 18, 2018

View reviewed changes

thrawnarn added 8 commits February 18, 2018 12:18

Added support for join and unjoin

93d8516

Added support for sleep functionality

84d9ff0

Fixed supported features

393d916

Removed long lines and fixed documentation strings

aa8ef9f

Fixed D401, imperative mood

dd6de95

Added shuffle support

fe46196

Removed unnecessary log row

d8d6b62

Removed model, modelname and brand

4e174fa

homeassistant added the cla-signed label Feb 18, 2018

thrawnarn added 3 commits February 18, 2018 12:24

Removed descriptions

23bf799

Removed polling command on method run. This change is not needed

11a506b

Fixed merge errors

2526fe5

houndci-bot reviewed Feb 18, 2018

View reviewed changes

Removed unused usings

ee90100

Pylint fixes

d6fdde2

houndci-bot reviewed Feb 18, 2018

View reviewed changes

Hound fixes

112e1b8

balloob reviewed Feb 18, 2018

View reviewed changes

Remove attr Sleep and removed white space in services.xml

cc6afa2

balloob approved these changes Feb 18, 2018

View reviewed changes

balloob merged commit 1143499 into home-assistant:dev Feb 18, 2018

MartinHjelmare reviewed Feb 18, 2018

View reviewed changes

thrawnarn deleted the bluesound-update-1 branch February 19, 2018 17:29

thrawnarn mentioned this pull request Feb 19, 2018

Changed to async_schedule_update_ha_state #12518

Merged

balloob mentioned this pull request Feb 22, 2018

0.64.0 #12609

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: bluesound and removed platform: media_player.bluesound labels Mar 21, 2019

		@@ -323,7 +323,7 @@ squeezebox_call_method:

		yamaha_enable_output:
		description: Enable or disable an output port

More features for the Bluesound component #11450

More features for the Bluesound component #11450

Conversation

thrawnarn commented Jan 4, 2018 • edited

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrawnarn commented Jan 4, 2018 • edited

thrawnarn commented Jan 5, 2018

Choose a reason for hiding this comment

balloob commented Feb 18, 2018

balloob commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrawnarn commented Jan 4, 2018 •

edited

thrawnarn commented Jan 4, 2018 •

edited