Updated generic thermostat to respect operation_mode and added away mode by ciotlosm · Pull Request #11325 · home-assistant/core

ciotlosm · 2017-12-27T15:07:22Z

Description:

Add away mode
Fixed Operation selection bug
Fixed error on startup, fixes Climate Generic Thermostat always generating error on HASS start #11273
Includes functionality in [WIP] Add away mode support for generic thermostat #11081 (that was original source for away_mode changes)

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4285

Example entry for `configuration.yaml` (if applicable):

climate:
  - platform: generic_thermostat
    name: Study
    heater: switch.study_heater
    target_sensor: sensor.study_temperature
    cold_tolerance: 0.3
    hot_tolerance: 0.3
    away_temp: 18

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

houndci-bot · 2017-12-27T15:07:31Z

 @asyncio.coroutine
 def test_no_restore_state(hass):
-    """Ensure states are not restored on startup if not needed."""
+    """Ensure states are restored on startup if they exist. Allows for graceful reboot."""


line too long (90 > 79 characters)

houndci-bot · 2017-12-27T15:07:31Z

+        self._is_away = False
+        self._target_temp = self._saved_target_temp
+        self._async_control_heating()
+        self.schedule_update_ha_state()


no newline at end of file

houndci-bot · 2017-12-27T15:07:32Z

+            self._target_temp = self._away_temp_cool
+        else:
+            self._target_temp = self._away_temp_heat
+


blank line contains whitespace

houndci-bot · 2017-12-27T15:07:32Z

                 min_temp, max_temp, target_temp, ac_mode, min_cycle_duration,
-                 cold_tolerance, hot_tolerance, keep_alive,
-                 initial_operation_mode):
+                 cold_tolerance, hot_tolerance, keep_alive, away_temp_heat, away_temp_cool):


line too long (92 > 79 characters)

houndci-bot · 2017-12-27T15:07:32Z

+    vol.Optional(CONF_AWAY_TEMP_HEAT,
+        default=DEFAULT_AWAY_TEMP_HEAT): vol.Coerce(float),
+    vol.Optional(CONF_AWAY_TEMP_COOL,
+        default=DEFAULT_AWAY_TEMP_COOL): vol.Coerce(float)


continuation line under-indented for visual indent

houndci-bot · 2017-12-27T15:07:32Z

-    vol.Optional(CONF_INITIAL_OPERATION_MODE):
-        vol.In([STATE_AUTO, STATE_OFF])
+    vol.Optional(CONF_AWAY_TEMP_HEAT,
+        default=DEFAULT_AWAY_TEMP_HEAT): vol.Coerce(float),


continuation line under-indented for visual indent

houndci-bot · 2017-12-27T15:07:32Z

-SUPPORT_FLAGS = SUPPORT_TARGET_TEMPERATURE | SUPPORT_OPERATION_MODE
+CONF_AWAY_TEMP_COOL = 'away_temp_cool'
+CONF_AWAY_TEMP_HEAT = 'away_temp_heat'
+SUPPORT_FLAGS = (SUPPORT_TARGET_TEMPERATURE | SUPPORT_AWAY_MODE | SUPPORT_OPERATION_MODE)


line too long (89 > 79 characters)

houndci-bot · 2017-12-27T15:12:34Z

trailing whitespace

houndci-bot · 2017-12-27T15:12:34Z

trailing whitespace

houndci-bot · 2017-12-27T15:12:34Z

continuation line under-indented for visual indent

houndci-bot · 2017-12-27T15:12:34Z

continuation line under-indented for visual indent

houndci-bot · 2017-12-27T15:12:34Z

continuation line under-indented for visual indent

houndci-bot · 2017-12-27T15:12:34Z

trailing whitespace

houndci-bot · 2017-12-27T18:46:03Z

blank line contains whitespace

MartinHjelmare · 2017-12-27T20:51:48Z

Setting operation mode doesn't seem to influence how the thermostat is operating, ie cooling or heating. So what's the point of enabling those options? Although _enabled can be replaced by _current_operation != STATE_OFF, so that selection will influence on/off.

Operation mode allows two options (Based on the type of generic_thermostat): Either heating or off. I was trying to fix operation_mode discrepancy initially introduced by making it use the correct concepts.

I plan to add the possibility to have both heater switch and cooling switch so you can have heating, cooling, off and also auto with the correct logic. This way you can have a separate air conditioning working together with a heating boiler to provide more complex climate control in a single component

Ah, ok, so currently it's still like an on/off setting basically.

Yes, however now the bug there the "on" or "auto" was selected and not visible on the interface is gone. Now you clearly see "heat" selected on the interface.

The only downside (which I have no solution yet) is that you lose the nice feedback on the interface when the heater is idle or heating, but the way it was represented now looked more like hack.

Later edit: I was able to fix the "state" display on the front-end with home-assistant/frontend#766

houndci-bot · 2017-12-27T21:48:13Z

line too long (96 > 79 characters)

houndci-bot · 2017-12-27T21:48:13Z

line too long (96 > 79 characters)

houndci-bot · 2017-12-27T21:48:13Z

line too long (83 > 79 characters)

houndci-bot · 2017-12-27T22:03:42Z

unexpected indentation

houndci-bot · 2017-12-27T22:03:42Z

trailing whitespace

houndci-bot · 2017-12-27T22:03:42Z

IndentationError: unexpected indent
unexpected indentation

houndci-bot · 2017-12-27T22:03:42Z

trailing whitespace

houndci-bot · 2017-12-27T22:06:08Z

SyntaxError: invalid syntax

houndci-bot · 2017-12-27T22:22:19Z

trailing whitespace

ciotlosm · 2017-12-27T23:52:10Z

Will require home-assistant/frontend#759 to keep displaying correct state (heat, idle, cool, off) and still allow for correct operation_mode

Later edit: The above PR got closed because of my mistake. Reopened correctly again as home-assistant/frontend#766

tinloaf · 2017-12-28T13:28:37Z


 DEFAULT_TOLERANCE = 0.3
 DEFAULT_NAME = 'Generic Thermostat'
+DEFAULT_AWAY_TEMP_COOL = 30


Why do you need away temperatures for both heating and cooling? Setting a heating away temperature together with ac_mode = True does not make sense, and vice versa. I guess that if the user sets ac_mode = True, he should known that the away temperature is used for cooling rather than for heating. I would just call it away_temperature.

Just read your answer to the comment by @MartinHjelmare , that you want to implement heating and cooling with the same entity. I think that's a good idea (and then you of course need both settings); however I think either both or none of it should be part of this PR, i.e., either also implement the "both heating and cooling logic", or postpone splitting the away temperature into two until a later PR.

I have to admit that I didn't think much about this. My initial intention was to fix the operation_mode and add away_mode functionality seen in another PR. I mostly reused what was developed there (#11081).
I think you are right, for now it should only be one away temperature and I will correct this until heating/cooling logic is added.

tinloaf · 2017-12-28T13:32:27Z

 CONF_COLD_TOLERANCE = 'cold_tolerance'
 CONF_HOT_TOLERANCE = 'hot_tolerance'
 CONF_KEEP_ALIVE = 'keep_alive'
-CONF_INITIAL_OPERATION_MODE = 'initial_operation_mode'


Why do you remove setting the initial operation mode? That change would make this a breaking change, which we should avoid if possible.

I was looking at usages for that specific setting, especially with the functionality to restore state from "old_state". It really didn't make much sense to have this. Also this doesn't look like any other thermostat feature observed and looked like a deviation from standards.

However looking at it from a breaking change perspective, it doesn't hurt to leave it on as you can just avoid using it.

I will take guidance here if I should leave it or remove it.

tinloaf · 2017-12-28T13:35:55Z

-            if self._target_temp is None:
-                self._target_temp = float(
-                    old_state.attributes[ATTR_TEMPERATURE])
+            self._target_temp = float(


The target temperature should only be restored from a previous state if no initial value (i.e., CONF_TARGET_TEMP) was given. The previous logic

if self._target_temp is None: self._target_temp = …

was correct.

I have to disagree from a usage perspective. This use case covers an accidental reboot of your unit, by keeping your previously set temperature. If you for example manually set a low temperature because you're away from home and your unit reboots, you will get probably a default for when you're at home. The more complex your schedule to update the temperature the more different will be your set default temperature from a restored one.

The other way around this would be to have something to set a target_temp after you try restoring it from state, and avoid having something set in settings.

Setting an initial value (for the target temperature / operation mode / whatever) has only one use case: You want this value to be set every time your HA instance starts, be it after a power loss or a manual restart. If that's not what you want, just don't set an initial value (which will then always restore the previous state). Thus, restoring the previous state even if an initial value is present basically renders the initial value useless.

I guess that makes sense. I was looking at config value as "initial" value when you have nothing saved to avoid having your target temperature as None.

The UI control doesn't look that well with "None" temperature and when starting to press arrows you start from 0 degrees regardless of your min/max settings.

Assuming everyone looks at the config value as an "always fallback" value after restart if set, I guess it would require some fixes on the UI to make the setup with "None" work better.

Yes, that's true. If you don't want an initial value to always be set on every restart, you need to configure your entity once (after you first added it) "starting from scratch". The UI should support this.

Didn't think of that option.

tinloaf · 2017-12-28T13:36:21Z

+                old_state.attributes[ATTR_TEMPERATURE])
+            self._is_away = True if str(
+                old_state.attributes[ATTR_AWAY_MODE]) == STATE_ON else False
+            if old_state.attributes[ATTR_OPERATION_MODE] == STATE_OFF:


Same holds for operation mode: If an initial value is given (see my comment above), this should not be restored from the database.

See my above comment around accidental reboots (power loss) after you set something different form initial setting which I think it's more harmful.

Here there is no initial value given, as it's computed based on ac_mode. I think restoring operation_mode based on previous makes more sense.

Oh yes, I falsely thought that you removed the initial setting for the operation mode, too. If there is no such initial value, always restoring is in fact the right thing to do. 👍

houndci-bot · 2017-12-28T14:58:15Z

        self._keep_alive = keep_alive
        self._initial_operation_mode = initial_operation_mode
+        self._saved_target_temp = target_temp if target_temp is not None \
+                else away_temp


continuation line over-indented for visual indent

houndci-bot · 2017-12-28T14:58:15Z

 from homeassistant.const import (
    ATTR_UNIT_OF_MEASUREMENT, STATE_ON, STATE_OFF, ATTR_TEMPERATURE,
-    CONF_NAME, ATTR_ENTITY_ID, SERVICE_TURN_ON, SERVICE_TURN_OFF)
+        CONF_NAME, ATTR_ENTITY_ID, SERVICE_TURN_ON, SERVICE_TURN_OFF)


continuation line unaligned for hanging indent

An error was being logged to display debug info. Removed it

* Huge ISY994 platform cleanup, fixes support for 5.0.10 firmware # * No more globals - store on hass.data # * Parent ISY994 component handles categorizing nodes in to Hass components, rather than each individual domain filtering all nodes themselves # * Remove hidden string, replace with ignore string. Hidden should be done via the customize block; ignore fully prevents the node from getting a Hass entity # * Removed a few unused methods in the ISYDevice class # * Cleaned up the hostname parsing # * Removed broken logic in the fan Program component. It was setting properties that have no setters # * Added the missing SUPPORTED_FEATURES to the fan component to indicate that it can set speed # * Added better error handling and a log warning when an ISY994 program entity fails to initialize # * Cleaned up a few instances of unecessarily complicated logic paths, and other cases of unnecessary logic that is already handled by base classes * Use `super()` instead of explicit base class calls * Move `hass` argument to first position * Use str.format instead of string addition * Move program structure building and validation to component Removes the need for a bunch of duplicate exception handling in each individual platform * Fix climate nodes, fix climate names, add config to disable climate Sensor platform was crashing when the ISY reported climate nodes. Logic has been fixed. Also added a config option to prevent climate sensors from getting imported from the ISY. Also replace the underscore from climate node names with spaces so they default to friendly names. * Space missing in error message * Fix string comparison to use `==` * Explicitly check for attributes rather than catch AttributeError Also removes two stray debug lines * Remove null checks on hass.data, as they are always null at this point

Bug in equality testing

* BC fix * more tests * inline if change * inline if change

Leak sensors were using the "wet" node as a negative node, which prevented them from ever gettng a Dry status unless the user pressed the button on the hardware after every Hass reboot. This change ignores the Wet node, as it is just always the exact inverse of the Dry node. We don't need to watch both.

…age url (#11315)

…sor state Added more tests to improve coverage and corrected again lint errors Fixed new test by moving to correct package Fixed bug not restoring away mode on restart

…ne for now

…perature

homeassistant added cla-signed platform: climate.generic_thermostat labels Dec 27, 2017