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

Sound mode support media_player general and denonavr #11764

Closed
wants to merge 12 commits into from

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jan 18, 2018

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.

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

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.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Add sound mode support as a general feature.
implemented in the denonavr component
Add sound mode support (media_player services.yaml)
Implement the sound mode support for the denonavr platform using the  new general sound mode support.
- improved code a bit
- added the option to disable sound mode support in the configuration.yaml file
- added the option to specify a custom list of sound modes in the configuration.yaml file
@@ -292,6 +336,12 @@ def select_source(self, source):
"""Select input source."""
return self._receiver.set_input_func(source)

def select_sound_mode(self, sound_mode):
"""Select sound mode."""
url = 'http://' + str(self._host) + '/MainZone/index.put.asp?cmd0=PutSurroundMode%2F' + sound_mode.upper().replace(" ", "+")

Choose a reason for hiding this comment

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

line too long (132 > 79 characters)

parsed_data = ET.parse(XML_data).getroot()
sound_mode_raw = parsed_data.find('selectSurround/value').text.rstrip()
try:
sound_mode = list(self._sound_mode_dict.keys())[list(self._sound_mode_dict.values()).index(sound_mode_raw.upper())]

Choose a reason for hiding this comment

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

line too long (131 > 79 characters)

except ConnectTimeoutError:
return
parsed_data = ET.parse(XML_data).getroot()
sound_mode_raw = parsed_data.find('selectSurround/value').text.rstrip()

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

try:
url = 'http://' + str(self._host) + '/goform/formMainZone_MainZoneXml.xml'
XML_data = urllib.request.urlopen(url)
except ConnectTimeoutError:

Choose a reason for hiding this comment

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

undefined name 'ConnectTimeoutError'

@@ -165,6 +185,20 @@ def update(self):
self._frequency = self._receiver.frequency
self._station = self._receiver.station

if self._sound_mode_support:
try:
url = 'http://' + str(self._host) + '/goform/formMainZone_MainZoneXml.xml'

Choose a reason for hiding this comment

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

line too long (90 > 79 characters)

@@ -117,7 +129,7 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
show_all_inputs=show_all_sources, timeout=timeout,
add_zones=add_zones)
for new_zone in new_device.zones.values():
receivers.append(DenonDevice(new_zone))
receivers.append(DenonDevice(new_zone, host, sound_mode_support, sound_mode_dict))

Choose a reason for hiding this comment

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

line too long (98 > 79 characters)

@@ -66,6 +75,7 @@ def setup_platform(hass, config, add_devices, discovery_info=None):

# Initialize list with receivers to be started
receivers = []
hosts = []

Choose a reason for hiding this comment

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

local variable 'hosts' is assigned to but never used

@@ -49,6 +56,8 @@
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_SOUND_MODE, default=DEFAULT_SOUND_MODE): cv.boolean,
vol.Optional(CONF_SOUND_MODE_DICT, default=DEFAULT_SOUND_MODE_DICT): vol.Schema({str: str}),

Choose a reason for hiding this comment

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

line too long (96 > 79 characters)

@@ -27,8 +30,12 @@
DEFAULT_NAME = None
DEFAULT_SHOW_SOURCES = False
DEFAULT_TIMEOUT = 2
DEFAULT_SOUND_MODE = True
DEFAULT_SOUND_MODE_DICT = OrderedDict([('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')])

Choose a reason for hiding this comment

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

line too long (250 > 79 characters)

SUPPORT_SELECT_SOURCE, SUPPORT_PLAY_MEDIA, MEDIA_TYPE_CHANNEL,
MediaPlayerDevice, PLATFORM_SCHEMA, SUPPORT_TURN_ON,
MEDIA_TYPE_MUSIC, SUPPORT_VOLUME_SET, SUPPORT_PLAY)
SUPPORT_SELECT_SOURCE,SUPPORT_SELECT_SOUND_MODE,

Choose a reason for hiding this comment

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

missing whitespace after ','

@@ -292,6 +336,12 @@ def select_source(self, source):
"""Select input source."""
return self._receiver.set_input_func(source)

def select_sound_mode(self, sound_mode):
"""Select sound mode."""
url = 'http://' + str(self._host) + '/MainZone/index.put.asp?cmd0=PutSurroundMode%2F' + sound_mode.upper().replace(" ", "+")

Choose a reason for hiding this comment

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

line too long (132 > 79 characters)

parsed_data = ET.parse(XML_data).getroot()
sound_mode_raw = parsed_data.find('selectSurround/value').text.rstrip()
try:
sound_mode = list(self._sound_mode_dict.keys())[list(self._sound_mode_dict.values()).index(sound_mode_raw.upper())]

Choose a reason for hiding this comment

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

line too long (131 > 79 characters)

except ConnectTimeoutError:
return
parsed_data = ET.parse(XML_data).getroot()
sound_mode_raw = parsed_data.find('selectSurround/value').text.rstrip()

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

try:
url = 'http://' + str(self._host) + '/goform/formMainZone_MainZoneXml.xml'
XML_data = urllib.request.urlopen(url)
except ConnectTimeoutError:

Choose a reason for hiding this comment

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

undefined name 'ConnectTimeoutError'

@@ -165,6 +185,20 @@ def update(self):
self._frequency = self._receiver.frequency
self._station = self._receiver.station

if self._sound_mode_support:
try:
url = 'http://' + str(self._host) + '/goform/formMainZone_MainZoneXml.xml'

Choose a reason for hiding this comment

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

line too long (90 > 79 characters)

@@ -117,7 +129,7 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
show_all_inputs=show_all_sources, timeout=timeout,
add_zones=add_zones)
for new_zone in new_device.zones.values():
receivers.append(DenonDevice(new_zone))
receivers.append(DenonDevice(new_zone, host, sound_mode_support, sound_mode_dict))

Choose a reason for hiding this comment

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

line too long (98 > 79 characters)

@@ -66,6 +75,7 @@ def setup_platform(hass, config, add_devices, discovery_info=None):

# Initialize list with receivers to be started
receivers = []
hosts = []

Choose a reason for hiding this comment

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

local variable 'hosts' is assigned to but never used

@@ -49,6 +56,8 @@
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_SOUND_MODE, default=DEFAULT_SOUND_MODE): cv.boolean,
vol.Optional(CONF_SOUND_MODE_DICT, default=DEFAULT_SOUND_MODE_DICT): vol.Schema({str: str}),

Choose a reason for hiding this comment

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

line too long (96 > 79 characters)

@@ -27,8 +30,12 @@
DEFAULT_NAME = None
DEFAULT_SHOW_SOURCES = False
DEFAULT_TIMEOUT = 2
DEFAULT_SOUND_MODE = True
DEFAULT_SOUND_MODE_DICT = OrderedDict([('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')])

Choose a reason for hiding this comment

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

line too long (250 > 79 characters)

SUPPORT_SELECT_SOURCE, SUPPORT_PLAY_MEDIA, MEDIA_TYPE_CHANNEL,
MediaPlayerDevice, PLATFORM_SCHEMA, SUPPORT_TURN_ON,
MEDIA_TYPE_MUSIC, SUPPORT_VOLUME_SET, SUPPORT_PLAY)
SUPPORT_SELECT_SOURCE,SUPPORT_SELECT_SOUND_MODE,

Choose a reason for hiding this comment

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

missing whitespace after ','

- fixed some too long lines and other houndci-bot issues
url = ('http://' + str(self._host) +
'/goform/formMainZone_MainZoneXml.xml')
XML_data = urllib.request.urlopen(url)
except URLError:

Choose a reason for hiding this comment

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

undefined name 'URLError'

fixed issue with except URLError
- fixed some naming issues
- fixed import order
@starkillerOG
Copy link
Contributor Author

sound_mode_support

@starkillerOG
Copy link
Contributor Author

sound_mode_support2

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare thanks for your help with the Hyperion pull request.
Do you have time to also look at this pull request for sound mode support for the media_player component?

Copy link
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Took a quick peek and it looks fair enough for me, although I would really like to have a more generic interface. To give an example of the settings my soundbar provides:
image

fields:
entity_id:
description: Name(s) of entities to change sound mode on.
example: 'media_player.media_player.txnr535_0009b0d81f82'
Copy link
Member

Choose a reason for hiding this comment

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

media_player.media_player

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in commit

description: Name(s) of entities to change sound mode on.
example: 'media_player.media_player.txnr535_0009b0d81f82'
sound_mode:
description: Name of the sound mode to switch to. Platform dependent.
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to have "Platform dependent" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it.

('AUTO', 'None'),
('DOLBY DIGITAL', 'DOLBY DIGITAL'),
('MCH STEREO', 'MULTI CH STEREO'),
('STEREO', 'STEREO')])
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that needs to be configurable / statically defined, or could it be fetched from the device?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, for now it needs to be statically defined because I could not figure out how to get that data from the receiver. At least it is not in the XML file that you can request from the receiver.
However in the official phone aplication of Marantz you can also change the sound mode and their is a list of sound modes their and it nows wich sound mode is currently set. Since this phone application is generic for all Marantz Receivers (that support ethernet), their must be a way to get this information. I am pretty sure the phone application uses normal http requests just like the build in web server does.

Theirfor it is probably possible, I just do not know how.
And at the moment I do not have enough time to figure this out.

It was already fairly difficult to get the whole dictionary working as a configurable option, because the voluptuous checker was not to happy with the syntax.
So for now I think we have to settle for this, but in the future it would definetly be a greath improvement to get this working using some sort of request to the receiver.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think it is fine (for now) ho hardcode that, although I think that the proper place for this is also inside denonavr library rather than here.

I understand that it can be quite hard to decipher what is going on, but assuming you want to look into it you may want to run tcpdump on your router to capture the traffic for later analysis with wireshark.

'/goform/formMainZone_MainZoneXml.xml')
xml_data = urllib.request.urlopen(url)
except urllib.error.URLError:
return
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a error/debug log output, if someone wonders why it is not working for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a log message if it fails, but I did not do logging befor. Do I need to define the LOGGING as a self. variable or is is recognized as I did it now? Also is the way I did it correct, or do I need to use some special thing for error/debug?

sound_mode = list(self._sound_mode_dict.keys())[mode_index]
self._current_sound_mode = sound_mode
except ValueError:
self._current_sound_mode = sound_mode_raw
Copy link
Member

Choose a reason for hiding this comment

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

IMHO protocol parsing should be done inside denonavr library and not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The denonavr library does not support sound mode. I have looked at that before I started doing this code myself in Home Assistant.
I do agree that it would be nicer to have this implemented in the denonavr library.
However I do not know the denonavr library at all and do not know any of the developers.
I looked at their code, but it is not as straightforward because they cover all kinds of exceptions and broke up the simple http requests in their big architecture.
Theirfore I chose to just implement it in HASS.

If anyone has the time and knowledge to implement this in denonavr library please do, you can use my code and I am willing to help. But it looked to complicated for me right now and I do not have the time at the moment.

However this code does work and I have tested it.

I propose we go with this for now and maybe someone has time to later implement this further in the denonavr library to get it a little neater.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should try to get this merged to denonavr, that's the normal procedure for homeassistant libs anyway, and I think no HA developer is very eager to merge such directly. Maybe you could ask if the authors of that lib are willing to help you to get it included? I think that would be the best way to handle this.

"""Select sound mode."""
url = ('http://' + str(self._host) +
'/MainZone/index.put.asp?cmd0=PutSurroundMode%2F'
+ sound_mode.upper().replace(" ", "+"))
Copy link
Member

Choose a reason for hiding this comment

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

Same as above wrt denonavr lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above

changed the discription of the service.
added a log message if the url get request fails to receive the XML file containing the sound mode.
'/goform/formMainZone_MainZoneXml.xml')
xml_data = urllib.request.urlopen(url)
except urllib.error.URLError:
_LOGGER.info("Denon receiver failed to get sound mode, URL-Error")

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

fixed line too long
changed loger.info to loger.error for the error message
@starkillerOG
Copy link
Contributor Author

@rytilahti Thanks for your feedback, I have implemented some adjustments based on your comments.
See my answers for detailed information about denonavr library and the configuration dictionary.

Do you agree with me?

Nice to see your configuration options of your sound bar.
I see they are indeed slightly diffrent.
May I ask what type of sound bar you use?
How did you manage to get those?
Is it a http request or something, it looks like a command prompt or something?

@starkillerOG
Copy link
Contributor Author

@rytilahti can you merge this PR, or does this code need more revieuws?

@starkillerOG
Copy link
Contributor Author

@rytilahti In general I made the frontend work and service.yaml and media_player init.py file changes generic so it can be implement for all platforms (also other than Marantz and denon). But the denonavr.py file changes are of course specific to the Marantz and denon devices. The configuration option (the dictionary) is also part of the specific denonavr.py component and not the general media_player files.
Other platforms like Onkyo and Sonos also have sound modes, and diffrent code will need to be developed to request the sound mode and sent the command for those devices.

@rytilahti
Copy link
Member

Nice to see your configuration options of your sound bar.
I see they are indeed slightly diffrent.
May I ask what type of sound bar you use?
How did you manage to get those?
Is it a http request or something, it looks like a command prompt or something?

That is a sony soundbar, which uses its own protocol for communicating (which offers a dynamic list of settings to use, all depending on what type of device it is), so that's one of the reasons I'm so hesitant for hardcoding things. The output is simply an output from the script I created: https://github.com/rytilahti/python-songpal .

Other platforms like Onkyo and Sonos also have sound modes, and diffrent code will need to be developed to request the sound mode and sent the command for those devices.

Yes, the sound mode code needs to be device specific. What I tried to point out with that screenshot is that there exists much more settings besides just "sound mode", which I would like to have exposed somehow by homeassitant.

@rytilahti can you merge this PR, or does this code need more revieuws?

As I'm not familiar with the mediaplayer code I'll leave it to others to decide, I just wanted to help with a brief review when I saw your message earlier on discord. Anyway, I think the response will be that the device-specific code must go to a library (as per: https://home-assistant.io/developers/add_new_platform/#interfacing-with-devices).

@starkillerOG
Copy link
Contributor Author

Thanks for all your help @rytilahti

@starkillerOG
Copy link
Contributor Author

@balloob @fabaff Could one of you maybe revieuw this pull request and then merge it?

@rytilahti
Copy link
Member

This will require getting those changes I mentioned into denonavr first, I think after that this can be merged!

implement some changes that were done in HASS v0.63.2
implemented the changes made in HASS v0.63.2
@starkillerOG
Copy link
Contributor Author

@rytilahti I have just implemented the code in the denonavr library and created a PR:
Denonavr library PR
Hopfully this PR can soon be merged when I make the changes in HASS to use these new functions.

entity_id:
description: Name(s) of entities that will be restored. Platform dependent.
example: 'media_player.living_room'

Copy link
Member

Choose a reason for hiding this comment

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

These are not supposed to be part of this PR, right?

@@ -293,3 +317,30 @@ kodi_call_method:
method:
description: Name of the Kodi JSONRPC API method to be called.
example: 'VideoLibrary.GetRecentlyAddedEpisodes'

squeezebox_call_method:
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this is not for sound mode PR.

description: Optional array of parameters to be appended to the command. See 'Command Line Interface' official help page from Logitech for details.
example: '["loadtracks", "track.titlesearch=highway to hell"]'

yamaha_enable_output:
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@starkillerOG
Copy link
Contributor Author

@rytilahti Yes you are right, It got messed up when home assistant updated to v0.63.2.
Because some changes were made in the media_player component. So I coppied those changes to my fork to make it work again.
This PR is therefore a mess right now.
I am waiting till my PR in the denonavr gets accepted, then I will update my fork to use the new denonavr library funcionality, then I will rebase and close this PR and make a new one to have a clean PR that can be merged.

@starkillerOG
Copy link
Contributor Author

@rytilahti
I opened a new PR, that is now using the new denonavr library v.0.7.0 (my code for the sound mode support is merged in the new denonavr v.0.7.0)
The new PR
It schould be fully ready to be merged now.

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants