Skip to content

Fixed race condition in Generic Thermostat #15784

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.

Sign up for GitHub

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

Merged
amelchio merged 2 commits into from
Aug 12, 2018
Merged

Fixed race condition in Generic Thermostat #15784

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abbefe0
Fixed race condition in Generic Thermostat
aronsky Aug 1, 2018
c2e1d2a
Added a comment to clarify the meaning of the `time` argument.
aronsky Aug 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 59 additions & 83 deletions homeassistant/components/climate/generic_thermostat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ def __init__(self, hass, name, heater_entity_id, sensor_entity_id,
self._enabled = True
self._active = False
self._cur_temp = None
self._temp_lock = asyncio.Lock()
self._min_temp = min_temp
self._max_temp = max_temp
self._target_temp = target_temp
Expand All @@ -140,7 +141,7 @@ def __init__(self, hass, name, heater_entity_id, sensor_entity_id,

if self._keep_alive:
async_track_time_interval(
hass, self._async_keep_alive, self._keep_alive)
hass, self._async_control_heating, self._keep_alive)

sensor_state = hass.states.get(sensor_entity_id)
if sensor_state and sensor_state.state != STATE_UNKNOWN:
Expand Down Expand Up @@ -234,31 +235,30 @@ async def async_set_operation_mode(self, operation_mode):
if operation_mode == STATE_HEAT:
self._current_operation = STATE_HEAT
self._enabled = True
self._async_control_heating()
await self._async_control_heating()
elif operation_mode == STATE_COOL:
self._current_operation = STATE_COOL
self._enabled = True
self._async_control_heating()
await self._async_control_heating()
elif operation_mode == STATE_OFF:
self._current_operation = STATE_OFF
self._enabled = False
if self._is_device_active:
self._heater_turn_off()
await self._async_heater_turn_off()
else:
_LOGGER.error("Unrecognized operation mode: %s", operation_mode)
return
# Ensure we update the current operation after changing the mode
self.schedule_update_ha_state()

@asyncio.coroutine
def async_set_temperature(self, **kwargs):
async def async_set_temperature(self, **kwargs):
"""Set new target temperature."""
temperature = kwargs.get(ATTR_TEMPERATURE)
if temperature is None:
return
self._target_temp = temperature
self._async_control_heating()
yield from self.async_update_ha_state()
await self._async_control_heating()
await self.async_update_ha_state()

@property
def min_temp(self):
Expand All @@ -278,15 +278,14 @@ def max_temp(self):
# Get default temp from super class
return super().max_temp

@asyncio.coroutine
def _async_sensor_changed(self, entity_id, old_state, new_state):
async def _async_sensor_changed(self, entity_id, old_state, new_state):
"""Handle temperature changes."""
if new_state is None:
return

self._async_update_temp(new_state)
self._async_control_heating()
yield from self.async_update_ha_state()
await self._async_control_heating()
await self.async_update_ha_state()

@callback
def _async_switch_changed(self, entity_id, old_state, new_state):
Expand All @@ -295,14 +294,6 @@ def _async_switch_changed(self, entity_id, old_state, new_state):
return
self.async_schedule_update_ha_state()

@callback
def _async_keep_alive(self, time):
"""Call at constant intervals for keep-alive purposes."""
if self._is_device_active:
self._heater_turn_on()
else:
self._heater_turn_off()

@callback
def _async_update_temp(self, state):
"""Update thermostat with latest state from sensor."""
Expand All @@ -314,62 +305,51 @@ def _async_update_temp(self, state):
except ValueError as ex:
_LOGGER.error("Unable to update from sensor: %s", ex)

@callback
def _async_control_heating(self):
async def _async_control_heating(self, time=None):
"""Check if we need to turn heating on or off."""
if not self._active and None not in (self._cur_temp,
self._target_temp):
self._active = True
_LOGGER.info("Obtained current and target temperature. "
"Generic thermostat active. %s, %s",
self._cur_temp, self._target_temp)

if not self._active:
return

if not self._enabled:
return

if self.min_cycle_duration:
if self._is_device_active:
current_state = STATE_ON
else:
current_state = STATE_OFF
long_enough = condition.state(
self.hass, self.heater_entity_id, current_state,
self.min_cycle_duration)
if not long_enough:
async with self._temp_lock:
if not self._active and None not in (self._cur_temp,
self._target_temp):
self._active = True
_LOGGER.info("Obtained current and target temperature. "
"Generic thermostat active. %s, %s",
self._cur_temp, self._target_temp)

if not self._active or not self._enabled:
return

if self.ac_mode:
is_cooling = self._is_device_active
if is_cooling:
too_cold = self._target_temp - self._cur_temp >= \
self._cold_tolerance
if too_cold:
_LOGGER.info("Turning off AC %s", self.heater_entity_id)
self._heater_turn_off()
else:
too_hot = self._cur_temp - self._target_temp >= \
self._hot_tolerance
if too_hot:
_LOGGER.info("Turning on AC %s", self.heater_entity_id)
self._heater_turn_on()
else:
is_heating = self._is_device_active
if is_heating:
too_hot = self._cur_temp - self._target_temp >= \
self._hot_tolerance
if too_hot:
if self.min_cycle_duration:
if self._is_device_active:
current_state = STATE_ON
else:
current_state = STATE_OFF
long_enough = condition.state(
self.hass, self.heater_entity_id, current_state,
self.min_cycle_duration)
if not long_enough:
return

too_cold = \
self._target_temp - self._cur_temp >= self._cold_tolerance
too_hot = \
self._cur_temp - self._target_temp >= self._hot_tolerance
if self._is_device_active:
if (self.ac_mode and too_cold) or \
(not self.ac_mode and too_hot):
_LOGGER.info("Turning off heater %s",
self.heater_entity_id)
self._heater_turn_off()
await self._async_heater_turn_off()
elif time is not None:
Copy link
Copy Markdown
Contributor

@amelchio amelchio Aug 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit odd, maybe add a comment or named variable that tells the reader that this is the keepalive case?

Copy link
Copy Markdown
Contributor Author

@aronsky aronsky Aug 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll add a comment.

# The time argument is passed only in keep-alive case
await self._async_heater_turn_on()
else:
too_cold = self._target_temp - self._cur_temp >= \
self._cold_tolerance
if too_cold:
if (self.ac_mode and too_hot) or \
(not self.ac_mode and too_cold):
_LOGGER.info("Turning on heater %s", self.heater_entity_id)
self._heater_turn_on()
await self._async_heater_turn_on()
Copy link
Copy Markdown
Contributor

@amelchio amelchio Aug 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have removed the logging that mentions AC and I think that is okay because the configuration for the entity_id is indeed named heater, even in ac_mode.

Copy link
Copy Markdown
Contributor Author

@aronsky aronsky Aug 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I hope we can come up with a better, more neutral naming convention (as heater is very, well, heating-oriented :) ), and when we do, I'll update the logging accordingly.

elif time is not None:
# The time argument is passed only in keep-alive case
await self._async_heater_turn_off()

@property
def _is_device_active(self):
Expand All @@ -381,36 +361,32 @@ def supported_features(self):
"""Return the list of supported features."""
return self._support_flags

@callback
def _heater_turn_on(self):
async def _async_heater_turn_on(self):
"""Turn heater toggleable device on."""
data = {ATTR_ENTITY_ID: self.heater_entity_id}
self.hass.async_add_job(
self.hass.services.async_call(HA_DOMAIN, SERVICE_TURN_ON, data))
await self.hass.services.async_call(HA_DOMAIN, SERVICE_TURN_ON, data)

@callback
def _heater_turn_off(self):
async def _async_heater_turn_off(self):
"""Turn heater toggleable device off."""
data = {ATTR_ENTITY_ID: self.heater_entity_id}
self.hass.async_add_job(
self.hass.services.async_call(HA_DOMAIN, SERVICE_TURN_OFF, data))
await self.hass.services.async_call(HA_DOMAIN, SERVICE_TURN_OFF, data)

@property
def is_away_mode_on(self):
"""Return true if away mode is on."""
return self._is_away

def turn_away_mode_on(self):
async def async_turn_away_mode_on(self):
"""Turn away mode on by setting it on away hold indefinitely."""
self._is_away = True
self._saved_target_temp = self._target_temp
self._target_temp = self._away_temp
self._async_control_heating()
self.schedule_update_ha_state()
await self._async_control_heating()
await self.async_update_ha_state()

def turn_away_mode_off(self):
async def async_turn_away_mode_off(self):
"""Turn away off."""
self._is_away = False
self._target_temp = self._saved_target_temp
self._async_control_heating()
self.schedule_update_ha_state()
await self._async_control_heating()
await self.async_update_ha_state()