Skip to content
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.

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

Fan entity speed settings #27

Closed
cdce8p opened this issue May 14, 2018 · 9 comments
Closed

Fan entity speed settings #27

cdce8p opened this issue May 14, 2018 · 9 comments

Comments

@cdce8p
Copy link
Member

cdce8p commented May 14, 2018

This issue was triggerd by home-assistant/core#14351 which was / is adding HomeKit support for fan entites.

During the work on the PR @schmittx and I noticed that the current way we assign fan speeds is overly complicated and not really practical for things like HomeKit, Alexa or Google Home.

Current state

Each fan platform creates a list of supported speed settings, the speed_list, and setting the speed value requires to pass a string that exists in the list.

The most popular entries to speed_list are:

  • STATE_OFF or SPEED_OFF
  • SPEED_LOW
  • SPEED_MEDIUM (sometimes)
  • SPEED_HIGH

Others include: auto and smart
The dyson platform taking a special case with: 0001, 0002, ... , 0010 and AUTO

Problem

Secondary platforms expect a) consistent values and b) most often percent instead of a string. In addition (c) the speed setting is mixed up with the current operation mode.

Solution?

  • The speed attribute should change to an int value between [0, 100]. The conversion should be handled by each platform individually.
  • auto, smart, off, (maybe manual) should get their own attribute: mode / operation_mode?

Their might be things that I've missed, especially how to cover this change in the fronted, but since the current way is somewhat all over the place we decided (at least for the homekit component) not to support the speed setting (for now).

@schmittx
Copy link

I definitely agree that some improvement needs to be made for fan speed, it's too inconsistent
currently.

My concern with using an int value between [0,100] for speed is that it's not clear how many discrete speed options there are for the entity.

An alternative approach might be:

  • speed_list would contain only discrete speed values like SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH, 0001, 0002, etc.
  • SPEED_OFF would be deprecated to prevent confusion, state should be used to show STATE_ON and STATE_OFF
  • A new attribute,mode or operation_mode (like climate), would be added to indicate auto, smart, manual, etc.
  • A section would be added to the dev pages to explain the attributes and expected values for each domain

With the above approach; secondary platforms, like HomeKit, that use percentage-based speed can read the length of speed_list to determine the correct interval step between [0,100]. This would allow for a more intuitive interface for users when using secondary platforms (ex. Home app with HomeKit). Secondary platforms can also read operation_mode to set an appropriate mode characteristic.

Finally, this approach may be less of an overhaul from the current architecture and therefore easier to implement than changing speed to an int value between [0,100].

@balloob
Copy link
Member

balloob commented May 30, 2018

We should definitely specify the speed constants that are allowed to be used. Dyson would need to map their 0001 etc to one of our values.

@cdce8p
Copy link
Member Author

cdce8p commented Jun 17, 2018

I was thinking about this issue again. What if we use an approach similar to the light brightness parameter, by defining an additional speed_pct?

Some advantages:

  • Full backwards compatibility
  • We don't need to limit ourself to just 4 predefined mode => better support for dyson
  • Each component can implement the conversion itself. It might make sense to move the most dominate one to a util class though.

I would imaging that a future roadmap could look something like this:

  1. New fan platforms must support speed_pct
  2. Change frontend (use slider instead of dropdown)
  3. Deprecation and later removal of speed and speed_list

To better illustrate my idea, I will post a link to a WIP PR shortly.


Update
Link to the PR: home-assistant/core#15030

@balloob
Copy link
Member

balloob commented Jun 19, 2018

I rather have a hard cut off, not sure why we wait to make the breaking change if we are going to make a breaking change?

@itineric
Copy link

itineric commented Oct 1, 2018

  1. Change frontend (use slider instead of dropdown)

Even if many fans use percents to define speed internally, many of them are "step-based", meaning that 1->20% = speed 1, 21->40% = speed 2, etc.
So I dont think that using a slider in all cases is a good idea.

@sknsean
Copy link

sknsean commented Jan 28, 2019

any news on this?
Can anyone point to a place where the discussion is ongoing?

@sknsean
Copy link

sknsean commented Jul 11, 2019

Is there at solution for this? Since the closing :-)

@cdce8p
Copy link
Member Author

cdce8p commented Jul 11, 2019

@sknsean Unfortunately, I don't have time to implement any of it. And besides the issue has probably been stale for far too long. Maybe someone else will pick up this topic in the future.

@mime29
Copy link

mime29 commented Mar 22, 2023

Any discussion about this topic somewhere?
Homekit allows to slide up and down the ON/OFF button. It seems that on Apple's side it's do-able.
That would free us from using the AC remote control at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants