Sound mode support media_player general and denonavr #13706

starkillerOG · 2018-04-05T19:41:39Z

Description:

Added backend support for selecting the sound mode and updating the sound mode status.
This is general support for all media_player components.
This box is not shown in media_players that did not (yet) implement sound mode support in the backend.
I implemented the sound mode support (backend) for the denonavr platform and that one works perfictly.
I already have a pull request for the frontend to have a dropdown select box in the user interface if the media_player supports sound mode.
home-assistant/frontend#815

I tested this code and it works on Hassbian on my pi.

On the home assistant chat I heard that other platforms like onkyo and sonos could also implement sound mode support and probably most receivers could support this, this also provides a base for other platforms to simply implement sound mode support.

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

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

media_player:
  - platform: denonavr
    host: IP_ADRESS
    name: "Marantz"
    show_all_sources: False
    timeout: 2
    sound_mode: True
    sound_mode_dict: {'MUSIC':['PLII MUSIC'], 'MOVIE':['PLII MOVIE'], 'GAME':['PLII GAME'], 'PURE DIRECT':['DIRECT'], 'AUTO':['None'], 'DOLBY DIGITAL':['DOLBY DIGITAL'], 'MCH STEREO':['MULTI CH STEREO'], 'STEREO':['STEREO']}

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

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.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

add sound mode support using new denonavr library functions

houndci-bot · 2018-04-05T19:41:50Z

homeassistant/components/media_player/denonavr.py

@@ -6,29 +6,32 @@
 """

 import logging
-from collections import namedtuple
+from collections import (namedtuple, OrderedDict)


'collections.OrderedDict' imported but unused

starkillerOG · 2018-04-05T20:04:21Z

Can someone please merge this PR?
It has already been discussed in this old PR .
It is now using the new denonavr v0.7.0 so all new code is in that library as was required by the revieuwers.

starkillerOG · 2018-04-06T07:40:52Z

@rytilahti could you review this new code which uses the denonavr library v.0.7.0?

starkillerOG · 2018-04-26T10:59:50Z

@scarface-4711 could you maybe review this PR, I would really like to wrap this up.

starkillerOG · 2018-05-01T21:33:56Z

@balloob Could you please take a look at this PR (since you merged the last denonavr PR)?
Waiting for a revieuw for a month now, really would like to finish this PR.
I am really confedent that it can be merged fast, since previous versions have already been looked at and revieuwd.

ol-iver · 2018-05-01T22:59:40Z

homeassistant/components/media_player/denonavr.py

+                self._sound_mode_list = list(sound_mode_dict)
+        else:
+            self._sound_mode = None
+            self._sound_mode_raw = None


self._sound_mode_raw attribute does not seem to be used at all and could be removed

Just made a commit where I implemented it as a state property.
Thanks for the feedback, I ment to implement this but forgot about it.
It is usefull to have this in the state because the raw sound mode provides more detailed information. (can be used in automations or to troubleshoot if the raw sound mode is not in the mapping in the denonavr library).

Did not test this change yet, will test it tonight.

ol-iver · 2018-05-01T23:02:41Z

homeassistant/components/media_player/denonavr.py

+        return self._sound_mode
+
+    @property
+    def sound_mode_raw(self):


Same as for self._sound_mode_raw. There is no equivalent property in constructor of parent media_player class, thus I assume it is not used.

same as above

balloob · 2018-05-02T13:40:18Z

homeassistant/components/media_player/__init__.py

@@ -106,6 +109,7 @@
 SUPPORT_CLEAR_PLAYLIST = 8192
 SUPPORT_PLAY = 16384
 SUPPORT_SHUFFLE_SET = 32768
+SUPPORT_SELECT_SOUND_MODE = 65536


We should not add this as a new supported feature for media players. We should only add things that exist among many media players.

Revert all changes to this file and add a new service to the denonavr platform called media_player.denonavr_set_sound_mode to allow changing thet mode.

I was planning to add support for sound modes in a form or another to songpal, so that'd be two platforms for using such a feature. I haven't checked out yet how to do that, but will try to find some time during the upcoming weekend for it, if that's fine?

@balloob I asked about this on the form when I first started with this code, and at that time there was also someone who was intrested in implementing sound mode support for the sonos platform based on my code (but he wanted to wait until I finished and it has been quite a long time).

But that would already make 3 platforms that would uses this.
Besides almost all sound systems have some kind of sound mode feature, therefore it could be implemented in most of the platforms if someone is willing to make the code (and has that device).

Therfore I made this a general support feature in the media_player/init.py file, and I think this is a good thing. Do you agree @balloob?

See https://developers.home-assistant.io/docs/en/entity_index.html#changing-the-entity-model

So let me close this PR and the frontend PR, and you can open an issue on the architecture repo to propose the new feature.

This is usefull because the sound_mode_raw shows more information than only the sound_mode.

houndci-bot · 2018-05-03T15:45:59Z

homeassistant/components/media_player/denonavr.py

+        """Return device specific state attributes."""
+        attributes = {}
+        if self._sound_mode_raw is not None and self._sound_mode_support\
+          and self._power == 'ON':


continuation line over-indented for visual indent

houndci-bot · 2018-05-03T15:47:28Z

homeassistant/components/media_player/denonavr.py

+        """Return device specific state attributes."""
+        attributes = {}
+        if self._sound_mode_raw is not None and self._sound_mode_support\
+        and self._power == 'ON':


continuation line missing indentation or outdented

starkillerOG · 2018-05-03T15:51:32Z

Tested the new commit (sound mode raw as state attribute), and it all works.
@scarface-4711, @balloob, anything else I schould change?
@balloob do you agree with keeping the general sound mode support for the other platforms (in the media_player/init.py file?

starkillerOG · 2018-05-04T06:45:27Z

@balloob How long is it going to take to get this merged if I go and open an issue on the architecture repo?
In fact everything in the general files (media_player/init.py, media_player/services.yaml) is just an coppy of the already implemented ATTR_INPUT_SOURCE = 'source' and ATTR_INPUT_SOURCE_LIST = 'source_list' attributes. Therefore the architecture is identical to this already implemented feature, so I do not see much problems.

I have been working on this sound mode support since january, and it was already working in january. I just experiance so much resistance to get this implemented. I am verry willing to implement changes, no problem, I just need some more detailed directions of what is expected.

So if this is going to take again so long to get it through the architecturee repo, I am not going to do that. Maybe I could then converted it to a denonavr specific service, and otherwise I will just continue as I have it now and use it as a custom component. I would really like to help the home assistant community implement some new features (such as this), but if you get so much resistance the fun fades away fast.

rytilahti · 2018-05-31T13:42:37Z

Is this PR now active again, that is, should I base songpal modifications on top of this PR?

starkillerOG · 2018-05-31T15:52:54Z

I will probably make some small adjustments to this PR based on the help of @balloob.
But I think we are continuing with this PR.

Bases on home-assistant#13706 Related to home-assistant/architecture#23

rytilahti · 2018-05-31T15:57:42Z

Ok, could you please separate the PR for support in general and support for denonavr? And also rebase on the current head. Then I could base my WIP on that generic support PR to avoid having denonavr commits in that linked PR.

starkillerOG · 2018-05-31T16:04:08Z

@rytilahti Yes I will do this tonight, I will make 2 seprate PR's, I just want to wait until @balloob gives the okay to go ahead with these PR's.
I need to update my HomeAssistant to the latest version and then check if everything still works.
I know the front-end has changed since the last time I made the PR, so might have to do some work to get everything up to date again.

balloob · 2018-06-01T14:09:45Z

Yes go for it.

starkillerOG added 4 commits April 5, 2018 21:14

Add sound mode support

062db8a

add sound mode support using new denonavr library functions

Denonavr to v.0.7.0

69440e2

Add sound mode support

8dab888

Add sound mode support

142a71a

homeassistant added cla-signed integration: media_player core platform: media_player.denonavr labels Apr 5, 2018

houndci-bot reviewed Apr 5, 2018

View reviewed changes

Add sound mode support

c7df1bb

homeassistant added the cla-signed label Apr 5, 2018

This was referenced Apr 5, 2018

Sound mode support media_player general and denonavr #11764

Closed

Add sound mode support (media_player frontend) home-assistant/frontend#815

Closed

Add sound mode support home-assistant/home-assistant.io#4470

Closed

starkillerOG mentioned this pull request Apr 16, 2018

Add arguments with effects #13500

Closed

8 tasks

ol-iver reviewed May 1, 2018

View reviewed changes

balloob requested changes May 2, 2018

View reviewed changes

Added sound_mode_raw as a state attribute

a5ae281

This is usefull because the sound_mode_raw shows more information than only the sound_mode.

homeassistant added the cla-signed label May 3, 2018

update sound mode raw property

90924ac

houndci-bot reviewed May 3, 2018

View reviewed changes

spacing

e4da0d4

houndci-bot reviewed May 3, 2018

View reviewed changes

spacing

29659cb

balloob closed this May 4, 2018

starkillerOG mentioned this pull request May 4, 2018

Sound mode support media_player general and denonavr home-assistant/architecture#23

Closed

rytilahti added a commit to rytilahti/home-assistant that referenced this pull request May 31, 2018

Implement sound mode support for songpal

bacb6b0

Bases on home-assistant#13706 Related to home-assistant/architecture#23

rytilahti mentioned this pull request May 31, 2018

WIP: Songpal sound mode support #14718

Closed

8 tasks

home-assistant locked and limited conversation to collaborators Sep 5, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sound mode support media_player general and denonavr #13706

Sound mode support media_player general and denonavr #13706

starkillerOG commented Apr 5, 2018 •

edited

Loading

houndci-bot Apr 5, 2018

starkillerOG commented Apr 5, 2018

starkillerOG commented Apr 6, 2018

starkillerOG commented Apr 26, 2018

starkillerOG commented May 1, 2018

ol-iver May 1, 2018

starkillerOG May 3, 2018

starkillerOG May 3, 2018

ol-iver May 1, 2018

starkillerOG May 3, 2018

balloob May 2, 2018

rytilahti May 3, 2018

starkillerOG May 3, 2018

balloob May 4, 2018

balloob May 4, 2018

houndci-bot May 3, 2018

houndci-bot May 3, 2018

starkillerOG commented May 3, 2018

starkillerOG commented May 4, 2018

rytilahti commented May 31, 2018

starkillerOG commented May 31, 2018

rytilahti commented May 31, 2018 •

edited

Loading

starkillerOG commented May 31, 2018

balloob commented Jun 1, 2018

Sound mode support media_player general and denonavr #13706

Sound mode support media_player general and denonavr #13706

Conversation

starkillerOG commented Apr 5, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

starkillerOG commented Apr 5, 2018

starkillerOG commented Apr 6, 2018

starkillerOG commented Apr 26, 2018

starkillerOG commented May 1, 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

starkillerOG commented May 3, 2018

starkillerOG commented May 4, 2018

rytilahti commented May 31, 2018

starkillerOG commented May 31, 2018

rytilahti commented May 31, 2018 • edited Loading

starkillerOG commented May 31, 2018

balloob commented Jun 1, 2018

starkillerOG commented Apr 5, 2018 •

edited

Loading

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

rytilahti commented May 31, 2018 •

edited

Loading