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

Add an additional fan speed #127

Closed
mkowalchuk opened this issue Jan 1, 2019 · 56 comments
Closed

Add an additional fan speed #127

mkowalchuk opened this issue Jan 1, 2019 · 56 comments

Comments

@mkowalchuk
Copy link

This issue is related to home-assistant/core#19179 (comment).

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 (#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
Copy link
Member

cdce8p commented Jan 3, 2019

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 home-assistant/core#19179 (comment)

@MartinHjelmare
Copy link
Member

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

@cdce8p
Copy link
Member

cdce8p commented Jan 3, 2019

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
Copy link
Member

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
Copy link
Member

balloob commented Feb 26, 2019

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
Copy link

sknsean commented Feb 27, 2019

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
Copy link
Member

balloob commented Feb 27, 2019

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
Copy link
Member

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
Copy link

quielb commented Sep 28, 2019

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
Copy link

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/core#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
Copy link
Member

Jc2k commented Oct 3, 2019

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
Copy link

quielb commented Oct 3, 2019

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
Copy link

@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
Copy link

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
Copy link

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
Copy link

Misiu commented Jun 25, 2020

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
Copy link

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
Copy link

Misiu commented Jul 28, 2020

@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
Copy link

@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.

@Jc2k
Copy link
Member

Jc2k commented Oct 26, 2020

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
Copy link

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
Copy link
Member

Jc2k commented Oct 26, 2020

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
Copy link

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
Copy link
Member

Jc2k commented Oct 29, 2020

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
Copy link

mikesalz commented Jan 1, 2021

@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
Copy link

jbouwh commented Jan 16, 2021

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. home-assistant/core#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
Copy link
Member

bdraco commented Jan 24, 2021

We have a mostly complete implementation of percentages in home-assistant/core#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
Copy link
Member

Jc2k commented Jan 24, 2021

👍 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
Copy link
Member

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.

@bdraco
Copy link
Member

bdraco commented Jan 24, 2021

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.

Yes, I can enumerate the list of speeds that have moved to presets in the breaking change.

Edit: I've done this in breaking change section of home-assistant/core#45407 (comment)

@bdraco
Copy link
Member

bdraco commented Jan 28, 2021

Closed via home-assistant/core#45407 (comment)

@quielb
Copy link

quielb commented Feb 4, 2021

Just a note that nobody has mentioned here. It seems there was a change with how Alexa can handle fans. There is now a fan device type available in Alexa. Along with that Alexa knows how to properly interpret speed_list. I have a fan with a speed_list of 1..10. I can tell Alexa to set the fan speed to 7 and she happily complies. I also have a fan with a speed_list that includes "low", "medium-low", "medium" etc. Alexa can also correctly interpret those speeds as well.

I'm not trying to undo what has been done, but are we trying to solve a problem that may not exist anymore? I can't speak to what Google assistant can handle, I don't own on. But if both the assistants can interpret speed_list is it even necessary to make a change? I understand that it's not only the voice assistants that this change is being made for, but the point has been made several times about the need for a deterministic speed_list. It seems now, at least for Alexa, that the assistant can interpret the the defined speed_list.

@bdraco
Copy link
Member

bdraco commented Feb 14, 2021

As a followup that was deferred from the original implementation to contain scope (home-assistant/frontend#8216 (comment)), I've opened home-assistant/core#46512 to implement the step sizes as well as the accompanying fan.increase_speed and fan.decrease_speed.

@jbouwh
Copy link

jbouwh commented Mar 4, 2021

Now the base is set, is there progress in updating the mqtt.fan and climate integrations? Climate also has a fan related properties.

@bdraco
Copy link
Member

bdraco commented Mar 4, 2021

This arch issue was only for the fan entity model. If we want to change the climate model, that will require another discuss and a champion to make the change.

As MQTT will require a more customized approach then the majority of cases, this is up to the MQTT integration maintainers as to how the implementation happens.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests