Add Songpal ("Sony Audio Control API") platform by rytilahti · Pull Request #12143 · home-assistant/core

rytilahti · 2018-02-03T02:12:30Z

Description:

This adds support for a variety of Sony soundbars and potentially
many more devices using the same API with the help of python-songpal: https://github.com/rytilahti/python-songpal

This device is automatically discovered by netdisco and added into homeassistant accordingly, so no manual configuration is required wherever it is working.

Official API from Sony: https://developer.sony.com/develop/audio-control-api/
Officially supported devices by Sony: HT-ST5000, HT-MT500, HT-CT800, SRS-ZR5, STR-DN1080 - however, see http://vssupport.sony.net/en_ww/device.html for list of (potentially) supported devices.

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

Example entry for `configuration.yaml` (if applicable):

media_player:
  - platform: songpal
    name: my soundbar
    endpoint: http://192.168.1.1:10000/sony

Checklist:

The code change is tested and works locally.

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.
New files were added to .coveragerc.

houndci-bot · 2018-02-03T02:12:39Z

homeassistant/components/media_player/songpal.py

line too long (98 > 79 characters)

houndci-bot · 2018-02-03T02:12:39Z

homeassistant/components/media_player/songpal.py

line too long (91 > 79 characters)

houndci-bot · 2018-02-03T02:12:39Z

homeassistant/components/media_player/songpal.py

line too long (84 > 79 characters)

houndci-bot · 2018-02-03T02:12:39Z

homeassistant/components/media_player/songpal.py

'typing.List' imported but unused

false positive, this is used in async_setup_platform.

rytilahti · 2018-02-04T18:31:30Z

Added WIP back as this depends on python >3.5 due due to async def usage. Can be merged after python 3.5 becomes a requirement, which can take some time.

In the meantime, it would be nice to get more people who have such a device to test the tool and the integration :-)

houndci-bot · 2018-02-23T22:17:15Z

homeassistant/components/media_player/songpal.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T22:17:15Z

homeassistant/components/media_player/songpal.py

line too long (85 > 79 characters)

balloob · 2018-02-24T04:36:37Z

homeassistant/components/media_player/songpal.py

Remove default=None. It's unnecessary for Optional to work and it also won't work since we upgraded voluptuous. The upgraded voluptuous will validate all values passed via default with the validator. None is not valid for cv.string

Good to know, thanks!

balloob · 2018-02-24T04:38:19Z

homeassistant/components/media_player/songpal.py

This won't work. devices is not shared between 2 configured entities. It would also mean that you are registering the service twice.

Indeed, no idea where I came up with that idea. Converted it to store devices in hass.data.

balloob · 2018-02-24T04:38:30Z

homeassistant/components/media_player/songpal.py

Let's move this to a constant.

What do you mean by moving this to a constant? Which part of it precisely?

The whole thing.

SET_SOUND_SCHEMA = vol.Schema(…)

Done, moved it on top of the file.

balloob · 2018-02-24T04:39:16Z

homeassistant/components/media_player/songpal.py

Why? Why not just assign it to self.dev in the constructor?

Indeed, changed that.

This adds support for a variety of Sony soundbars and potentially many more devices using the same API. See http://vssupport.sony.net/en_ww/device.html for list of supported devices.

…r better exception handling

balloob · 2018-02-26T08:09:54Z

homeassistant/components/media_player/songpal.py

+        except SongpalException as ex:
+            # if we were available, print out the exception
+            if self._available:
+                _LOGGER.error("Got an exception %s", ex, exc_info=True)


Use _LOGGER.exception(""), it will log the exception.

Does this also mean that whenever the device goes offline, we log a whole stack trace? That seems undesirable.

That it will do indeed. I thought it'd be a good idea to have something visible in case some of the above calls fail, but I can remove it and just print out the exception.

balloob · 2018-02-26T08:10:38Z

homeassistant/components/media_player/songpal.py

+        from songpal import SongpalException
+        try:
+            volumes = await self.dev.get_volume_information()
+            if not volumes:


Can you move all code that will not raise out of the try…except

The problem is that all the method calls may result in such an exception. One solution would be to do the fetching inside one block and then access the properties outside of it, I just kept it like this to keep the assignments & their usage nearby. I would not really like to have three separate try-except-blocks for this.

Fixed the rest of the issues (plus one extra I spotted in wrong handling of multiple volume controls) except this one. If you really prefer to have those fetches in one block, I could simply return in case of failure and set the state variables outside the try-catch block.

balloob · 2018-02-26T08:11:04Z

homeassistant/components/media_player/songpal.py

+        try:
+            volumes = await self.dev.get_volume_information()
+            if not volumes:
+                _LOGGER.error("Got no volume controls, bailing out")


Should this set available to False?

It should never get that far I suppose, but I can add a safe-guard for it.
edit: done.

balloob · 2018-02-26T08:11:50Z

.coveragerc

    homeassistant/components/media_player/roku.py
    homeassistant/components/media_player/russound_rio.py
    homeassistant/components/media_player/russound_rnet.py
+    homeassistant/components/media_player/songpal.py


* Fix coveragerc ordering * Do not print out the whole exception trace when failing to update * Do not bail out when receiving more than one volume control * Set to unavailable when no volume controls are available

MartinHjelmare · 2018-02-28T07:12:15Z

homeassistant/components/media_player/songpal.py

+        """Set volume level."""
+        volume = int(volume * self._volume_max)
+        _LOGGER.debug("Setting volume to %s", volume)
+        return await self._volume_control.set_volume(volume)


Why return here?

That is indeed unnecessary here, but it should not cause any side effects so I will fix that with the same PR where this will be converted non-polling, if that's okay.

rytilahti requested a review from andrey-git as a code owner February 3, 2018 02:12

homeassistant added cla-signed integration: discovery new-platform platform: media_player.songpal labels Feb 3, 2018

houndci-bot reviewed Feb 3, 2018

View reviewed changes

homeassistant added the cla-signed label Feb 3, 2018

rytilahti mentioned this pull request Feb 3, 2018

Add sony songpal docs home-assistant/home-assistant.io#4570

Merged

2 tasks

rytilahti changed the title ~~WIP: Add Songpal ("Sony Audio Control API") platform~~ Add Songpal ("Sony Audio Control API") platform Feb 3, 2018

homeassistant added the cla-signed label Feb 3, 2018

rytilahti force-pushed the add_sony_audio branch from f666564 to 7a0c1da Compare February 4, 2018 16:03

homeassistant added the cla-signed label Feb 4, 2018

rytilahti force-pushed the add_sony_audio branch from 7a0c1da to 84e95e5 Compare February 4, 2018 16:06

homeassistant added the cla-signed label Feb 4, 2018

rytilahti changed the title ~~Add Songpal ("Sony Audio Control API") platform~~ WIP: Add Songpal ("Sony Audio Control API") platform Feb 4, 2018

rytilahti mentioned this pull request Feb 18, 2018

Volume down error rytilahti/python-songpal#7

Closed

rytilahti force-pushed the add_sony_audio branch from 63d0704 to e315db8 Compare February 23, 2018 22:17

homeassistant added the cla-signed label Feb 23, 2018

houndci-bot reviewed Feb 23, 2018

View reviewed changes

rytilahti changed the title ~~WIP: Add Songpal ("Sony Audio Control API") platform~~ Add Songpal ("Sony Audio Control API") platform Feb 23, 2018

homeassistant added the cla-signed label Feb 23, 2018

rytilahti force-pushed the add_sony_audio branch from 47eeb6b to 555c0b5 Compare February 23, 2018 22:59

homeassistant added the cla-signed label Feb 23, 2018

rytilahti force-pushed the add_sony_audio branch from 555c0b5 to cae80d5 Compare February 23, 2018 23:25

homeassistant added the cla-signed label Feb 23, 2018

balloob reviewed Feb 24, 2018

View reviewed changes

rytilahti added 15 commits February 24, 2018 22:40

Add Songpal ("Sony Audio Control API") platform

7a6b08c

This adds support for a variety of Sony soundbars and potentially many more devices using the same API. See http://vssupport.sony.net/en_ww/device.html for list of supported devices.

add songpal requirement

75c20d5

update coveragerc

ad1a190

fix linting

92868c7

add service description to yaml

d23ca29

add entity_id

14d7ebf

make pylint also happy.

561b1f7

raise PlatformNotReady when initialization fails, bump requirement fo…

2c5cfe7

…r better exception handling

use noqa instead of pylint's disable, fix newline error

7240b61

use async defs and awaits

851bd78

Make linter happy

69bc0ab

Changes based on code review, fix set_sound_setting.

b21d2f6

define set_sound_setting schema on top of the file

6f36077

Move initialization back to async_setup_platform

08e47d7

Fix linting

9a5f653

rytilahti force-pushed the add_sony_audio branch from ae4d6f9 to 9a5f653 Compare February 24, 2018 21:41

homeassistant added the cla-signed label Feb 24, 2018

balloob reviewed Feb 26, 2018

View reviewed changes

Fixes based on code review

d09c8fb

* Fix coveragerc ordering * Do not print out the whole exception trace when failing to update * Do not bail out when receiving more than one volume control * Set to unavailable when no volume controls are available

balloob merged commit 39cee98 into home-assistant:dev Feb 27, 2018

rytilahti deleted the add_sony_audio branch February 28, 2018 00:27

MartinHjelmare reviewed Feb 28, 2018

View reviewed changes

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

tleegaard mentioned this pull request Mar 9, 2018

Songpal error with 0.65 #13022

Closed

home-assistant locked and limited conversation to collaborators Jul 26, 2018

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

Uh oh!

Conversation

rytilahti commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rytilahti commented Feb 3, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

rytilahti commented Feb 4, 2018 •

edited

Loading

balloob Feb 26, 2018 •

edited

Loading

rytilahti Feb 26, 2018 •

edited

Loading