search

home-assistant / architecture

Repo to discuss Home Assistant architecture
313 stars 100 forks source link

Architectural review of climate entity #22

Closed balloob closed 3 years ago

balloob commented 6 years ago

This issue was triggered by https://github.com/home-assistant/home-assistant/pull/13340 which was adding Alexa controls for our thermostats. I also just did a rewrite of Google Assistant and made it so it only supports our official operation modes.

I think that it's time to do a review of our climate entity. Clean it up and then be better in code reviews in enforcing the rules of the climate component.

Supported features

We have individual support flags for setting a temperature, setting the upper bound of a temp range and setting the lower bound of a temp range. This is still an artifact from when supported features was used to show/hide frontend controls.

Cleanup of made up values

The goal of our abstract base class is to specify which values can be used. That way we can build things on top like frontend, Alexa integration, Google Assistant integration or compare the usage history of two thermostats from different brands with machine learning.

However, there are a bunch of platforms that return their own values for properties. Especially operation mode suffers a lot here. Instead of using the STATE_* constants, these platforms return their own made up values. It works with our frontend because we also allow returning an operation list. It does not, however, work with Google Assistant or Alexa. Both have been coded now to be very strict and ignore any non standard operation.

Operation mode

Operation mode describes in what kind of operation the thermostat currently is. This is what we have right now:

STATE_HEAT = 'heat'
STATE_COOL = 'cool'
STATE_IDLE = 'idle'
STATE_AUTO = 'auto'
STATE_DRY = 'dry'
STATE_FAN_ONLY = 'fan_only'
STATE_ECO = 'eco'
STATE_ELECTRIC = 'electric'
STATE_PERFORMANCE = 'performance'
STATE_HIGH_DEMAND = 'high_demand'
STATE_HEAT_PUMP = 'heat_pump'
STATE_GAS = 'gas'

Here are just some initial observations:

Actually: I noticed that we have a platform called econet that is not a climate device but is a water heater. It's the only one that uses STATE_GAS, STATE_HEAT_PUMP and STATE_HIGH_DEMAND (they are also used by Wink, but that's because Wink represents econet via their API). I think that econet should not be a climate device.

State property

This should be just operation_mode. Right now it's a combination of is_on and operation_mode. By trying to blend two attributes, we've made the value pretty much useless.

is_on property

Do we need this? Can we blend this in with operation mode? For example, if something is an on/off device, it could just have operation mode be heat/idle or cool/idle. Idle and off are obviously the same kind of action: nothing is happening.

References

andrey-git commented 6 years ago

A comment on on/off as I added that: off is not the same as idle. On/Off is a setting while idle is an observation. A device can switch on its own from idle to another state. While we could merge idle into off I think we would lose important information.

That said, maybe operation_mode should be an attribute and not a state?

tinloaf commented 6 years ago

Some obervations, some of which come from the stale https://github.com/home-assistant/home-assistant.github.io/pull/3910 :

pvizeli commented 6 years ago

We need a abstraction how a thermostat should look like on HomeAssistant.

I think the state should be the current temperature. All others are attributes. Like before the refactory of this components.

fanthos commented 6 years ago

I think the state should be current running state, like heat, cool, idle and off. Off means the component is turned off. There also should heave operation_mode, it shows current mode, auto, off, heat, cool.

cdce8p commented 6 years ago

I don't know if a idle current state is necessary. It's the same as currently off. Otherwise thats the way HomeKit handles it as well @fanthos.

fanthos commented 6 years ago

I think the off state means manually turned off and should not turn on by temperature or other reason. idle means the component is in "working" state and it will turn into heat or cool when temperature goes cool or warm.

cdce8p commented 6 years ago

Isn't that covered by operation_mode=Auto? Or should that just be a list of supported modes?

trisk commented 6 years ago

Regarding feature flags, the Alexa.ThermostatController design allows thermostats to report whether they support single, dual and triple setpoints (target temperatures). There's an extended series of examples here: https://developer.amazon.com/docs/device-apis/alexa-thermostatcontroller.html#thermostat-setpoints Note that these may be the same thermostat device but in different operational modes.

There may certainly be thermostat devices out there that only support an upper and lower setpoint in certain modes (though it is unspecified in the Alexa documentation which setpoint a AdjustTargetTemperature request would apply to). Our climate entity has assumptions that there is always a target temperature value separate from the higher/lower ones, so it does not provide an accurate representation of these devices.

trisk commented 6 years ago

I think the off state means manually turned off and should not turn on by temperature or other reason. idle means the component is in "working" state and it will turn into heat or cool when temperature goes cool or warm.

You seem to be describing climate.STATE_AUTO, which is the state where the device will automatically maintain temperatures between the high and low target temperatures. climate.STATE_IDLE means the device is not operating, but will accept operation mode or target temperature changes.

I agree that there should be an "off" state for when the device is not allowed to be turned on/operated through software.

trisk commented 6 years ago

Regarding operation modes: Looking at implementations of "eco" mode such as Nest and Honeywell it appears to behave similarly to "away" mode in that it changes the target temperature range, without replacing the current setpoints. There may be limitations on the target temperatures allowed, such as in the case of Nest. While this may be considered a variant of "auto, "heat", or "cool" mode the way "away" mode is, given the independent setpoint(s) it may be necessary to express this as its own mode.

I believe "high demand" mode only applies to water heaters. "performance" mode seems to also be for water heaters, though climate.toon is abusing climate.STATE_PERFORMANCE to mean "auto" mode right now.

trisk commented 6 years ago

There are a lot of commonalities between thermostats and water heaters (or other temperature control devies) that make it attractive to use a single entity type (and in some cases such as climate.zwave, platform) to support them. Perhaps it is sufficient to have operation modes that are distinct for e.g. thermostat devices, and use a feature flag to specify the type of device?

balloob commented 6 years ago

@tinloaf aaah there the issue is! I was looking for. I remember us talking about it but couldn't find it.

I think that idle is not a correct operation mode. There is a difference between "current operation" and "current operation mode". Ours is the latter. Being in an operation mode doesn't mean that you are currently doing something. For example, a climate component in heat mode with a target temperature of 21 and a current temperature of 23, will not be doing something, yet it's mode remains "HEAT".

I would expect "off" to be a correct operation mode as it means that we will not do anything regardless of target and current temperature.

balloob commented 6 years ago

@trisk about the overlapping part: we should make it a separate component or else we end up with a lot of if…else things. The same reason light and switch are different components.

Landrash commented 6 years ago

Adding some from my thermostats that gets registered as climate entities. Off and Idle are two different things.

Those two should most likely not be merged since it could vary with each device.

trisk commented 6 years ago
  • Idle with mine equals to it waiting and adjusting temperature when necessary.
  • Off equals to it doing no actions.

What thermostat model is this? Most thermostats only adjust temperatures when in the operating modes "heat", "cool", or "auto". Both terms "idle" and "off" are usually reserved for modes where the thermostat will not attempt to adjust temperatures, though there may be a separate mode for when the device must be turned back on manually.

trisk commented 6 years ago

This is where the current state and operational mode differ: some devices report a state to indicate whether a heating or cooling cycle is actually running at a moment in time and this should not be confused with the (user selected) operational mode, which determines which setpoints the device will try to adjust temperatures between.

balloob commented 6 years ago

Trisk is right and our current naming is confusing, we actually want to represent "HVAC operation mode" but named it "current operation", which is confused with the current state. Oops.

So to make it clear, let's add hvac to the name and rename them. I suggest:

Some popular climate APIs that I checked that all use "off" for hvac mode:

fanthos commented 6 years ago

I suggest that HVAC_STATE have both idle and off. idle for turned on but idling, off for HVAC_MODE is off.

balloob commented 6 years ago

By adding idle as a state we are again conflating mode and state. Look at mode to see if a device is off, look at the state if a device is cooling/heating a room.

andrey-git commented 6 years ago

@balloob, is there a way in your proposal to distinguish between HVAC set to heat and actually heating and one set to heat and doing nothing because the room is hit enough?

pvizeli commented 6 years ago

@balloob most of thermostat device can only heat and other can cool. That make the state in 70% to a constant and we store no constant into statemachine?

100% of a thermostat is the temperature in the focus of device. All other will be construct around this value. I vote for a temperature state and other will be construct around this as attribute.

EDIT

Hmm, there are other device they use also humidity and they make the temperature as state a bit wired. Difficult, but the state like you describe will be also bit mess.

balloob commented 6 years ago

@andrey-git set hvac_mode to heat and hvac_state is either heat or off

@pvizeli you're right that HVAC state can be derived from comparing current and target temp. We can't have the decice state be a temperature because target temperature can be a range when device is set to hvac mode "auto".

I guess HVAC mode is not that important as it indeed can be derived from target and current temperature.

We should change state to be just HVAC mode. Because HVAC state can be confusing. You'll see off but the actually heating but is not currently heating.

andrey-git commented 6 years ago

@balloob that would be enough for automation, but would be less usefull for visualization.

Hvac_mode could also be fan or dry.

balloob commented 6 years ago

The frontend can calculate itself what the current state would be? We should aim for single source of truth and if data can be derived from other pieces of data, we should not include it.

After reading this article on what "dry mode" is, I agree that we should include dry. (tl;dr: it dehumidifies the air)

I don't like the "FAN" HVAC mode because we already have a "fan mode" property. In case a unit is set to fan, I expect fan mode to be set to "on" and hvac_mode set to off.

hthiery commented 6 years ago

As maintainer of the fritzbox thermostate implemenation I can tell that there is no way to get the actual state of the thermostate. The thermostate is a device directly mounted to valve at the radiator and controls the state of the valve always automatically. Its only setteble parameter is the desired target temperature. The other parameters like eco or comfort temperatures are only values that come from the schedule form inside the fritzbox webfrontend to plan the target temperature change. The state of the valve (open, close or something in between) cannot be read.

Additionally an away mode (for holidays) can be set and also a mode where the valve is forced to close (for sommer).

So my suggestion for these kind of devices is to have an automatic mode, away mode and maybe something . But I dont't know how to handle the state.

balloob commented 6 years ago

We already support away mode and you don't have to implement the set_operation_mode service either. Eco and comfort are profiles, and are not related to hvac modes.

trisk commented 6 years ago

Given the fact that some devices cannot report if e.g. heating or cooling equipment is currently running, if we keep the operational mode as an attribute, what should the the entity's actual state be? A placeholder state like STATE_UNKNOWN or STATE_ON that never changes?

@balloob When a device is in "eco" mode it will use different (not exposed directly) setpoints than when it is in "auto" mode. How should this be expressed to the user?

cgarwood commented 6 years ago

"The frontend can calculate itself what the current state would be?"

Not necessarily. I know with my Nest atleast the heat is set to not kick on until it drops 2 degrees below the setpoint. If we're just comparing mode against current and target temps, HA will be saying the unit is running a lot more than it actually is. Most of the smart thermostats report back their current state in the APIs, I think that's the best source of truth for what the state of the device is, as opposed to trying to calculate if it's actually running or not based on other factors.

I think "heating" "cooling" "idle" "off" etc would be the best states for the actual climate entity.

trisk commented 6 years ago

"The frontend can calculate itself what the current state would be?"

Not necessarily. I know with my Nest atleast the heat is set to not kick on until it drops 2 degrees below the setpoint. If we're just comparing mode against current and target temps, HA will be saying the unit is running a lot more than it actually is.

Agreed, we don't always have access to all of the parameters for the state machine used by the device internally, which in modern devices may be more complex than "start heat/cool when above/below X with a hysteresis of Y". This is almost certainly not something that can be simulated by a generic frontend. We could suggest that platforms which don't report the current state to simulate it if the logic is known and the necessary parameters are exposed by the device (and fixed between firmware updates) but this is not applicable to all platforms.

I think "heating" "cooling" "idle" "off" etc would be the best states for the actual climate entity.

Is there value in including "off" as a different state from "idle"?

cgarwood commented 6 years ago

Is there value in including "off" as a different state from "idle"?

Good point, probably not. My initial thought was using "off" if the thermostat was set to off, and idle if it was set to heat/cool but not actually running. But you could figure out the difference there based on what mode the thermostat is set to, so there's not really value in separate off and idle states

balloob commented 6 years ago

@trisk: When a device is in "eco" mode it will use different (not exposed directly) setpoints than when it is in "auto" mode. How should this be expressed to the user?

So Eco is just AUTO with different setpoints. The setpoints would be target_temp_high and target_temp_low?

balloob commented 6 years ago

We can include an hvac_state attribute but as it stands right now in the frontend, we won't show it because state will become equal to hvac_mode. If it it's not available for a device (because we can't know), we'll just return None.

trisk commented 6 years ago

So Eco is just AUTO with different setpoints. The setpoints would be target_temp_high and target_temp_low?

"eco" mode overrides the original setpoints with temporary values (like away mode) and restores the original setpoints once the device leaves "eco" mode. The "eco" mode setpoints usually require different API calls and in some cases do not allow arbitrary changes (just a range, or read-only).

Not sure if the regular target_temp_* attributes should reflect eco mode or if they should be in a separate attribute. Regardless there needs to be some way to indicate and enter/leave eco mode.

balloob commented 6 years ago

So a profile attribute right? Profile is "normal" or "eco". targettemp* should represent the current used setpoints. If a current set point is not used because a different profile is selected, it should not be part of the current state.

zxdavb commented 6 years ago

IMHO, there is a hierarchy of modes/states. The zone thermostat may be in Auto, but it would be 'overruled' by the hub, which is dictating (say) Away or Eco mode... Unless the zone itself detects an open window, and sets the target temp to 5C until the window is closed...

Many of the newer heating systems have a relatively complex hierarchy regarding Operating Mode, maybe like:

We have to be careful about derived values - for example, setting a heating object to IDLE just because its target temp is lower than its current temp is folly. For example, the zone calls for heat, even though its current temp is higher than its target temp because it knows that in 30 mins it needs to be (say) 5C higher than it is now.

TL;DR - HA cannot expect to encompass all climate objects, and so some flexibility is required. For example:

========================

I am in the throws of re-writing the EU (US is different) Honeywell evohome component, as a custom component because the existing (honeywell) component is no longer up to the task. I have some observations that are directly relevant to the above, and some additional issues. It may be useful to describe how the evohome system works...

An evohome account has multiple locations, each with multiple gateways, each with multiple controllers, each with up to 12 zones. The vast majority of installations would consist of one account/location/gateway with one zone (and the existing code assumes this).

However it is sufficient, I think, to assume a 1 to many relationship by merging account/location/gateway/controller into one, to get a controller:zones(s) setup in 1:many (I know others have asked for multiple locations, but the underlying API actively does not currently support this). The zones (each is a collection of actuators and one thermostat) act as you'd expect but have 3 modes (this is not strictly true):

Note that zones do not have an On, Off, or Away state; they are always on (and it is not possible to know if they are active, or idle). For an Away mode, the target temp may be set to (say) 10C, and for Off, it would (usually) be 5C.

The controller has no temperature, and no target temperature. It does have the following modes (in addition to 'Auto' - follow the programmed schedule) that override the target temperature of any zone in 'FollowSchedule' mode

Sorry if I have rambled on.

zxdavb commented 6 years ago

So a profile attribute right? Profile is "normal" or "eco". targettemp* should represent the current used setpoints. If a current set point is not used because a different profile is selected, it should not be part of the current state.

I agree (but there would be a hierarchy of these profiles). The way I think about it is that the schedule consists of a whole bunch of setpoints (from a particular time the target temp is...), unless overridden by a mode (you say profile), or some other state.

The hierarchy of operating modes (and other event-driven modes) is resolved against the schedule to produce a current target temp. Thus, the target_temp and current_temp are states of the zone. If I want to know the scheduled setpoint, then (for evohome), the only way to know is to interrogate the schedule.

Only if the Controller is in Auto mode and the Zone is in FollowSchedule mode, would the target temp then be the setpoint, unless (for example) the zone is in 'OpenWindow' state (there are other such states).

So Eco is just AUTO with different setpoints. The setpoints would be target_temp_high and target_temp_low?

With evohome, if the Controller is in AutoWithEco mode, the the zones that are in FollowSchedule mode follow their scheduled setpoints, only 3C lower; they only have a target_temp.

balloob commented 6 years ago

The state of a climate component should always show the actual state. So if we're in eco mode and because of that the target setpoint changes, we should show the eco target as our target temperature, as that's what is the current state. What is overriden is not important.

We will include both operation mode and operation state as attributes and will make the state be equal to the operation mode.

It is currently out of scope for our climate entity to also represent a multizone setup with temporary overrides. We first need to fix it up to a place where it will support the majority of the thermostat models out there.

zxdavb commented 6 years ago

Actually, it would be enough to decouple operation mode (as requested) from operating state (as it eventuates for whatever reason) - the controller can be instantiated with different supported_features to its zones.

How do you feel about a climate object without a temperature?

maufl commented 6 years ago

Just for your information, here's the way MAX! thermostats work (and also the eq3 bluetooth smart, basically the same device with different RF).

The are mounted directly on the radiator. You can program a weekly schedule. Then you can switch between three operation modes, manual, auto and boost. (There is also temporary, but I don't remember what it does).

In manual mode, the thermostat tries to keep the currently set temperature. In auto mode, it follows the weekly schedule. In boost mode it fully opens the valve for a configurable amount of time and then goes back into the previous mode.

Modes like heat, cool or idle don't make much sense for these devices. The auto mode is probably not very interesting in the context of homeassistant, as most users would probably use automations and not the schedule in the thermostat. Manual is the preferred mode, because the thermostat will then not change the target temperature it self. However, boost mode is also interesting as it can be used to quickly heat up a room.

worm-ee commented 6 years ago

For HomematicIP_cloud the same as for MAX! thermostats holds. A STATE_BOOST and several STATE_AUTO1...5? for different time/temperature profile would be great.

balloob commented 6 years ago

Profiles are not hvac states, they are profiles.

worm-ee commented 6 years ago

Hi @balloob, but you have also to admit a time/temperature profile is a kind of automatic state for devices like a radiator thermostat. I just have implemented the climatic platform for homematicip_cloud and I had in mind to implement the profiles as different automatic modes. This has been refused and I also fully support this if you wanna keep things consistent.
https://github.com/home-assistant/home-assistant/pull/14388

maufl commented 5 years ago

What's the status on this? This seems to block some climate platform implementations.

balloob commented 5 years ago

Somebody needs to implement it.

maufl commented 5 years ago

Is there even a consensus on how to implement it yet? Implementing the changes would probably also require rewriting all existing platforms right?

balloob commented 5 years ago

Correct. It will cause some breaking changes

pilehave commented 5 years ago

Obviusly this needs to be sorted out, before adding anymore fancy bells and whistles, like new gauges and a new UI. Important stuff first, please ;-)

KlaasH commented 5 years ago

This is an attempt to summarize the target implementation of mode/state/profile, since there has been a bit of discussion which I think has evolved and clarified things since the initial comment. But it's an opinionated summary, including some opinions I don't think have been expressed above.

Operation mode

Defines what action(s) the thermostat is instructed to take. Where verbs make sense, use imperative.

Values: heat, cool, auto, off, auxHeatOnly, dry, ~humidify~

State

What the thermostat is currently instructing the controlled system(s) to do, i.e. the action of the system based on the mode and target temperature (/humidity). Use progressive verbs to avoid confusion/conflation.

Values: heating, cooling, idle, drying, ~humidifying~

Profile

This concept is less clear and much more device specific. Maybe it defines what approach should be taken in carrying out the action (i.e. eco, normal, comfort, boost)? Or what motivates the current targets (home, away, bed, override)? This might not be clear enough, but then again it might be OK for this to be a more nebulous category for anything the thermostat uses to describe its operation that doesn't clearly fit into "operation mode" or "state" as defined above.

Notes, guidelines, questions:

balloob commented 5 years ago

Thanks for summarizing it @KlaasH. It looks like you included humidify for operation and operating mode, but we should remove that. It's the same as dry (as per this comment).

KlaasH commented 5 years ago

I included it based on this comment and because it seems reasonable that there'd be a corollary to dry for increasing humidity. But I don't know if there are actually systems that have an operation mode specific to targeting higher humidity (in my head it's possible, maybe in the desert?), and if it's not a real thing then it definitely shouldn't be taking up space in the component. I edited the comment to strike them out.