jason0x43 / hacs-hubitat

A Hubitat integration for Home Assistant
MIT License
196 stars 48 forks source link

Should use set*Mode(cmd) instead of calling cmd() for enum attributes #139

Open amosyuen opened 3 years ago

amosyuen commented 3 years ago

Current implementation runs into problems when there are conflicting commands from different capabilities.

Example is Hubitat's controlled thermostat. It has switch capability as well as thermostat capability.

jason0x43 commented 3 years ago

Hmmm...that seems like a conflict. The Thermostat capability defines off as well as the set* commands, andoff` works as intended (as far as I'm aware) with thermostat devices. The Thermostat Controller docs also suggest that "off" functionality controls the thermostat rather than the controller. (I'm not in a position to test that right now, though.)

Assuming that off does something undocumented for Thermostat Controller devices, we'd need some way to detect a controller vs a normal thermostat to do something else.

amosyuen commented 3 years ago

My assertions are from direct testing of the thermostat controller.

Assuming that off does something undocumented for Thermostat Controller devices, we'd need some way to detect a controller vs a normal thermostat to do something else.

Is there as reason why you need to detect the controller? If you use set*Mode() command it should work for both normal thermostats and the controller.

jason0x43 commented 3 years ago

Without further testing, it's hard to say. A normal thermostat device has an off command, and thermostat mode supports an "off" state. Is it guaranteed that those always do the same thing?

amosyuen commented 3 years ago

Usually they are the same, but there's no guarantees since the author of the device can always code something different for the two methods.

If the behavior is the same, there's no reason to prefer one over the other. If the behavior differs I don't see any particular reason to think that off() does the right thing over setThermostatMode(off). The one benefit of setThermostatMode() is that it is guranteed not to conflict with other capabilities. If you look in the capabilities list, 10 different capabilities have the off() command, so anytime thermostat is combined with any of those 10 capabilities the off() may be ambiguous. Though hopefully authors shouldn't be combining those capabilities in the first place.

jason0x43 commented 3 years ago

Hmmm...I feel like the fact that both HA and Hubitat distinguish between an "off" command and the ability to set a thermostat mode to "off" implies that they might not always be the same function.

After considering it a bit more, I don't think that the fact that off disables the thermostat controller rather than turning off the thermostat shows any ambiguity in the API. That's just what off does for a Hubitat thermostat controller device (although I think the documentation for that is a bit ambiguous).

amosyuen commented 3 years ago

Could you clarify what your conclusion is on using off() vs setThermostatMode(off) all the time? Assuming they have different behaviors, I think HA service climate.set_temperature to hvac_mode: off should correspond directly to setThermostatMode(off). Rather service climate.turn_off should correspond to off().

jason0x43 commented 3 years ago

Hubitat has off and setThermostatMode for thermostats. Home Assistant has switch.off and climate.setThermostatMode for thermostats. I feel like there isn't an issue with how HA is interacting with thermostats, the issue is just that a Thermostat Controller isn't thermostat, so the way "off" behaves for it (turning off the controller) is different than the way off behaves for a thermostat (turning off the HVAC system).

amosyuen commented 3 years ago

Maybe we're misunderstanding each other. It sounds like you're agreeing that there's a difference betwen "turning off a thermostat" and "setting hvac to off". Yet you seem to insist that HA climate.set_hvac_mode(off) should map to off().

If you recognize those behaviors are distinct then I think that this should be the mapping: HA Hubitat
climate.set_hvac_mode(off) setThermostatMode(off)
climate.turn_off() off()
switch.off() off()

It seems to me that a method called set_hvac_mode() with documentation "Set climate device’s HVAC mode" in HA, should corespond to the Hubitat setThermostatMode() which is clearly for setting the HVAC mode.

jason0x43 commented 3 years ago

So, that is the mapping I was suggesting, yes -- climate.set_hvac_mode should map to Hubitat's setThermostatMode. After reviewing Hubitat's API more (it's been a while), I don't think that should be necessary, as Hubitat's mode commands are intended to cover the cases in the setThermostatMode enum. I think Thermostat Controller is simply not meant to be used like an actual thermostat device -- it mostly acts like a thermostat, but its off doesn't do what off does for any real thermostat device (that I'm aware of).

However, it does seem reasonable enough to use setThermostatMode in async_set_hvac_mode if that makes the controller happy.

jason0x43 commented 2 years ago

It sounds like Hubitat may have fixed the thermostat controller in 2.2.9.

  • Thermostat Controller virtual device now supports all thermostat commands.