Skip to content

Added sound mode support#43

Merged
ol-iver merged 17 commits intool-iver:masterfrom
starkillerOG:patch-1
Apr 4, 2018
Merged

Added sound mode support#43
ol-iver merged 17 commits intool-iver:masterfrom
starkillerOG:patch-1

Conversation

@starkillerOG
Copy link
Copy Markdown
Contributor

new functions to use:

  • denonavr.sound_mode (gives the current matched sound mode (after an denonavr.update))
  • denonavr.sound_mode_raw (gives the current sound mode as received from the AVR (after an denonavr.update))
  • denonavr.sound_mode_list (gives a list of supported sound mode commands)
  • denonavr.sound_mode_dict (gives the full dictionarry containing all supported commands including their matching values)
  • denonavr.set_sound_mode(VALUE) (change the sound mode of the AVR using one of the VALUE's of the denonavr.sound_mode_list)
  • denonavr.set_sound_mode_dict (change the matching sound mode dict, this will throw an error if the wrong syntax is used with an example of a correct syntax)

new functions to use:
- denonavr.sound_mode (gives the current matched sound mode (after an denonavr.update))
- denonavr.sound_mode_raw (gives the current sound mode as received from the AVR (after an denonavr.update))
- denonavr.sound_mode_list (gives a list of supported sound mode commands)
- denonavr.sound_mode_dict (gives the full dictionarry containing all supported commands including their matching values)
- denonavr.set_sound_mode(VALUE) (change the sound mode of the AVR using one of the VALUE's of the denonavr.sound_mode_list)
- denonavr.set_sound_mode_dict (change the matching sound mode dict, this will throw an error if the wrong syntax is used with an example of a correct syntax)
Comment thread denonavr/denonavr.py Outdated
return sound_mode
except ValueError:
pass
_LOGGER.warning("Not able to match sound mode, returning raw sound mode.")
Copy link
Copy Markdown

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)

Comment thread denonavr/denonavr.py Outdated
else:
_LOGGER.error(Error_msg)
return False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

Comment thread denonavr/denonavr.py Outdated
return False

def set_sound_mode_dict(self, sound_mode_dict):
Error_msg = "Syntax of sound mode dictionary not valid, use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (117 > 79 characters)

Comment thread denonavr/denonavr.py Outdated
# Set all tags to be evaluated
relevant_tags = {"Power": None, "InputFuncSelect": None, "Mute": None,
"MasterVolume": None}
"MasterVolume": None, "selectSurround": None, "SurrMode": None}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (88 > 79 characters)

fixed houndci-bot typo's
Comment thread denonavr/denonavr.py Outdated

def set_sound_mode_dict(self, sound_mode_dict):
Error_msg = "Syntax of sound mode dictionary not valid, "
"use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IndentationError: unexpected indent
unexpected indentation

Comment thread denonavr/denonavr.py Outdated

def set_sound_mode_dict(self, sound_mode_dict):
Error_msg = ("Syntax of sound mode dictionary not valid, "
"use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 could you please merge this PR?
It would be greath if you could then push to the next version of denonavr, such that I can use the new sound mode implementation in my PR for HomeAssistant.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 could you please merge this PR?
I am waiting until this gets merged, before I can continue my work with the sound mode support in HomeAssistant and I would really like to finisch this.
It would be greath if you could find the time to accept this PR.

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Mar 25, 2018

@starkillerOG , was on vacation and did have lots of work before. I was not able to test your changes until now.

Setting sound modes using the values from sound_mode_list is working fine. The modes for setting seem to be the same over several receiver models.

The mapping of sound modes does not work for my receiver. Every time I run the update method there is a Not able to match sound mode, returning raw sound mode. warning, because the mapping of my receiver is totally different than yours. When you look at other XML files from the tests/xml subdirectory of this repository, many other models will have the same issue.
Thus, please think about a more generic mapping (which would be a huge mapping table, as I wrote in your issue thread) or remove it and use the raw values instead.

Comment thread denonavr/denonavr.py Outdated
self._sound_mode = self.match_sound_mode(sound_mode_raw)
self._sound_mode_raw = sound_mode_raw
if "selectSurround" in relevant_tags.keys():
relevant_tags.pop("selectSurround", None)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you pop both keys from the list, when the first one was found?
They seem to cover the same information. At the moment there might be a second HTTP request, which is not needed, because the STATUS_URL is called first and there is a second call to MAINZONE_URL if some tags are not found yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am poping both keys when the first one is found.
when one of the keys is found elif (child.tag == "selectSurround" or child.tag == "SurrMode"): , both keys are poped using relevant_tags.pop("selectSurround", None) and relevant_tags.pop("SurrMode", None).
The if "selectSurround" in relevant_tags.keys(): and if "SurrMode" in relevant_tags.keys(): will in principle always return true, however I put those in to prevent the occasion that later when the code is changed there is no problem with trying to remove a key that does not exist. In the case that if "selectSurround" in relevant_tags.keys(): or if "SurrMode" in relevant_tags.keys(): returns false, the key is not in the list anyway so their is no need to pop it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Therefore I think this part of the code is correct, and a second HTTP request is not possible unless none of the two keys ("selectSurround" and "SurrMode") are found.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ah ok, I was confused by the if statement.
Btw. you don't need the if statements here. If you pop with an default value, there won't be an exception even if the key is not in the list

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 The mapping is indeed quite difficult.
I have searched in all xml files i could find from my receiver but I cannot find the exact spelling of the returned sound mode anywhere. Therefore it seems not possible to get the mapping list from the receiver, I have tryed.

I am happy to include more mapping values and make the mapping structure bigger (just fill in the list), This will make this list pretty big indeed. The problem is that I do not know the values that are returnd. For my receiver I just asked for all the sound modes I use and then put the returned values in the mapping list. However if I compare my retrun values with list such as:
Marantz Command list
Marantz Command list 2
They do not match.
For instance the return value for 'MCH STEREO' is for me 'MULTI CH STEREO', but that is not in any of the above documentation.
Therefore I would need the return values for diffrent types of receivers (if you know them I can included them in the list).

Note however that I implemented the option to change the mapping list:
denonavr.set_sound_mode_dict(DICT_TO_USE)
I also made an optional configuration parameter in my HomeAssistant PR so people can change the mapping list inside HomeAssistant to adapt it to match their type of receiver.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 The raw values can always be obtained using:
denonavr.sound_mode_raw()
(after an update)
I do need the matched values for HomeAssistant, because the input list wil otherwise not show the correct sound mode.
What I will do is move the warning from the denonavr.update() to the denonavr.sound_mode(), such that the warning is not shown if you do not use the matched values but only the raw values.

Moved the warning if the sound mode can not be matched from the update function to when you call the matched sound mode.
Added all the sound mode values that were present in denonavr/tests/xml/
Comment thread denonavr/denonavr.py Outdated
"Flickr": "FLICKR", "Favorites": "FAVORITES"}

SOUND_MODE_MAPPING = OrderedDict([('MUSIC', ['PLII MUSIC']),
('MOVIE', ['PLII MOVIE','PLII CINEMA',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing whitespace after ','

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 I added all the sound mode values that were present in denonavr/tests/xml/.
Could you provide me with any aditional sound mode values for receivers that you know/own?

I suggest we merge this PR, and over time add more mapping values to the list when people owning a diffrent model open an issue for missing mapping values. In the mean time those people can customly define their mapping values with denonavr.set_sound_mode_dict(DICT_TO_USE), or when using HomeAssistant define it in the options of HomeAssistant.

Copy link
Copy Markdown
Owner

@ol-iver ol-iver left a comment

Choose a reason for hiding this comment

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

Please check my remarks. I'll check additional sound mode of my receiver.

Comment thread denonavr/denonavr.py Outdated
relevant_tags.pop(child.tag, None)
elif (child.tag == "selectSurround" or child.tag == "SurrMode"):
sound_mode_raw = child[0].text.rstrip()
self._sound_mode = self.match_sound_mode(sound_mode_raw)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you move the match_sound_mode to the sound_mode property?
You won't need the self. _sound_mode_match_warning attribute then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

just did it.

Comment thread denonavr/denonavr.py Outdated
self._sound_mode = self.match_sound_mode(sound_mode_raw)
self._sound_mode_raw = sound_mode_raw
if "selectSurround" in relevant_tags.keys():
relevant_tags.pop("selectSurround", None)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ah ok, I was confused by the if statement.
Btw. you don't need the if statements here. If you pop with an default value, there won't be an exception even if the key is not in the list

Comment thread denonavr/denonavr.py Outdated

def match_sound_mode(self, sound_mode_raw):
try:
mode_list = list(self._sound_mode_dict.values())
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

When I look at these lines, I get the impression you created the SOUND_MODE_MAPPING and self._sound_mode_dict the wrong way and mixed key and values.
You are using it to get the sound modes from the raw values. For me this is a normal dictionary with each sound_mode_raw as key and its sound_mode as value. That would simplify this part here a lot.
sound_mode_list would be a set set(self._sound_mode_dict.values()) then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In principle you would be right, and it would be nicer to do it that way if each sound_mode_raw would correspond to exactly 1 matched sound mode.
Unfortunately this is not the case, and multiple sound_mode_raw's can be matched to the same matched sound_mode.

I did think about this before I set it up, and in my opinion it is more elegant to do it like I did it because now I have a single matched sound_mode as key, and then all corresponding raw_sound_modes as list in the value corresponding to that key. In this way I do not need to repeat the matched sound_mode as the value for several raw_sound_mode keys (if you would do it the other way around).

Therefore my method seemed more compact and in my opinion more elegant for this situation.

Copy link
Copy Markdown
Contributor Author

@starkillerOG starkillerOG Apr 2, 2018

Choose a reason for hiding this comment

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

In principle I chose for keeping the self._sound_mode_dict as compact and most eligant as I could make it, and having a bit more complicated code in the match_sound_mode function.

I did this because users of this library and Home Assistant will probably want to tweak the self._sound_mode_dict to have the exact mapping values for their model of receiver, if it is not yet incorperated in the default mapping dict.
However those users will in principle never have to deal with the code of the match_sound_mode function, so it is no problem if that gets a bit more complicated.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It does not look too efficient to replace a dictionary key access by a nested loop across all values of a dictionary during run time.

Maintaining own mapping rules is indeed easier using your way of modelling.

Thus, I would use a dictionary with key: sound_mode_raw and value: sound_mode during run time as I suggested.
This dictionary could be created by the set_sound_mode_dict method you already created from a dictionary with the structure you suggested (key: sound_mode, value: list of sound_mode_raw)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@scarface-4711 smart, it is indead more efficient to use a simple dict key acces during runtime, I have implemented this change in the code.

moved match_sound_mode to sound_mode property.
Therefore I could get rid of self._sound_mode and self. _sound_mode_match_warning.
@starkillerOG
Copy link
Copy Markdown
Contributor Author

starkillerOG commented Apr 2, 2018

@scarface-4711 shall I remove the if "selectSurround" in relevant_tags.keys(): check's, or just keep them just to be sure?

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Apr 2, 2018

The sound modes are different at my receiver
When I switch to MOVIE, MUSIC and GAME the sound_mode will be the one I have chosen the last time on my remote control for this category.
"Dolby D + Dolby Surround", "Dolby Digital", "Multi Ch Stereo", "Stereo", "Virtual" could be assigned from each of those three categories.

There are other sound modes which are specific to one category:
"Dolby D +Neo:X C": MOVIE
"Dolby D +Neo:X M": MUSIC
"Dolby D +Neo:X G": GAME
"Rock Arena": MUSIC
"Jazz Club": MUSIC
"Matrix": MUSIC
"Mono Movie": MOVIE
"Video Game": GAME

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Apr 2, 2018

@starkillerOG : you can remove the if "selectSurround" in relevant_tags.keys()

Added aditional sound mode mapping values
removed the if tag in list checks for the pop function.
Comment thread denonavr/denonavr.py Outdated
"Media Server": "SERVER", "Spotify": "SPOTIFY",
"Flickr": "FLICKR", "Favorites": "FAVORITES"}

SOUND_MODE_MAPPING = OrderedDict \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

whitespace before '('

instead of looping over the original nice syntax self._sound_mode_dict, a second dict is created based on self._sound_mode_dict, this new dict is called self._SM_match_dict.
In this bigger dictionary the key value structure is reversed (but it contains duplicate values).
This self._SM_match_dict is used to match the raw sound mode with a simple dict value request based on key.
Comment thread denonavr/denonavr.py Outdated
def set_sound_mode_dict(self, sound_mode_dict):
Error_msg = ("Syntax of sound mode dictionary not valid, "
"use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])")
if (type(sound_mode_dict) == OrderedDict or type(sound_mode_dict) == dict):
Copy link
Copy Markdown

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)

Comment thread denonavr/denonavr.py Outdated

@property
def sound_mode_match_dict(self):
"""Return a dictionary that is used to map each sound_mode_raw to it's matched sound_mode."""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (101 > 79 characters)

Comment thread denonavr/denonavr.py Outdated

@property
def sound_mode_dict(self):
"""Return a dictionary of available sound modes with their mapping values."""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

Comment thread denonavr/denonavr.py Outdated
Error_msg = ("Syntax of sound mode dictionary not valid, "
"use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])")
if (type(sound_mode_dict) == OrderedDict or\
type(sound_mode_dict) == dict):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

visually indented line with same indent as next logical line

Comment thread denonavr/denonavr.py Outdated
def set_sound_mode_dict(self, sound_mode_dict):
Error_msg = ("Syntax of sound mode dictionary not valid, "
"use: OrderedDict([('COMMAND', ['VALUE1','VALUE2'])])")
if (type(sound_mode_dict) == OrderedDict or\
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the backslash is redundant between brackets

Comment thread denonavr/denonavr.py Outdated
@property
def SM_match_dict(self):
"""
Return a dictionary that is used to
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

trailing whitespace

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 I implemented all your change requests. Thanks for the feedback.
I also included all the mapping values of your receiver.

Your receiver works the same as mine, I can also define "stereo" from each of the menu's (MUSIC, MOVIE, GAME etc. However this is actually the same sound mode regardless from wich menu you selected it. You can see this if you look at the phone application or at the web-interface of the receiver. Their the sound mode menu's are organized in a diffrent way and their there is only 1 "stereo" option.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 could you have a final look at the code and give any aditional comments or merge it?

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Apr 3, 2018

@starkillerOG changes are looking good.

There are some small comments and warnings from pylint and pydocstyle. Could you please fix them? I'll push a new version for you tomorrow evening then.
Thanks.

PS C:\Users\Oliver\python-venv\python35-testing\denonavr-patch-1> pylint denonavr
No config file found, using default configuration
************* Module denonavr.denonavr
C:1056, 0: Unnecessary parens after 'elif' keyword (superfluous-parens)
C:1306, 0: Wrong continued indentation before block (add 4 spaces).
            type(sound_mode_dict) == dict):
            ^   | (bad-continuation)
W:1307, 0: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
W:1308, 0: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
W:1309, 0: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
W:1310, 0: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
W:1311, 0: Bad indentation. Found 28 spaces, expected 24 (bad-indentation)
W:1312, 0: Bad indentation. Found 32 spaces, expected 28 (bad-indentation)
W:1313, 0: Bad indentation. Found 32 spaces, expected 28 (bad-indentation)
W:1314, 0: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
W:1315, 0: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
W:1316, 0: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
C:239, 8: Invalid attribute name "_SM_match_dict" (invalid-name)
C:1155, 4: Invalid attribute name "SM_match_dict" (invalid-name)
C:1302, 4: Missing method docstring (missing-docstring)
C:1303, 8: Invalid variable name "Error_msg" (invalid-name)
C:1305,12: Using type() instead of isinstance() for a typecheck. (unidiomatic-typecheck)
C:1306,12: Using type() instead of isinstance() for a typecheck. (unidiomatic-typecheck)
C:1309,23: Using type() instead of isinstance() for a typecheck. (unidiomatic-typecheck)
C:1311,31: Using type() instead of isinstance() for a typecheck. (unidiomatic-typecheck)
C:1324, 4: Invalid method name "construct_SM_match_dict" (invalid-name)
C:1324, 4: Missing method docstring (missing-docstring)
R:1324, 4: Method could be a function (no-self-use)
C:1332, 4: Missing method docstring (missing-docstring)
W:1293,16: Attribute '_sound_mode' defined outside __init__ (attribute-defined-outside-init)

-------------------------------------------------------------------
Your code has been rated at 9.72/10 (previous run: 10.00/10, -0.28)

PS C:\Users\Oliver\python-venv\python35-testing\denonavr-patch-1> pydocstyle denonavr
denonavr\denonavr.py:1147 in public method `sound_mode_dict`:
        D205: 1 blank line required between summary line and description (found 0)
denonavr\denonavr.py:1147 in public method `sound_mode_dict`:
        D400: First line should end with a period (not 's')
denonavr\denonavr.py:1155 in public method `SM_match_dict`:
        D205: 1 blank line required between summary line and description (found 0)
denonavr\denonavr.py:1155 in public method `SM_match_dict`:
        D400: First line should end with a period (not 'o')
denonavr\denonavr.py:1302 in public method `set_sound_mode_dict`:
        D102: Missing docstring in public method
denonavr\denonavr.py:1324 in public method `construct_SM_match_dict`:
        D102: Missing docstring in public method
denonavr\denonavr.py:1332 in public method `match_sound_mode`:
        D102: Missing docstring in public method
PS C:\Users\Oliver\python-venv\python35-testing\denonavr-patch-1> flake8 denonavr
denonavr\denonavr.py:40:33: E211 whitespace before '('
denonavr\denonavr.py:1306:13: E129 visually indented line with same indent as next logical line
PS C:\Users\Oliver\python-venv\python35-testing\denonavr-patch-1>

Comment thread denonavr/denonavr.py Outdated
# sent command
try:
if self.send_get_command(command_url):
self.sound_mode() = sound_mode
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SyntaxError: can't assign to function call

Comment thread denonavr/denonavr.py Outdated
"Flickr": "FLICKR", "Favorites": "FAVORITES"}

SOUND_MODE_MAPPING = OrderedDict\
([('MUSIC', ['PLII MUSIC', 'DTS NEO:6 MUSIC', 'DOLBY D +NEO:X M',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line missing indentation or outdented

Comment thread denonavr/denonavr.py Outdated
"Media Server": "SERVER", "Spotify": "SPOTIFY",
"Flickr": "FLICKR", "Favorites": "FAVORITES"}

SOUND_MODE_MAPPING = OrderedDict(\
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the backslash is redundant between brackets

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 I think I fixed all small comments and warnings.
Could you check again, I do not know how I can check it.

@ol-iver ol-iver merged commit f2d9087 into ol-iver:master Apr 4, 2018
@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Apr 4, 2018

@starkillerOG version 0.7.0 is now available on pypi including your sound mode support.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 Thank you!
I have my HomeAssistant code almost ready, will push a PR tommorow using this new version.
I just tested all the sound mode functions in version 0.7.0 and it seems to work perfectly.
Thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants