8000 supported_features Vs current_supported_features · Issue #1 · home-assistant/architecture · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

supported_features Vs current_supported_features #1

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

Jump to bottom
Closed
lucasweb78 opened this issue Feb 4, 2018 · 15 comments
Closed

supported_features Vs current_supported_features #1

lucasweb78 opened this issue Feb 4, 2018 · 15 comments

Comments

@lucasweb78
Copy link

Currently Entity exposes supported_features that has become overloaded to mean both:

  • all of the entities supported features
  • the current set of supported features based on the entities state

The second use case breaks Alexa Discovery which relies on supported_features to discover all of the capabilities of an entity not just the ones that are available in it's current state.

One proposal from @balloob is to use supported_features as the list of all features and introduce a new current_supported_features that will reflect the features available at the current time base on the players state.
@lucasweb78
Copy link
Author

Adding current_supported_features is fairly straight forward:

  • It could be added to Entity and default to supported_features
  • supported_features can be updated in components that currently make it dynamic to be static e.g. apple_tv
  • current_supported_features can be updated in components to expose current features dynamically e.g. apple_tv
  • The media player ui component can be updated to use current_supported_features
  • Any other components that rely on dynamic supported_features can also be updated

@lucasweb78 lucasweb78 mentioned this issue Feb 4, 2018
Merged
8 tasks
@balloob
Copy link
Member
balloob commented Feb 4, 2018

I wouldn't say it was a proposal from my part, it was merely an idea as part of a brainstorm.

I think that supported_features should explain the capabilities of a device. This can be used as a guide to which services can be used with which parameters. A write up of how all of these features map to services/parameters should be added to https://dev-docs.home-assistant.io

Things that are supported_feature "worthy":

  • Light that supports color (should it be type of color?)
  • Light that supports brightness

Sometimes however, we see that we don't need a supported_features flag because if something is supported can be derived from other things that are available in the state (ie if a climate entity has no swing_list, one cannot set the swing mode).

@lucasweb78
Copy link
Author

I agree that supported_features should explain the capabilities of a device.

I'm not sure I fully understand how this would work with also being able to derive features that are available in the state. Does this mean you would need to check both the supported_features and state to determine a devices capabilities, or would supported_features be calculated based on information on the state? Ideally we just want a single property that can be read to determine everything a device supports.

The other question is do we need a current_supported_features or active_features which specifies what a device supports in it's current state e.g. an idle media player does not support play/pause so the UI component should not show the play/pause button?

Updating the documentation makes sense and would be happy to help with that. I would also like to figure out if we can start moving components to using supported_features to describe all of the devices capabilities and what impact this has on components that expose supported_features based on current state e.g. Apple TV Media Player

@balloob
Copy link
Member
balloob commented Feb 7, 2018

I've thought about it a bit more and we should go for the first implementation: supported_features should be all of the entities supported features. We should not have our entity design be steered by the frontend.

For "active supported features" I want to make sure that we do not use the word "features" or the current flags as I don't want to conflate concerns. Maybe call it something as "player functionality" or something. It can still work with bit flags like supported features, just should be clearly differently called.

@lucasweb78
Copy link
Author

Sounds good to me. How do you normally manage a somewhat breaking change like this? I would like to start moving entities to ensuring supported_features list all capabilities especially given the impact on Alexia discovery.

I’m happy to work through the media players as I already have a PR for Apple TV and understand the impact this change has on the UI component (its mostly around hiding showing controls).

The options are:

  • add the new concept of active_player_functionality or similar and update the UI card to hide/show controls based on this first and then update the supported_features

  • update supported_features first and live with the media player card not hiding showing controls based on state (which is already the case for most of the media player implementations) and then implement the UI changes later

Similar decisions also need to be made for other entities that have cards which rely on dynamic supported_features to decide when to hide/show elements based on the entities current state

@balloob
8000 Copy link
Member
balloob commented Feb 8, 2018

I think that only the media player has the frontend depend on supported_features being dynamic.

To make the transition easier, I would expect us to indeed start with adding the new functionality attribute to the media player. We will have to look at the frontend to see what kind of checks we actually need.

Source code for media player is here

Some things can still be derived from supported_features, like turn on/turn off. I think that we just need prev track, next track, pause/play/stop (not completely sure what is going on here)

This was referenced Feb 8, 2018
Closed
Closed
@lucasweb78
Copy link
Author

The UI changes look fairly straight forward:

What is the difference between state-card-media_player.html and ha-media_player.html?

In HA it is a case of:

  • adding active_player_functionality (or other name) here
  • adding the new flags
  • Implementing support for active_player_functionality in the different media components

Questions:

  1. While I agree we should use new flags to avoid confusion, can we use the same values for the flags for the bitwise checks or do we need new values?

  2. Do we need to update all media components or is there a sensible default we can use in media_player/init.py?

@balloob
Copy link
Member
balloob commented Feb 9, 2018

  1. New values. Let's make it very clear that it's different. (also we start at 2^0 anyway)

  2. I guess update. Although I don't mind to not do that all in 1 PR.

@lucasweb78
Copy link
Author

I've started mapping media player support_features to services and am going to update the docs as a first step. I just got bitten by not understanding which services map to which supported_features and have had to submit another PR for the Alexa StepSpeaker to StepVolume integration.

I also start to define active_player_functionality. Am still a little unsure on how the bitwise values are assigned but will take a best guess and get a WIP out for feedback

@uchagani uchagani mentioned this issue Mar 1, 2018
Merged
7 tasks
@amelchio
Copy link
amelchio commented Mar 3, 2018

This seems like much ado about (almost) nothing. Can we just remove the dynamic features?

It is crazy to have each individual platform add/remove a bunch of SUPPORT_PLAY-like bits based on their current playing state. That will give a lot of corner cases with inconsistencies and bugs.

Rather than a generic framework, I think much of this could be solved by adding e.g. a first_track: True attribute when the Previous control should be disabled.

For now, though, I propose that we just ignore the issue and have a visible no-op Previous control in that situation. My DVD remote control always worked like that.

According to a survey by @lucasweb78, only three media players use dynamic controls so it will be a simple cleanup.

@lucasweb78
Copy link
Author

I've been thinking about this some more and tend to agree. I'm still working on improving the documentation for the media_player component so that it's clear what flag maps to which service but I am not sure about introducing a second set of flags just for managing the UI controllers.

It would be good to figure out a path forward as right now the Apple Media Player still doesn't have all of it's capabilities correctly discovered by Alexa (unless you run my PR home-assistant/core#12167 as a custom_component).

I've been running the above PR for a few weeks and not having the media_player controls in the UI disappear when it's not in use has not caused any problems and is actually, IMO, nicer as the UI component doesn't keep shifting in size and changing the layout.

@balloob
Copy link
Member
balloob commented Mar 4, 2018

👍 for making supported_features static across Home Assistant. It follows very much the "backend should not care nor try to solve frontend issues".
8000

@amelchio amelchio mentioned this issue Mar 4, 2018
Merged
2 tasks
@lucasweb78
Copy link
Author

It looks like we've reached a consensus on this and are moving the media_player platforms to using a static set of supported features (only plex is left cc @ryanm101). Any objections to closing this discussion?

@balloob balloob closed this as completed Mar 5, 2018
@ryanm101
Copy link
ryanm101 commented Mar 5, 2018

@lucasweb78 What needs done for plex?

@dgomes dgomes mentioned this issue Jun 7, 2018
Merged
8 tasks
@MartinHjelmare MartinHjelmare mentioned this issue Jun 8, 2018
Merged
2 tasks
@MartinHjelmare MartinHjelmare mentioned this issue Jul 19, 2018
Merged
8 tasks
@MartinHjelmare MartinHjelmare mentioned this issue Aug 12, 2018
Closed
8 tasks
@abarbaccia abarbaccia mentioned this issue Aug 21, 2018
Closed
elupus added a commit to elupus/home-assistant that referenced this issue Sep 7, 2018
@elupus
Support list should be static
d3a0e20 
home-assistant/architecture#1
@MartinHjelmare MartinHjelmare mentioned this issue Feb 1, 2019
Merged
5 tasks
@amelchio amelchio mentioned this issue Mar 29, 2019
Merged
8 tasks
@MartinHjelmare MartinHjelmare mentioned this issue Jan 17, 2020
8000 Merged
22 tasks
@frenck frenck mentioned this issue Jan 17, 2020
Closed
@MartinHjelmare MartinHjelmare mentioned this issue Feb 1, 2020
Merged
20 tasks
@OnFreund OnFreund mentioned this issue Jul 2, 2020
Closed
@allenporter allenporter mentioned this issue Jan 4, 2021
Closed
@emontnemery
Copy link
Contributor

The conclusions in this issue have been reconsidered in #985, we now allow updating supported_features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@lucasweb78 @balloob @ryanm101 @emontnemery @amelchio
0