Skip to content

Implemented basic async volume transition for media_player#6766

Closed
PetePriority wants to merge 2 commits into
home-assistant:devfrom
PetePriority:volume_transition_async
Closed

Implemented basic async volume transition for media_player#6766
PetePriority wants to merge 2 commits into
home-assistant:devfrom
PetePriority:volume_transition_async

Conversation

@PetePriority
Copy link
Copy Markdown
Contributor

@PetePriority PetePriority commented Mar 23, 2017

Description:

This PR implements a simple volume_transition service for the media_player platform. The service calls async_volume_transition which starts a coroutine _async_transition_helper.

Since I'm new with asyncio, I'd appreciate feedback on my implementation. One problem I see is that when the service gets called while another transition is already running, they'd interfere. Is there a way to cancel an already running coroutine? Another solution would be to have a class-owned threading.Timer which calls itself after the interval, but this would require __init__ for the platform and super-calls from all platform components.

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

@mention-bot
Copy link
Copy Markdown

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

@PetePriority PetePriority changed the title Implemented basic async volume transition Implemented basic async volume transition for media_player Mar 23, 2017
@emlove
Copy link
Copy Markdown
Contributor

emlove commented Mar 27, 2017

Also we'd need to abort the transition if the user changes the volume externally during a transition. We have the same issues here as implementing software dimmer transitions. There's a thread about this somewhere, but I can't seem to find it.

Much later edit, for posterity: #656

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Mar 29, 2017

I don't think we should add logic on base stuff. At last we only represent a device a not extend it. For that we have automation. I don't think we should do that. CC @balloob ?

@PetePriority
Copy link
Copy Markdown
Contributor Author

This branch shows an example of cancelable transitions. I had to make changes to the set_volume_level and __init__ of the mpd component, which might be a bit too invasive, as this would have to be done with every component.

Alternatively, we could move the functionality to a separate "fader" module that calls the set_volume service of specified media players. Maybe that would be a better approach?

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 31, 2017

I'm not a fan of this either. @armills is right. It will add too much complexity and eventually will end up with a pile of workarounds and hacks.

Some alternatives to achieve this:

  • Create a script that calls set volume with incremental volumes.
  • Create a custom component that calls the services for you
  • Add an optional transition parameter to async_set_volume_level. It is then up to the platforms to implement transition. They can choose to do this with a generic volume helper transition method. That way we leave the power to the platforms to create this the way they want it.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Apr 7, 2017

@PetePriority I close this PR. You can choise a way they Paulus describe in his comments.

@pvizeli pvizeli closed this Apr 7, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Jul 17, 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.

6 participants