Skip to content

Onkyo: volume control fixes#14053

Closed
rsmeral wants to merge 2 commits intohome-assistant:devfrom
rsmeral:onkyo-volume-control-fixes
Closed

Onkyo: volume control fixes#14053
rsmeral wants to merge 2 commits intohome-assistant:devfrom
rsmeral:onkyo-volume-control-fixes

Conversation

@rsmeral
Copy link
Copy Markdown
Contributor

@rsmeral rsmeral commented Apr 22, 2018

Description:

Two fixes for Onkyo media player:

  • Added support for volume steps
    • the underlying library directly supports it and translates to a receiver-native single unit of volume
    • without this fix, volume up/down is subject to the default media_player behaviour of stepping by 10% which seems too coarse for most receivers
  • Serialized command execution to prevent connection errors
    • for example, when the volume_up (or any other) service was called repeatedly in a very fast succession (e.g. when pressing a button on a remote), I'd frequently get a Resetting connection... error
    • this happened most likely because the next command was sent to the receiver before the response from the first one was received
    • this fix corrects that by introducing a queue for service calls (using ThreadPoolExecutor with a single worker)

Disclaimer: I'm not much of a pythonista. Tried running tests, got:

Results (236.02s):
    3540 passed
     100 skipped

But also:

ERROR:   lint: commands failed
ERROR:   typing: commands failed

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @rsmeral,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

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)

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 (97 > 79 characters)


def __init__(self, receiver, sources, name=None):
"""Initialize the Onkyo Receiver."""
from concurrent.futures import ThreadPoolExecutor
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.

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

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

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.

@rsmeral rsmeral mentioned this pull request May 5, 2018
2 tasks
@andrey-git
Copy link
Copy Markdown
Contributor

@rsmeral Any updates on this PR?

@rsmeral
Copy link
Copy Markdown
Contributor Author

rsmeral commented May 12, 2018

@andrey-git One part got merged (#14299), but the second part (command serialization) I didn't find the time for yet – I agree with the approach proposed by @balloob, but it's non-trivial for me to implement given my limited experience with Python.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 1, 2018

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

@balloob balloob closed this Jun 1, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants