Skip to content

Add more configuration options for the Harman Kardon AVR component#23569

Closed
Devqon wants to merge 6 commits intohome-assistant:devfrom
Devqon:hkavr-sources
Closed

Add more configuration options for the Harman Kardon AVR component#23569
Devqon wants to merge 6 commits intohome-assistant:devfrom
Devqon:hkavr-sources

Conversation

@Devqon
Copy link
Copy Markdown
Contributor

@Devqon Devqon commented Apr 30, 2019

Description:

This allows the user to:

  • Configure the input sources of the AVR
  • Configure a key interval to sleep between the sending of the key commands
  • Simulate the volume setter of the AVR (This enables to control the volume through Google Assistant)

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

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: harman_kardon_avr
    host: IP_HERE
    key_interval: 0.3
    simulate_volume_set: True
    sources:
      - source: TV
        name: Television
      - source: Game
        name: Xbox One

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (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.

Note: There was a previous pull request, but I cannot re-open it: #23162

@Devqon Devqon changed the title Hkavr sources Add more configuration options for the Harman Kardon AVR component Apr 30, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 30, 2019

Codecov Report

Merging #23569 into dev will increase coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #23569      +/-   ##
==========================================
+ Coverage   93.83%    94.2%   +0.36%     
==========================================
  Files         462      453       -9     
  Lines       37686    36913     -773     
==========================================
- Hits        35363    34773     -590     
+ Misses       2323     2140     -183
Impacted Files Coverage Δ
homeassistant/components/mqtt/fan.py 74.19% <0%> (-23.78%) ⬇️
homeassistant/components/zha/core/registries.py 85% <0%> (-1.96%) ⬇️
homeassistant/components/hue/bridge.py 73.07% <0%> (-1%) ⬇️
homeassistant/components/zha/binary_sensor.py 92.3% <0%> (-0.97%) ⬇️
homeassistant/components/zha/core/discovery.py 92.1% <0%> (-0.76%) ⬇️
homeassistant/components/local_file/camera.py 95.91% <0%> (-0.58%) ⬇️
homeassistant/components/axis/binary_sensor.py 72.22% <0%> (-0.51%) ⬇️
homeassistant/components/hassio/auth.py 90.47% <0%> (-0.44%) ⬇️
homeassistant/helpers/service.py 91.58% <0%> (-0.4%) ⬇️
setup.py 86.41% <0%> (-0.25%) ⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d71424f...f55a01f. Read the comment docs.

def source_list(self):
"""Available sources."""
return self._source_list
return self._sources
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.

We should not allow overriding the sources via the config. What is wrong with the options that come from the API?

@property
def supported_features(self):
"""Flag media player features that are supported."""
if self._simulate_volume_set:
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.

We shouldn't add this.

name = config[CONF_NAME]
host = config[CONF_HOST]
port = config[CONF_PORT]
key_interval = config[CONF_KEY_INTERVAL]
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.

I don't see a use for this either, and it's only used for the fake volume? We should drop it.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 19, 2019

I don't think that each of the three features this PR aims to add belong in Home Assistant.

@balloob balloob closed this Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants