Add sound mode support by starkillerOG · Pull Request #14910 · home-assistant/core

starkillerOG · 2018-06-10T12:51:46Z

Description:

Add sound mode support for the DenonAVR platform.
Using the just merged general sound mode support.

Added backend support for selecting the sound mode and updating the sound mode status.

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

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 or updated 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.

houndci-bot · 2018-06-10T12:51:54Z

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-06-10T13:01:09Z

homeassistant/components/media_player/denonavr.py

+        attributes = {}
+        if (self._sound_mode_raw is not None and self._sound_mode_support and
+            self._power == 'ON'):
+              attributes[ATTR_SOUND_MODE_RAW] = self._sound_mode_raw


indentation is not a multiple of four

houndci-bot · 2018-06-10T13:01:10Z

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'):


visually indented line with same indent as next logical line

starkillerOG · 2018-06-10T13:17:34Z

@balloob Can you merge this PR for the denonAVR implementation of sound mode support?
(this is a follow up of the general sound mode support that you merged last week)

balloob · 2018-06-10T15:54:11Z

homeassistant/components/media_player/denonavr.py

+        if sound_mode_support:
+            self._sound_mode = self._receiver.sound_mode
+            self._sound_mode_raw = self._receiver.sound_mode_raw
+            if sound_mode_dict is None:


If we can fetch it from the device, we should not allow it to be configured. Integrations should only represent the raw information that we receive from the device.

@balloob the sound_mode_dict can not be fetched from the device (receiver) itself, the denonavr library contains an default sound_mode_dict that is hard coded in the denonavr library. However that dictionary is based upon the data of about 10 diffrent models of denon receivers (from which the information xml files are available in the denonavr git repositry, but their could potentially be other values available for newer or very old receivers.
This allows for flexibility for these older or newer receivers.

I think that we should not include this config option because it will not motivate people to update the dictionary upstream for old and new receivers, resulting in all users having to collect the same information.

starkillerOG · 2018-06-10T16:25:44Z

see lines 38 to 49 in the denonavr.py file in the scarface-4711 github denonavr library for the default sound mode dict:

SOUND_MODE_MAPPING = OrderedDict(
    [('MUSIC', ['PLII MUSIC', 'DTS NEO:6 MUSIC', 'DOLBY D +NEO:X M',
                'ROCK ARENA', 'JAZZ CLUB', 'MATRIX']),
     ('MOVIE', ['PLII MOVIE', 'PLII CINEMA', 'DTS NEO:X CINEMA',
                'DTS NEO:6 CINEMA', 'DOLBY D +NEO:X C', 'MONO MOVIE']),
     ('GAME', ['PLII GAME', 'DOLBY D +NEO:X G', 'VIDEO GAME']),
     ('AUTO', ['None']),
     ('VIRTUAL', ['VIRTUAL']),
     ('PURE DIRECT', ['DIRECT']),
     ('DOLBY DIGITAL', ['DOLBY DIGITAL', 'DOLBY D + DOLBY SURROUND']),
     ('MCH STEREO', ['MULTI CH STEREO', 'MULTI CH IN']),
('STEREO', ['STEREO'])])

starkillerOG · 2018-06-10T17:11:10Z

@balloob I removed the option to configure the sound_mode_dict.
Do you have any aditional improvements/feedback?

starkillerOG · 2018-06-11T06:21:40Z

@balloob can this now be merged?

MartinHjelmare · 2018-06-11T09:32:29Z

homeassistant/components/media_player/denonavr.py

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


This isn't used.

- removed the sound_mode_raw propertie because it was not used, (still available through self._sound_mode_raw (as device attribute for automations and diagnostics)

starkillerOG · 2018-06-11T18:42:53Z

@MartinHjelmare Just removed the un-used sound_mode_raw propertie.
Thanks for the review!
Any adittional feedback?

balloob · 2018-06-11T18:53:12Z

homeassistant/components/media_player/denonavr.py

 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
    vol.Optional(CONF_HOST): cv.string,
    vol.Optional(CONF_NAME): cv.string,
+    vol.Optional(CONF_SOUND_MODE, default=DEFAULT_SOUND_MODE): cv.boolean,


Why is sound mode configurable? Can it not be detected from the device?

This is the option to disable sound_mode_support for old types receivers that do not have sound modes. As far as I know this can not be detected from the receiver itself. At least their is no direct bolean if sound mode is supported. It might be possible to see it from the absense of the current sound mode if you request it from the receiver.

However I always like to have the option to disable components in case they give problems for certain types/models. Of course I only have my own receiver to test with and that one does support sound_mode. So I cann't really test with a receiver that doesn't support sound mode.

However I always like to have the option to disable components in case they give problems for certain types/models.

In Home Assistant we aim to represent devices as-is. If we go down the route to disable sound modes, we can start adding config options for everything…

Please remove the config option and base it on detecting support.

I am working on it in the denonAVR library of @scarface-4711.

Okay. This PR can be merged when the config option has been removed and the lib has been updated.

starkillerOG · 2018-06-12T15:42:04Z

@balloob can this denonavr sound mode support be merged?

fabaff · 2018-06-30T16:55:23Z

The library update and the removal of the option is not done as mentioned here.

Removed the config option to indicate if sound mode is supported. Added detection if sound mode is supported from the receiver itself. Pushed denonavr library to V.0.7.4

starkillerOG · 2018-07-09T09:47:31Z

@balloob Thank you for all your help througout this PR!

* Add sound mode support * continuation line indent * indentation * indentation * Remove option to configure sound_mode_dict * Sound mode support - removed the sound_mode_raw propertie because it was not used, (still available through self._sound_mode_raw (as device attribute for automations and diagnostics) * Detect sound mode support from device Removed the config option to indicate if sound mode is supported. Added detection if sound mode is supported from the receiver itself. Pushed denonavr library to V.0.7.4 * Pushed denonavr to v.0.7.4

Martinvdm · 2018-07-20T19:54:49Z

After update to 0.74 the log is beeing spammed with the following items:

2018-07-20 21:51:12 WARNING (SyncWorker_9) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:51:23 WARNING (SyncWorker_13) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:51:34 WARNING (SyncWorker_5) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:51:45 WARNING (SyncWorker_7) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:51:56 WARNING (SyncWorker_17) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:52:07 WARNING (SyncWorker_3) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:52:18 WARNING (SyncWorker_17) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:52:29 WARNING (SyncWorker_14) [DenonAVR] Not able to match sound mode, returning raw sound mode.
2018-07-20 21:52:40 WARNING (SyncWorker_12) [DenonAVR] Not able to match sound mode, returning raw sound mode.

the device is turned off

MartinHjelmare · 2018-07-20T20:00:11Z

Please open an issue.

Add sound mode support

8280a19

homeassistant added cla-signed platform: media_player.denonavr labels Jun 10, 2018

ghost added the in progress label Jun 10, 2018

houndci-bot reviewed Jun 10, 2018

View reviewed changes

continuation line indent

5430c5f

homeassistant added the cla-signed label Jun 10, 2018

houndci-bot reviewed Jun 10, 2018

View reviewed changes

starkillerOG added 2 commits June 10, 2018 15:05

indentation

81ca56b

indentation

7d2d249

starkillerOG mentioned this pull request Jun 10, 2018

Sound mode support home-assistant/home-assistant.io#5521

Merged

2 tasks

balloob reviewed Jun 10, 2018

View reviewed changes

Remove option to configure sound_mode_dict

554884c

homeassistant added the cla-signed label Jun 10, 2018

MartinHjelmare reviewed Jun 11, 2018

View reviewed changes

Sound mode support

4368e31

- removed the sound_mode_raw propertie because it was not used, (still available through self._sound_mode_raw (as device attribute for automations and diagnostics)

homeassistant added the cla-signed label Jun 11, 2018

balloob reviewed Jun 11, 2018

View reviewed changes

starkillerOG mentioned this pull request Jun 13, 2018

Add support sound mode boolean ol-iver/denonavr#50

Merged

Detect sound mode support from device

6a7454c

Removed the config option to indicate if sound mode is supported. Added detection if sound mode is supported from the receiver itself. Pushed denonavr library to V.0.7.4

homeassistant added the cla-signed label Jul 9, 2018

Pushed denonavr to v.0.7.4

c93701b

balloob approved these changes Jul 9, 2018

View reviewed changes

balloob merged commit b9eb008 into home-assistant:dev Jul 9, 2018

ghost removed the in progress label Jul 9, 2018

balloob mentioned this pull request Jul 20, 2018

0.74 #15570

Merged

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

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

Uh oh!

Conversation

starkillerOG commented Jun 10, 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!

starkillerOG commented Jun 10, 2018

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!

starkillerOG commented Jun 10, 2018

Uh oh!

starkillerOG commented Jun 10, 2018

Uh oh!

starkillerOG commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkillerOG commented Jun 11, 2018

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!

starkillerOG commented Jun 12, 2018

Uh oh!

fabaff commented Jun 30, 2018

Uh oh!

starkillerOG commented Jul 9, 2018

Uh oh!

Martinvdm commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Jul 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

starkillerOG commented Jun 10, 2018 •

edited

Loading

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

Martinvdm commented Jul 20, 2018 •

edited

Loading