Skip to content

Onkyo: volume control fixes #14053

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.

Sign up for GitHub

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

Closed
rsmeral wants to merge 2 commits into from
Closed

Onkyo: volume control fixes #14053

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e08f850
Onkyo: serialize command execution to prevent connection errors
rsmeral Apr 22, 2018
5fec0d4
Onkyo: SUPPORT_VOLUME_STEP
rsmeral Apr 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions homeassistant/components/media_player/onkyo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@

from homeassistant.components.media_player import (
SUPPORT_TURN_OFF, SUPPORT_TURN_ON, SUPPORT_VOLUME_MUTE, SUPPORT_VOLUME_SET,
SUPPORT_SELECT_SOURCE, SUPPORT_PLAY, MediaPlayerDevice, PLATFORM_SCHEMA)
SUPPORT_VOLUME_STEP, SUPPORT_SELECT_SOURCE, SUPPORT_PLAY,
MediaPlayerDevice, PLATFORM_SCHEMA)
from homeassistant.const import (STATE_OFF, STATE_ON, CONF_HOST, CONF_NAME)
import homeassistant.helpers.config_validation as cv

Expand All @@ -27,7 +28,8 @@
DEFAULT_NAME = 'Onkyo Receiver'

SUPPORT_ONKYO = SUPPORT_VOLUME_SET | SUPPORT_VOLUME_MUTE | \
SUPPORT_TURN_ON | SUPPORT_TURN_OFF | SUPPORT_SELECT_SOURCE | SUPPORT_PLAY
SUPPORT_VOLUME_STEP | SUPPORT_TURN_ON | SUPPORT_TURN_OFF | \
SUPPORT_SELECT_SOURCE | SUPPORT_PLAY

KNOWN_HOSTS = [] # type: List[str]
DEFAULT_SOURCES = {'tv': 'TV', 'bd': 'Bluray', 'game': 'Game', 'aux1': 'Aux1',
Expand Down Expand Up @@ -82,6 +84,8 @@ class OnkyoDevice(MediaPlayerDevice):

def __init__(self, receiver, sources, name=None):
"""Initialize the Onkyo Receiver."""
from concurrent.futures import ThreadPoolExecutor
Copy link
Copy Markdown
Member

@balloob balloob Apr 22, 2018

Choose a reason for hiding this comment

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

Oh no. This is waaay too heavy of a solution. If you would want 1 thread, you should start just a thread, not introduce a whole pool. However, even a thread is not allowed to be added. Imagine all our integrations include a thread, we would swamp the CPU.

So instead, you should use asyncio and locks (Home Assistant core is based on asyncio). Asyncio tasks waiting for a lock don't take up any resources. It means that you need to convert all commands to their async variant and include the lock. Once lock has been acquired asynchronously, use the built-in worker pool to run a threaded command: await hass.async_add_job(self.execute_command, command).

Copy link
Copy Markdown
Contributor Author

@rsmeral rsmeral Apr 22, 2018

Choose a reason for hiding this comment

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

Thanks for review. Good point, I'll try to rework.

If the other commit (SUPPORT_VOLUME_STEP) looks OK, I'll make it a separate PR.

Copy link
Copy Markdown
Member

@balloob balloob Apr 23, 2018

Choose a reason for hiding this comment

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

Yeah that one is fine.


self._receiver = receiver
self._muted = False
self._volume = 0
Expand All @@ -93,7 +97,9 @@ def __init__(self, receiver, sources, name=None):
self._source_mapping = sources
self._reverse_mapping = {value: key for key, value in sources.items()}

def command(self, command):
self._executor = ThreadPoolExecutor(max_workers=1)

def execute_command(self, command):
"""Run an eiscp command and catch connection errors."""
try:
result = self._receiver.command(command)
Expand All @@ -107,6 +113,11 @@ def command(self, command):
return False
return result

def command(self, command):
"""Add a command to the execution queue and wait for a result."""
future = self._executor.submit(self.execute_command, command)
return future.result()

def update(self):
"""Get the latest state from the device."""
status = self.command('system-power query')
Expand Down Expand Up @@ -186,6 +197,14 @@ def set_volume_level(self, volume):
"""Set volume level, input is range 0..1. Onkyo ranges from 1-80."""
self.command('volume {}'.format(int(volume*80)))

def volume_up(self):
"""Increase volume by 1 step."""
self.command('volume level-up')

def volume_down(self):
"""Decrease volume by 1 step."""
self.command('volume level-down')

def mute_volume(self, mute):
"""Mute (true) or unmute (false) media player."""
if mute:
Expand Down Expand Up @@ -251,6 +270,14 @@ def set_volume_level(self, volume):
"""Set volume level, input is range 0..1. Onkyo ranges from 1-80."""
self.command('zone2.volume={}'.format(int(volume*80)))

def volume_up(self):
"""Increase volume by 1 step."""
self.command('zone2.volume=level-up')

def volume_down(self):
"""Decrease volume by 1 step."""
self.command('zone2.volume=level-down')

def mute_volume(self, mute):
"""Mute (true) or unmute (false) media player."""
if mute:
Expand Down