RobertD502 / home-assistant-flair

Custom component for Home Assistant Core for Flair pucks, vents, rooms, structures, and minisplits
MIT License
87 stars 12 forks source link

Add support for HVAC "off" mode #59

Closed gbleaney closed 1 year ago

gbleaney commented 1 year ago

Description

Flair pucks do not support an "off" mode. Instead they have a sperate setting for power:

mode: "Heat", "Cool", "Fan", "Dry" or "Auto", depending on availability power: the last power state set by Flair "On" or "Off"

Home Assistant does support an "off" mode, which when set silently did nothing (except log an error if you knew to look for it).

This PR maps the HA "off" mode to Flair's "off" power setting, so the two can play nicely.

Testing

I performed these modifications locally on my HA install. I verified that I could set the off mode and the Flair puck would go off. I additionally verified that the HVAC power switch state correctly remained in sync.

RobertD502 commented 1 year ago

With your Flair structure in Manual mode, to change the mode for your IR controlled HVAC unit or make any other changes, the device has to be currently powered On.

There are a couple of issues with this implementation:

  1. If you were to set the HVAC mode to off, the only way to turn the HVAC device back on is to use the switch entity designed for controlling power. As a result, instead of the power being handled by the single switch entity, power off mode is now being handled by both an OFF HVAC mode as well as the switch entity, which as explained would still be required because there is no HVAC ON mode. Essentially, this isn't simplifying things.
  2. Although Home Assistant supports a HVAC mode of OFF, this isn't a mode in Flair. It is a mode that is reserved for devices that have an actual OFF mode where switching to another mode (Heat, Cool, etc) controls the device to power on and go into that mode.

Home Assistant does support an "off" mode, which when set silently did nothing (except log an error if you knew to look for it).

This is an issue with Home Assistant and not the integration. They have not implemented showing only supported modes in the UI (as returned by the integration) within their automation builder/developer tools services tab (they have done it for some things, but doesn't look like it has been implemented for the set hvac mode service). In addition, how power should be controlled (via the switch entity) is documented in the repo. So, users should never be attempting to call an unsupported mode on the climate entity.

gbleaney commented 1 year ago

Thanks for the detailed response. I think it's pretty easy to work around the issue in 1), and the whole point of this PR is to address 2), and I'd be happy to make the changes if you'd accept them. But it sounds like this whole idea is a non-starter, so it' that's the case feel free to reject the PR and save me some time.

In addition, how power should be controlled (via the switch entity) is documented in the repo. So, users should never be attempting to call an unsupported mode on the climate entity.

You document the power switch, but you don't actually specify which modes are supported, so I assume most people (as I did) will believe what he UI tells them. If you're not willing to accept a PR to work around HA bugs (reasonable, but disappointing), then I'd suggest making the docs more explicit about what modes are supported. I don't think it'll prevent most instances of this mistake (who reads docs end-to-end when things are working :) ), but at least when they go back to the docs it'll be clearer to them.

RobertD502 commented 1 year ago

You document the power switch, but you don't actually specify which modes are supported, so I assume most people (as I did) will believe what he UI tells them.

The available HVAC modes aren't documented because it differs from one HVAC unit to another. While one unit may have Cool mode, another may not. The code is handling this logic and only presenting modes (when viewing the entity via the UI) that are available as returned by the Flair API for their particular device model. As with any climate entity that supports setting HVAC modes in Home Assistant, users can determine what the available modes are via either the dropdown menu in the UI for the climate entity or by going to the developer tools states tab, selecting the climate entity, and looking at the hvac_modes attribute where all the available hvac modes for that particular entity are listed.

If you're not willing to accept a PR to work around HA bugs (reasonable, but disappointing), then I'd suggest making the docs more explicit about what modes are supported. I don't think it'll prevent most instances of this mistake (who reads docs end-to-end when things are working :) ), but at least when they go back to the docs it'll be clearer to them.

I'm not trying to be difficult here. If this was something that could be fixed I would be open to it- We have no control over what the UI shows as hvac mode options in the automation builder / developer tools services tab. The most that we would be able to do is to create an error in the log when an unsupported HVAC mode is selected....We'd be doing exactly what you described Home Assistant already does when an unsupported mode is selected, silently logging.

gbleaney commented 1 year ago

Alright, looks like this is a non-starter. I'll try and go upstream and fix this in Home Assistant: https://github.com/home-assistant/frontend/issues/17122

If that doesn't work out, I will come revisit this conversation, because Home Assistant itself seems to think it's perfectly fine to blur the line between modes and switches in ClimateEntitys: https://github.com/home-assistant/core/blob/dev/homeassistant/components/climate/__init__.py#L531-L560

RobertD502 commented 1 year ago

@gbleaney I have already raised an issue in core and frenck saw it as a good suggestion for general improvement. He recommended opening up a feature request in the Home Assistant community forums for showing only options in a service call that are available for an entity/device.

What they did in 2023.6 is improved it by only showing what settings are available for a particular service call/device, but not the actual options within the setting.

RobertD502 commented 1 year ago

@gbleaney To add: Thanks for pointing out the async turn on and off functions in the climate entity. Just tested it out and was able to add the functions. However, having those two functions doesn't add a power toggle to the climate entity. All it does is enables the climate.turn_on and climate.turn_off service. To control power via the UI we'd still need a switch entity. At that point users might as well also control the power via switch service instead of introducing yet another layer.

The only other option, which would get rid of the switch entity entirely, is to have HVAC mode off control the power off and have switching to the other modes send a power on request followed by a request to set the mode. The only downside to this is that it may throw some individuals off that are used to the Flair app where mode can't be changed when the power is off and would catch them by surprise that changing the mode also turns on the HVAC unit. We'd also falsely be returning a mode of OFF when the unit is actually in a different mode (cool, heat, etc).

gbleaney commented 1 year ago

I have already raised an issue in core and frenck saw it as a good suggestion for general improvement.

Got a link? Would be good to understand pre-existing discussion

The only other option, which would get rid of the switch entity entirely, is to have HVAC mode off control the power off and have switching to the other modes send a power on request followed by a request to set the mode.

The more I dig into it, the more I think this is the way Home Assistant intends climate entities to operate. But you're right that Flair has a different paradigm, and one or the other has to be worked around and it'll probably lead to user confusion either way. Personally I prefer the HA paradigm and would be willing to make the PR to adopt it, but it's definitely preference and not objectively right / wrong. For now I'll keep pursuing the changes to HA itself, because showing unsupported options is objectively wrong :(

RobertD502 commented 1 year ago

Issue in core

RobertD502 commented 1 year ago

@gbleaney version 0.1.9 should do exactly what you want!

gbleaney commented 1 year ago

Nice! Just upgraded and tested it out, and it seems to work. Thanks for implementing that.