mkowalchuk commented 5 years ago

_This issue is related to https://github.com/home-assistant/home-assistant/pull/19179#discussion_r243912612._

I'm seeking approval for adding an additional fan speed to the base fan component, so that 4-speed fan controllers (e.g., the HomeSeer FC200+) can be fully supported in HA. I propose that this speed be called "medium-low", as this is commonly used on 4-speed fan controllers.

I see that there's already an issue discussing a larger scale revamp of the fan entity speed system (https://github.com/home-assistant/architecture/issues/27). It's not clear to me if additional fan speeds should wait until that issue has been resolved, or if more speeds can be supported in the short-term.

cdce8p commented 5 years ago

We didn't really get to a good conclusion on #27 and since I don't have much time at the moment this issue has more or less gone stale.

As far as I know, each fan platform uses their own speed list settings. So IMO it shouldn't be an issue to add another one, at least until we get to a more permanent solution how to handle fan speeds in the future. However I would add it to the platform and not the component.

Examples

dyson Fan speeds imported from platform library. More or less: 1 - 10, Auto
template Some default settings, but individual speed list can be specified.
mqtt Same as template

@MartinHjelmare What do you think? I saw you raised this issue originally in https://github.com/home-assistant/home-assistant/pull/19179#discussion_r243912612

MartinHjelmare commented 5 years ago

We should only use speeds in the base fan component. Existing bad examples shouldn't be followed.

cdce8p commented 5 years ago

I might not have gotten my argument across quite yet.

What I was saying is that basically all fan platforms are bad in that way (except the demo one maybe). But that isn't really a bad thing. The backend, frontend and even the coresponding services are designed to support custom speed settings. Take the dropdown in the frontend as an example.

If those four settings in the base component were enough, why use a list of string arguments for the speed. An integer would be more then enough and could support even more services (increase / decrease speed). The point I'm trying to make is that those aren't enough (take eco, auto or different step ranges).

The only solution I see would be to extract auto and eco modes and use speed as an integer with min and max values. However changing this would require a lot of effort, something probably no one has at the moment. Not to overlook the problem that even this suggestion in a slightly different form wasn't even able to gain consenses in #27

In the end I think it doesn't hurt us to allow even another different speed setting if that means we can support a device better then we can at the moment.

MartinHjelmare commented 5 years ago

All new platforms have to follow the speed modes in the base component and not all existing platforms are breaking this. If we need to add more speeds we can do so in the base component if we get approval in the architecture issue.

balloob commented 5 years ago

If we don't follow a base implementation, it is impossible to integrate with Google Assistant, Alexa or HomeKit. This is because we can only translate values into what those systems expect if we know the value.

sknsean commented 5 years ago

I don't think we should restrict core home-assistant to what's possible in Google Assistant, Alexa or HomeKit. It's better to have some kind of translation into those systems

balloob commented 5 years ago

We don't restrict Home Assistant to what's possible.

To be able to translate though, we still need to know the values in advance and we do that by creating an abstraction. That's literally the purpose that HA exists 😉

cgarwood commented 5 years ago

Any other thoughts/updates on this? There are a number of devices with 4 fan speed settings, so I think it makes sense to add a 4th available speed to the base component. It shouldn't affect fans that only support 3 speeds should it? I'm not sure how it all plays in with alexa, google home, etc.

quielb commented 4 years ago

I've been working on integrating 2 fans into HA. One which has 10+off speeds and the other which has 6+off . The six speed one isn't a problem to map to the base fan speeds. It's a ceiling fan, so losing an few minor incremental speeds is a big deal. But the 10 speed fan is a whole house fan. Loosing access to all the speed steps defeats the efficiency purpose of the fan. The idea behind it, and how I've automated it, is to take advantage of deltaT between inside and outside temperatures. The greater the deltaT the higher the speed. And as deltaT increases the fan speed should step with it. But I can only do that if I can access all the speeds from a speed list. In my case I don't really care if this device can integrate with Alexa/Google. Because the deltaT is highest early in the morning, I wouldn't be using the assistant(s) to control it I would be relying on automation.

The trend in the industry is also moving away from the traditional 3 speed fan. They are still by far the most common fans on the market. But you are seeing manufacturers introduce DC motor fans which allow for many more steps in speed than a tradition AC motor fan offers.

I think the compromise here is to let each individual decide on functionality with assistants by mapping or not mapping the "standard" speeds to the fan platform they are coding. It doesn't necessarily make sense that all fans need to be controlled with an assistant. And to not allow all speeds can diminish the effectiveness of a fan.

fuzzie360 commented 4 years ago

I was about to file a proposal for a architecture change for a separate fan.set_speed_pct and fan.set_mode because some fans do not have a linear fan speed list and thus causing issues for the homekit component (related: home-assistant/home-assistant#27113 )

This is quite important to me because I am controlling 7 fans in my house.

Would love to have fans to have the same first-class treatment as lights in home assistant soon.

Jc2k commented 4 years ago

When triaging homekit issues i've seen a fair few reports about fan speed, and having a fan percentage (with components mapping the speeds they support to percentages) seems like a good way of addressing it.

quielb commented 4 years ago

I also found a technical issue with only allowing 3 speeds mapped. I am working on a new integration with a fan that has 6 speeds. I mapped low,medium,high to 2,4,6. When the speed is changed locally on the control pad for the fan, say to 5, that doesn't match any of the speeds I have defined and generates a KeyError when looking up in the dict to match fan speed to home assistant speed. I do a try-except when the fan is polled and basically just throw away the error. But now HA doesn't represent the actual state of the fan.

nkaminski commented 4 years ago

@fuzzie360 I would also be in favor of a fan speed percentage, as this would make the full speed range of DC or VFD driven fans accessible.

Additionally, for fans with more than 3 discrete speeds, a map-to-nearest function could be implemented to translate a percentage to the closest discrete speed, similar to the fan abstraction in the emulated hue component.

RefineryX commented 4 years ago

Are there any more thoughts on this? I recently added my Dyson fan to HA but unable to get it to work correctly through HomeKit due to the fan speed setting needing to be a percentage.

smurf12345 commented 4 years ago

I finally found this thread after a lot of searching which explains why my lutron caseta fan switch is only able to use 3 speeds of the 4. I was assuming the mapping of the speed_list was just a specific lutron issue but looks like its based on only 3 speeds in the main HA fan component. Is there a way to copy the fan component into the custom component directory and manually add the 4th speed for now?

Misiu commented 4 years ago

I also agree with @fuzzie360. I recently created an integration for Argon40 Case for RaspberryPi. The case has a built-in fan that can be used to cool Raspberry. The control is done via I2C by sending a value between 0-100. For now, I use a service but having percentage support for fan speed would allow me to rewrite the integration and use the fan platform as I wanted from the beginning.

I'm not a python expert, but I think it would be possible to remove the type from set_speed method (https://developers.home-assistant.io/docs/core/entity/fan/#set-speed) and check for type in method body or better - use multimethod (https://www.artima.com/weblogs/viewpost.jsp?thread=101605, https://pypi.org/project/multimethod/). Question is: does HA supports this.

EvTheFuture commented 4 years ago

I'm currently building my own motor and software for controlling the ventilation in my house. The fan speed is controlled on each fan with a 5 step transformer (off+5 speeds).

Reading this thread it sound to me the suggestion about having a percentage is a good idea. But isn't it possible to add percentage as the forth speed in the speed list to not break all current integrations?

Off, Low, Medium, High, Percentage

Then add a percentage value that can be read and written to.

If Percentage are set as current speed, then use the percentage value instead. This shouldn't break existing code since percentage would only be set/used on new/updated integrations.

Wouldn't this be a viable compromise to take this issue forward? Or have I missed some thread where this already has been resolved?

Misiu commented 4 years ago

@mukens currently fan has a property called supported_features. a new flag, called SUPPORT_SET_SPEED_PERCENTAGE could be added. This way the integration author could decide if they want to support percentage speed. This would address your needs. Only updated integrations would support this. You will be able to use the same service to set speed. fan component must allow positive int and string in async_set_speed and then in fan integration we can check if speed is a string or if it is int (isinstance(speed, str)). I'm sure the frontend must also have some changes, but maybe there other places too.

EvTheFuture commented 4 years ago

@mukens currently fan has a property called supported_features. a new flag, called SUPPORT_SET_SPEED_PERCENTAGE could be added. This way the integration author could decide if they want to support percentage speed.

@Misiu sounds like an option, but just to understand, will it be added in the core fan component? If so I think it's a good idea to have people on board with such a change.

Misiu commented 4 years ago

@mukens if this will be added as a new flag, then without changes in current integration everything will work as now. Only integrations that use this new flag will be able to use percentage value when setting up speed. I think this must be done that way to don't break backward compatibility. So all existing integration that wants to support percentage speed must get updated.

Let's wait for the core developer's opinions. Maybe they'll agree on this or propose a better solution.

EvTheFuture commented 4 years ago

@Misiu Do I need to do something to get the attention of the core developers or do they usually find these threads? (I just started with development and integrations in HASSIO).

Misiu commented 4 years ago

@mukens not sure what to do. I suppose we should wait for the core team's attention.

Jc2k commented 4 years ago

This has come up again as WTH here, targeted at the integration I maintain. But my hands are tied while I don't have an interface to map these fans to. What is the correct way to proceed? Would a PR that added an optional SUPPORT_SET_SPEED_PERCENTAGE so that we can further this discussion be a good way to go?

emontnemery commented 4 years ago

@Jc2k Yeah, there seems to be consensus that SUPPORT_SET_SPEED_PERCENTAGE is a good way to go so I think a PR on the base component is the next step.

bdraco commented 4 years ago

Speed percentage should work no mater what the speeds are called. Makes a lot of sense to me 👍

balloob commented 4 years ago

I like the idea of percentage too. Limited options was a mistake

Jc2k commented 4 years ago

I'll see if I can sketch something out as a proof of concept for homekit_controller and see what falls out then, unless anyone already has something ready to go.

MartinHjelmare commented 4 years ago

The only question is the migration plan. Do we have to have two different interfaces or can we migrate all integrations to percentage?

Jc2k commented 4 years ago

That's a good question. To avoid a breaking breaking change I was going to experiment with mapping between the 2 interfaces. Some of the code already has to do this and I thought it might be possible to generalise it. It might be easier to answer that when we see how onerous such a mapping is?

bdraco commented 4 years ago

HomeKit already maps these and it works for the majority of cases (except the ones that are not actually compliant with the spec and have added ‘auto’). We could probably reuse some of that code.

Sent from my Mobile

On Aug 23, 2020, at 2:04 PM, Jc2k notifications@github.com wrote:

That's a good question. To avoid a breaking breaking change I was going to experiment with mapping between the 2 interfaces. Some of the code already has to do this and I thought it might be possible to generalise it. It might be easier to answer that when we see how onerous such a mapping is?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

RefineryX commented 4 years ago

Not sure if this helps (probably not) but I do get this in my logs regularly. It's definitely related to what you are chatting about but wanted to share incase it can help in anyway.

Jc2k commented 4 years ago

@bdraco exactly my thoughts. homekit probably has the most pragmatic/useful mapping off the lot because it tries to handle all the non standard fans that we've ended up with. The one I have in homekit_controller only maps to the 3 speeds HA officially supports for example.

@RefineryX this is an interesting case! Home Aassistant define's OFF as a speed, and it transparently maps set_speed('off') to the "turn this fan off" code. But your fan doesn't define that as a valid option. So while it probably does accept set_speed('off') you probably can't select 'off' in the speed drop down. and that might affect whether some parts of HA can turn the fan off or not. It's similar to the 'auto' problem @bdraco mentioned. I think we should clarify whether its mandatory for speed_list to contain off, or whether it should never contain off.

Jc2k commented 4 years ago

I've pushed an early draft PR https://github.com/home-assistant/core/pull/39204 to enable some more discussion around this. At this stage it's mainly to aid in discussing the approach in this ticket. Given that the approach might change and require a rewrite of that PR, please hold off on cleanup ideas and nits till we are happy with the high level approach :).

Not all devices have off in speed_list. It would be good to clarify if that is expected or not and make them conform. Should off always be in speed_list and shout it be first?

The mapping between the 2 systems works, but its made more painful by fans that don't follow the spec. The current scheme accomodates fans that have added their own custom speeds. But like the homekit one, it won't tolerate "modes" or "presets" such as "AUTO" or where the first first speed isn't SPEED_OFF. Should we add presets like in climate for these? Just drop support? Leave as is for now and deal with separately?

If we change the UI to expose a percentage slider then the UX for 3 button fans would suck. But I don't think we want to maintain 2 different UI widgets for fan speed. If we can deal with this nicely then long term we could get rid of the original set_speed interface? One option is that we could add a step property. This lets you turn your slider into less than a percentage i guess. E.g. for dyson we would set step to 10 and the UI would know to snap to 0, 10, 20, etc. This isn't just a UI feature, we'd probably want to round values we were passing around to the nearest snap point. Unclear how well it would work where the step is really 33.333333333 though. It would mean for our spec compliant fans we'd have snap points at 33, 66, 99... that wouldn't be great. Any one got any ideas there?

Survey of existing fans

The fans with (potentially) non standard fan speeds are:

dyson - AUTO and 10 speeds
zha - supports the standard speeds, but then sticks 3 presents on the end of the list (on / smart / auto)
xiaomi_miio - uses the speed list as a preset list (only 1 model seems to reasonably have what you would call speeds in its speed list?)
tuya - it doesn't obviously use the HA mandated speeds, it fetches its own list from the device and its not clear what that is. This should work with my mapping but is non standard so highlighting.
wink - adds AUTO to the speed list, adds an extra speed, doesn't have off in speed_list
smarty - doesnt haveoff` in its speed_list (is this the fan @RefineryX has?)

Also some external HACS fans are known to have non standard speeds, like this one.

The fans that will be improved by this work:

homekit_controler - supports a percentage
dyson - supports 1-10
wemo - supports 5 speeds
zwave - looking at the SPEED_TO_VALUE mapping i think it supports a percentage?
wink - has 4 speeds.
isy994 - has 0-255. currently maps 63/64 as low, 190/191 as medium and 255 as high.
ozw - same as zwave
lutron_caseta_pro - (external fan, looks like its percentage based)

sknsean commented 4 years ago

I like the idea of percentage too. Limited options was a mistake

1, 2, many :) Always good practice in programming to prepare for more options Keep up the good work on this :-D

MartinHjelmare commented 4 years ago

If we implement the new service for all integrations while keeping the old service we could migrate the frontend to the new service but avoid a breaking change for user automations that use the old service.

Jc2k commented 4 years ago

@MartinHjelmare thats certainly one of my aims, if we can make the frontend for 4 button fans good at least. And i'm happy enough (I think) with the mapping code that it could stay long term, not just as a transition thing. What i'm less sure of is how this interactions with the state reproduction code. IIRC there shouldn't be 2 ways to set the same underlying property in that code, right?

If my PR works as intended then it implements the new API for all existing fans, though some need updating to unlock their full potential. Whilst preserving the old API for automations etc. If my PR was merged now, the 6 fans I called out will continue to work as is until a new UI is merged, before then they will need unpicking to comply with the spec a bit better. It's unclear that xiaomi can retain its current speed controls though.

I'm writing up another approach which part of me thinks is a bit cleaner, but it is a lot more work. In the meantime I think the biggest question I have about this PR is how to make the UI for fans with small speed lists good. I think step is the right answer. I'm just uneasy where 100 isn't cleanly divisible by step (like low/medium/high fans)

Jc2k commented 4 years ago

As an alternative I was going to suggest renaming all the existing speed list code to be preset_list, set_preset etc. And then add a new set_speed that only dealed in percentages. And to not force a tight coupling between that and the speed percentage. That approach would have caused quite a bit of breaking change, though.

Should we add a preset or mode interface (like on climate) for the weird things like "auto" and "smart" first, before adding percentages? Or should we just drop these features from speed_list being spec violations?

MartinHjelmare commented 4 years ago

What the state should be is important. We can't change that without making a breaking change.

balloob commented 4 years ago

Fans are based on ToggleEntity so are on/off. We should be able to represent that with percentage too.

cprussin commented 3 years ago

Hi 👋 just curious if anyone involved knows if there's any progress here? This doesn't seem like good first contribution type work, but I really would love to see this get fixed so I'm happy to give it a try if anyone can help give me pointers (both in the code and in terms of how the ui should work)

Jc2k commented 3 years ago

I had a first pass at it here. Unfortunately i've not had time to work on it further, would be very happy to pass it on to someone else as I want to support my fan better too!

My approach tried to convert both ways - it would "upgrade" existing fans to have an approximate set_percentage interface, but when a fan implemented the set_percentrage interface it would then instead automatically provide the existing backwards compatibility speed_list interface.

If i understood the feedback correctly, instead of converting both ways all the fans should work in terms of percentages. The only conversaion should be from the old system to the new system. This will required set_percentage to be added to each fan.

Note that there are some fans that misuse the speed_list and these do not map well to set_speed_percentage. For example xiaomi_miio misues speed list as some kind of preset list and i'm not sure you can set a speed for some models with the current code.

I don't have any direction about UI, sorry.

cprussin commented 3 years ago

I'm probably missing something but the percentage approach seems counter intuitive to me--I would expect the fan speed selection to be an enum where the device can somehow specify valid values in the enum. Is that not possible for some reason?

Jc2k commented 3 years ago

You'd have to re-read this ticket to see how consensus was reached.

The fan abstraction needs to cover fans as generally as possible otherwise it's not a useful abstraction. When it was originally designed I guess most fans had only a handful of speed presets, but this is less true now. Additionally, a good abstraction preserves semantics, it preserves meaning.

There is currently an explicit goal for the speed_list to have some meaning that we can "translate" between systems. People want to be able to use Alexa/Siri/Google with their fans. Opaque meaningless presets are no good for that. So we can't have arbritary values in speed_list. With the current setup this means that each new speed preset needs an issue like this one. And this one has been open since Jan 2019 so that's not a scalable solution.

(Note that some fans have gotten away with breaking the rules and as such other integrations have had to add hacks to cope with them. That's not a justification to continue, removing said hacks would be nice).

The alternative is to find a way to preserve the meaning of the fan speed in a way thats useful for voice assistants etc whilst allowing much more flexibility for developers.

Some fans may just be on/off or have 3 or 4 speeds. But some literally do already have a percentage speed control already. We don't really want a speed_list dropdown containing 100 speeds for those fans. And at the end of the day percentage scales better for "old" style fans then a speed list does for "new" fans. Especially if we can provide a "step size" for the UI.

(Personally I think fans should probably gain a preset feature and 3/4 speed fans could probably have presets for their small number of speeds. This would also preserve the functionality of the xiaomi_miio integration. But that's not got approval yet.)

cprussin commented 3 years ago

Got it, that's helpful context. Thank you for summarizing @Jc2k. I took a look at the PR you opened; what would be required to get that to land? Is it mostly just addressing the feedback that was left there or is there more to take it out of draft state?

Jc2k commented 3 years ago

Addressing the feedback is the main thing. But if I understood the feedback correctly then it's quite a bit off being ready. Well, you should read the feedback and see what you think.

It's a while since I opened the PR so it will need rebasing or cherry picking onto a fresh copy of dev as well.

I'd also want to see tests added, I'm sure the core devs would too.

mikesalz commented 3 years ago

@MartinHjelmare @balloob @cprussin Happy New Year, all! Just wondering if any progress has been made here? I am happy to help test any solutions. I am using a 4-speed + off Hunter SIMPLEconnect fan through homekit controller.

jbouwh commented 3 years ago

So it possible to add additional speeds or replace the defaults, but the device can not report those states back over mqtt. Cannot believe this is by design. https://github.com/home-assistant/core/issues/45234

Over mqtt it is possible to use the speeds attribute to set additional or alternative speeds, the speeds are shown in the list and can be set using the set topic. But state updates only happen when the state is off , low medium or high

bdraco commented 3 years ago

We have a mostly complete implementation of percentages in https://github.com/home-assistant/core/pull/45407 that allows the existing calls to set_speed to be automatically translated to the new interface, however we have run into a snag.

The core issues we are solving is:

non portable interface to speed for voice assistants
limited about of speeds
presets and speeds mixed together in speeds

There are a few fan devices where that have speeds of auto, smart, interval, favorite, .. etc. These cannot be represented by a percentage. They seem like presets to me.

To accommodate these values, I propose we have a preset_list and the service to support it similar to how climate works since it will be familiar for both users and developers.

This allows the presets continue to work while solving the core issue of abstracting the speeds by separating them. We should be able to automatically separate the existing presets (at least for core integrations) so integration authors can update over time without breakage and continue to provide backwards compatibility with the set_speed service using this approach.

cc members who have previously given feedback on this topic: @balloob @cdce8p @cgarwood @emontnemery @Jc2k @MartinHjelmare

Jc2k commented 3 years ago

:+1: x 100 - I had wondered about a preset_list previously and it seems like a neat way of fixing the integrations that violate the existing fan entity schema without taking functionality away from users.

MartinHjelmare commented 3 years ago

My main worry is about clear communication to users. Can we be explicit about what speeds have moved to presets if we do an automatic separation of speeds and presets? This part isn't clear to me.

home-assistant / architecture

Add an additional fan speed #127

Examples

Survey of existing fans