Skip to content

Add support sound mode boolean#50

Merged
ol-iver merged 3 commits intool-iver:masterfrom
starkillerOG:patch-2
Jul 8, 2018
Merged

Add support sound mode boolean#50
ol-iver merged 3 commits intool-iver:masterfrom
starkillerOG:patch-2

Conversation

@starkillerOG
Copy link
Copy Markdown
Contributor

Add a function and property to check if the receiver supports sound modes. (check if the status XML contains information about the sound mode).
This is required for HomeAssistant, see home-assistant/core#14910

@starkillerOG
Copy link
Copy Markdown
Contributor Author

starkillerOG commented Jun 13, 2018

@scarface-4711 can you give feedback on this PR?
I need a method of checking if the receiver supports sound modes that I can run once when HomeAssistant initializes.
If you find this okay, can you merge it and push a new version so I can use that in HomeAssistant?

The PR's for general sound mode support in the backend and frontend have already been merged and will be available in the next HomeAssistant version.
I hope I can finisch the denonAVR implementation this week, so it can be merged into the same new version of HomeAssistant.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

I got conformation that the HomeAssistant PR can be merged as soon as this PR is finisched.
Thank you for all you help throughout this project @scarface-4711, sound mode support is almost finished.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 could you please have a look at this PR?

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Jul 1, 2018

@starkillerOG Could you please include the check to _update_avr method in a way that there will be no error messages during update if sound mode tags are not in the XML?

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Jul 1, 2018

I did not have too much time to check this during Football World Cup so far 😉

Comment thread denonavr/denonavr.py Outdated

# Sound mode information only available in main zone
if self._zone == "Main":
if (self._zone == "Main" and self._support_sound_mode == True):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

comparison to True should be 'if cond is True:' or 'if cond:'

Comment thread denonavr/denonavr.py Outdated
self._get_zone_name()
else:
self._get_receiver_name()
# Determine if sound mode is supported
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

I implemented the check (_get_support_sound_mode()) at initialization, and subsecently prevented the sound mode tags to be added to relevant tags in the _update_avr() method if the initial _get_support_sound_mode() resulted in a False value.

This schould prevent errors in the _update_avr() method.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 Could you check why the Travis check failed?
seems to me like their is some sort of version conflict between pytest 3.4.2 and pytest 3.6.0.
But I think this has nothing to do with my code.
Could you check?

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 is their something wrong with my code or with the Travis check?

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Jul 7, 2018

Probably a new version of pytest-timeout, which is not compatible to the pytest version.

Please go to https://github.com/scarface-4711/denonavr/blob/master/test-requirements.txt , replace pytest-timeout>=1.2.1 with pytest-timeout==1.2.1 and try again

@ol-iver ol-iver merged commit 16c9942 into ol-iver:master Jul 8, 2018
@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Jul 8, 2018

@starkillerOG just did it by myself and published version 0.7.4

@starkillerOG
Copy link
Copy Markdown
Contributor Author

Thank you @scarface-4711 for all your help!

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@scarface-4711 The sound mode support is merged in HomeAssistant, so with the next release in two weeks everyone schould have sound mode support in HomeAssistant (frontend and backend).

@ol-iver
Copy link
Copy Markdown
Owner

ol-iver commented Jul 9, 2018

great, I'll test it then 👍

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