Skip to content

Add support of Zone2 and Zone3#8025

Merged
balloob merged 2 commits into
home-assistant:devfrom
ol-iver:denonavr
Jun 22, 2017
Merged

Add support of Zone2 and Zone3#8025
balloob merged 2 commits into
home-assistant:devfrom
ol-iver:denonavr

Conversation

@ol-iver
Copy link
Copy Markdown
Contributor

@ol-iver ol-iver commented Jun 14, 2017

Description:

Support for Zone2 and Zone3 of the the receiver was added. If configured, new instances of media_player are created for the additional zones.

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

Example entry for configuration.yaml (if applicable):

# media player
media_player:
  - platform: denonavr
    host: 192.168.0.250
    name: receiver living room
    zones:
      - zone: Zone2
      - zone: Zone3
        name: balcony

Checklist:

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.

@mention-bot
Copy link
Copy Markdown

@scarface-4711, thanks for your PR! By analyzing the history of the files in this pull request, we identified @happyleavesaoc, @fabaff and @balloob to be potential reviewers.



DENON_ZONE_SCHEMA = vol.Schema({
vol.Required(CONF_ZONE): denon_zone,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of writing your own validator, you can use vol.In(CONF_VALID_ZONES).

cache = hass.data[KEY_DENON_CACHE] = set()

# Get config option for show_all_sources
show_all_sources = config.get(CONF_SHOW_ALL_SOURCES) if not None else False
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't work ? also, there is already a default value specified for CONF_SHOW_ALL_SOURCES so also not necessary.


# Get config option for additional zones
zones = config.get(CONF_ZONES)
if zones is not None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Zones is never None since you pass it via ensure_list.

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.

The whole zones tag is optional. It could be left out completely , if only an instance for Main Zone of the receiver should be created. In this case zones could be None. I just tested it.

if host not in cache:
new_device = denonavr.DenonAVR(
host, name, show_all_sources, add_zones)
for new_zone in new_device.zones.values():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any way you can abstract this out instead of having to copy paste it 3 times?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 22, 2017

Awesome!

@balloob balloob merged commit 2e3b279 into home-assistant:dev Jun 22, 2017
@ol-iver ol-iver deleted the denonavr branch June 24, 2017 16:08
@balloob balloob mentioned this pull request Jul 1, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
* Add support of Zone2 and Zone3

* Changes from balloobs feedback
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
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.

5 participants