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

Architectural review of climate entity #22

Closed
balloob opened this issue Mar 27, 2018 · 111 comments
Closed

Architectural review of climate entity #22

balloob opened this issue Mar 27, 2018 · 111 comments
Assignees

Comments

@balloob
Copy link
Member

balloob commented Mar 27, 2018

This issue was triggered by home-assistant/core#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.

  • Action: merge temp lower and temp higher
  • Action: merge humidithy lower and temp higher

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.

  • Action: rename constants from STATE_* to OPERATION_MODE_*
  • Action: list violating platforms and open issues to get them fixed

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:

  • Fan only means the current operation is idle. The fan state should show if fan is on. Can we remove this one?
  • eco, performance, high demand: these sound like performance profiles. Should be represented differently? (also performance and high demand are the same?)
  • heat pump: this needs to be merged with heat

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

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

tinloaf commented Mar 27, 2018

Some obervations, some of which come from the stale home-assistant/home-assistant.io#3910 :

  • Regarding on/off: Yes, idle is an observation, @andrey-git . I think that every property of a climate device should have a current_* and a requested_* version. Thus, if the requested_operation_mode is e.g., auto, the current_operation_mode can still be idle to indicate that the device is currently not doing anything. Obviously, when the requested_operation_mode is set to off, the current_operation_mode should also go to off, or the device is doing something wrong. This way, we can get rid of the separate on/off property, I think.

  • I'm all in favor of throwing out econet. Maybe it should become its own component that offers some switches or something?

  • I still think we need to split up the values we allow for operation_mode, something like the suggested action (which has a very limited range of allowed values, e.g., off, auto, heat and cool). Maybe we would then need to add the same thing for humidity (humidity_action? With values off, auto, humidify and dry?) Everything else should go somewhere else. A value of electric or fan_only just isn't the same type of choice as auto vs. heat.

@pvizeli
Copy link
Member

pvizeli commented Mar 27, 2018

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

fanthos commented Mar 27, 2018

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

cdce8p commented Mar 27, 2018

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

fanthos commented Mar 27, 2018

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

cdce8p commented Mar 27, 2018

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

@trisk
Copy link

trisk commented Mar 27, 2018

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

trisk commented Mar 27, 2018

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

trisk commented Mar 27, 2018

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

trisk commented Mar 27, 2018

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

balloob commented Mar 27, 2018

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

balloob commented Mar 27, 2018

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

Landrash commented Mar 29, 2018

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

  • Idle with mine equals to it waiting and adjusting temperature when necessary.
  • Off equals to it doing no actions.

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

@trisk
Copy link

trisk commented Mar 29, 2018

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

trisk commented Mar 29, 2018

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

balloob commented Mar 31, 2018

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:

  • Introduce a new property hvac_mode. Limited to off, cool, heat, auto, auxHeatOnly (for ecobee). Operation mode values are defined via constants starting with names HVAC_MODE_.
  • Propertyoperation_list will be renamed to hvac_modes. It will list all available HVAC operation modes for this device.
  • current_operation property will be renamed to hvac_state and will be limited to off, cool, heat. Current operation values are defined in constants starting with names HVAC_STATE_.
  • state will return hvac_state

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

@fanthos
Copy link

fanthos commented Mar 31, 2018

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

@balloob
Copy link
Member Author

balloob commented Mar 31, 2018

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

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

pvizeli commented Mar 31, 2018

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

balloob commented Mar 31, 2018

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

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

Hvac_mode could also be fan or dry.

@balloob
Copy link
Member Author

balloob commented Mar 31, 2018

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

hthiery commented Apr 2, 2018

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

balloob commented Apr 2, 2018

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.

@zxdavb
Copy link

zxdavb commented Feb 15, 2019

@OnFreund asked: > @elupus do you have an example of such a device so we can understand better? Also, what is the interface? Is it connected in any way?

A lot of heating systems are all about moving heat from A to B - e.g. as warm air (I guess HVAC, but I've already demonstrated my relative inexperience there), or water (i.e the hydronic central heating systems as per UK/Europe).

With my 3 different makes (brands) of combi boiler (I'm in the UK), they all have means to vary the temperature of the CV (the Circulating Volume of water that leave the boiler, passes through all the open radiators, and returns to the boiler to be re-heated).

With the combi boiler/CV system, there are two considerations: a) having the return temperature of the CV (i.e. before it is re-heated) below 55C (makes them much more efficient), and b) modulating the CV temp to compensate for how quickly the house loses heat to the outside (the heat gradient).

  1. Assuming the target temp for a room is 20C: outside air temp @ 15C , then CV = 45C, or if @ 0C, then CV=65C (to compensate for increased rate of heat loss to outside) -- google 'Weather Compensation' (curve). On my boilers the curve parameters can be adjusted, and the outside air temp can either be from a device attached to an outside wall, or an Internet source.

  2. Or, it the target temp is 20C: current room temp @ 14C (just left 'away' mode, then CV=65C, or current temp =19C, then CV=45C (to prevent overshoots) -- google 'OpenTherm'.

Quite a few boiler support adjusting the upper limit of the CV via an api. One of my boilers allows the parameters of the entire curve to be adjusted via an app (not a public API, but hackable). All boilers support WC or OpenThem or your choice of one of the two.

(BTW, there are other considerations, like when the radiators are too small for their rooms, etc - this is less likely for HVAC as they'd have a tendency to be better designed.

@OnFreund
Copy link

Thanks @zxdavb
Seems like there's indeed more complexity in the source than I've initially thought. I'm not sure if there's a common interface to be extracted here, but that's a separate discussion.
In any case, what I want to re-iterate, is that any such split between source and thermostat should keep the thermostat fully capable to run the day to day operation. In the physical world, the thermostat is the interface through which 99% (or more) of the daily interaction goes, and I think we should keep it as such in Hass.

@Swamp-Ig
Copy link

@OnFreund Not always. On my ducted aircon system the thermostats are there for each zone (room) and are quite simple essentially on-off devices, the temperature they pick up determines the airflow into the area. The HVAC system master controller has all the essential modes like heat/cool/auto and fan speed, and is even used to set the target temp for each zone.

@elupus
Copy link

elupus commented Feb 17, 2019

@swamp-lg does that really matter? If the entitity called termostat controls settings for a zone. Does it really matter if that is actuated by the heater? The hvac entitity would handle global things. The termostat entities zone local things.

@zxdavb
Copy link

zxdavb commented Feb 17, 2019

In many ways HVAC & thermostats are similar. You could even say thermostats are a true subset of HVAC.

I think thermostats should be separate because they are common, and much simpler than HVAC (thermostats have only one 'dimension', temperature). Much like fan is a subset of the current climate implementation.

@b4dpxl
Copy link

b4dpxl commented Mar 2, 2019

This should also consider the generic_thermostat, which has no scheduling or similar capabilities of its own. At the moment, with generic_thermostat heating schedules have to be set via automations, and these override any temperature set by "away mode", rendering it unusable: see #20154

@jnimmo
Copy link

jnimmo commented Mar 20, 2019

Here are my thoughts, coming from a primarily heat pump/air conditioner user perspective rather than boilers/radiators

  1. Fan only mode is important to keep, I would expect to see
    Operation Mode/State: heat, cool, fan_only, dry, auto, off
    Every heat pump/air conditioning unit certainly used in my part of the world has these modes.
  2. Fan mode should be changed to fan speed. This could potentially be a percentage or just a 1-5, and let each platform handle how to convert that into their equivalent settings.
  3. Current operation (in other words, is the room already up to temperature) should be optional; as should current ambient temperature; some devices are a one way integration only and not capable of sending any information back.
  4. Target temperature should return None if there is not a current target temperature set (i.e. in fan only mode), or if the device is off.

Additional -

  • Some HVAC units have a humidifier included too. That could be an extra supported feature, which would then require On/Off, target humidity

  • It would be nice to show how hard the unit is working if it is able to supply that information - probably as a percentage

  • Eco/Powerful mode; personally don't think these are necessary but many devices to support this so could be included as a separate mode to the state, i.e. Heating in Eco or Powerful mode. Nothing needs to change about temperatures etc that is just up to the platform/device to handle.

  • Swing mode: would be nice to have optional horizontal and vertical swing modes as well as vent position- i.e left to right (1-5) vertical and high to low
    SUPPORT_HORIZONTAL_VANE
    SUPPORT_VERTICAL_VANE

@shbatm
Copy link

shbatm commented Apr 23, 2019

Trying to generate some momentum with this thread:

To me Operation Mode ≠ State. Operation Mode should be the system's current setting (as @jnimmo put it: Heat, Cool, Fan_Only, Dry, Auto, Off). State should be what the system is currently doing: Heating, Cooling, Idle, Drying, Off. I can have my current_operation mode set to HEAT, but the furnace might not be on at the moment because it is within temperature range--if I am reporting state as current_operation I have no way of knowing if the unit is actually running or not. I understand that not all climate devices can report their status--in these cases, let the individual implementations report back that the state = current_operation--but do not restrict the devices that can differentiate from doing so (goes to @jnimmo's Point 3).

For Fan_Mode: Couldn't devices that let you set the speed of the fan just report a separate device as a fan entity?

Finally, away_mode is not fully implemented. What about combining Hold Mode, Program/Schedule Mode, Away Mode, Eco Mode, Powerful Mode, etc, into an "Auxiliary Modes" attribute and let each device report back what mode(s) it is currently in?

Side note to @balloob -- is there a way to implement polls or a better location for this conversation to help drive consensus on these issues? Maybe it'd be better to break this up into separate sub-issues that we can discuss?

EDIT: Just also wanted to add--borrowing from the UDI WSDK which has it's own mini-bible of Units of Measure/State--here is a potential more "exhaustive" list of valid modes:

Thermostat heat/cool state: 0 - Idle, 1 - Heating, 2 - Cooling , 3 - Fan Only, 4 - Pending Heat, 5 - Pending Cool, 6 - Vent, 7 - Aux Heat, 8 - 2nd Stage Heating, 9 - 2nd Stage Cooling, 10 - 2nd Stage Aux Heat, 11 - 3rd Stage Aux Heat

Thermostat mode: 0 - Off, 1 - Heat, 2 - Cool, 3 - Auto, 4 - Aux/Emergency Heat, 5 - Resume, 6 - Fan Only, 7 - Furnace, 8 - Dry Air, 9 - Moist Air, 10 - Auto Changeover, 11 - Energy Save Heat, 12 - Energy Save Cool, 13 - Away

Thermostat fan mode: 0 - Auto, 1 - On, 2 - Auto High, 3 - High, 4 - Auto Medium, 5 - Medium, 6 - Circulation, 7 - Humidity Circulation

Thermostat fan running state: 0 - Off, 1 - On, 2 - On High, 3 - On Medium, 4 – Circulation, 5 - Humidity Circulation, 6 - Right/Left Circulation, 7 - Up/Down Circulation, 8 - Quiet Circulation

@MatsNl
Copy link

MatsNl commented Jun 14, 2019

Hi!

Not sure this is the correct place to bring this, but it seems mostly related.
I ran into the same operation mode vs state issue while developing a new climate platform (for atag thermostat). I'll hold off on further development of that component until climate 1.0 is merged, but I was wondering whether the same logic will be applied also to the water heater component? There too I run into the problem where state is not the same as current_operation. e.g. eco vs comfort as operation mode, but state can be Idle / heating in my case.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jul 3, 2019
@pvizeli
Copy link
Member

pvizeli commented Jul 3, 2019

I locked this conversation because over 105 coments are not able to track anymore and second this issue is fix with climate 1.0

For things after climate 1.0 / open a new issue

@MartinHjelmare
Copy link
Member

Can we close this now that the climate 1.0 PR is merged?

Copy link
Member

frenck commented Jul 12, 2019

The ADR itself still needs to be made?

@MartinHjelmare
Copy link
Member

Do we need to do ADRs for component api changes? I'd assume that the api and dev docs speaks for itself.

Copy link
Member

frenck commented Jul 12, 2019

Quoting @balloob who I asked this question before: "ADR should be everything changed to core.", that makes it a yes I guess?

@frenck
Copy link
Member

frenck commented Jan 31, 2021

Alright time has come to close this one up 👍

@frenck frenck closed this as completed Jan 31, 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