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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MQTT expire_after effects after reloading #65359

Merged
merged 11 commits into from Feb 2, 2022
41 changes: 40 additions & 1 deletion homeassistant/components/mqtt/binary_sensor.py
Expand Up @@ -20,13 +20,16 @@
CONF_PAYLOAD_OFF,
CONF_PAYLOAD_ON,
CONF_VALUE_TEMPLATE,
STATE_UNAVAILABLE,
STATE_UNKNOWN,
)
from homeassistant.core import HomeAssistant, callback
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity_platform import AddEntitiesCallback
import homeassistant.helpers.event as evt
from homeassistant.helpers.event import async_track_point_in_utc_time
from homeassistant.helpers.reload import async_setup_reload_service
from homeassistant.helpers.restore_state import RestoreEntity
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType
from homeassistant.util import dt as dt_util

Expand Down Expand Up @@ -96,7 +99,7 @@ async def _async_setup_entity(
async_add_entities([MqttBinarySensor(hass, config, config_entry, discovery_data)])


class MqttBinarySensor(MqttEntity, BinarySensorEntity):
class MqttBinarySensor(MqttEntity, BinarySensorEntity, RestoreEntity):
"""Representation a binary sensor that is updated by MQTT."""

_entity_id_format = binary_sensor.ENTITY_ID_FORMAT
Expand All @@ -114,6 +117,42 @@ def __init__(self, hass, config, config_entry, discovery_data):

MqttEntity.__init__(self, hass, config, config_entry, discovery_data)

async def async_added_to_hass(self) -> None:
"""Restore state for entities with expire_after set."""
await super().async_added_to_hass()
if (
(expire_after := self._config.get(CONF_EXPIRE_AFTER)) is not None
and expire_after > 0
and (last_state := await self.async_get_last_state()) is not None
and last_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]
):
expiration_at = last_state.last_changed + timedelta(seconds=expire_after)
if expiration_at < (time_now := dt_util.utcnow()):
# Skip reactivating the binary_sensor
_LOGGER.debug("Skip state recovery after reload for %s", self.entity_id)
return
self._expired = False
self._state = last_state.state

self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self._value_is_expired, expiration_at
)
_LOGGER.debug(
"State recovered after reload for %s, remaining time before expiring %s",
self.entity_id,
expiration_at - time_now,
)

async def async_will_remove_from_hass(self) -> None:
"""Remove exprire triggers."""
# Clean up expire triggers
if self._expiration_trigger:
_LOGGER.debug("Clean up expire after trigger for %s", self.entity_id)
self._expiration_trigger()
self._expiration_trigger = None
self._expired = False
await MqttEntity.async_will_remove_from_hass(self)

@staticmethod
def config_schema():
"""Return the config schema."""
Expand Down
5 changes: 5 additions & 0 deletions homeassistant/components/mqtt/mixins.py
Expand Up @@ -523,6 +523,11 @@ async def discovery_callback(payload):
async def async_removed_from_registry(self) -> None:
"""Clear retained discovery topic in broker."""
if not self._removed_from_hass:
# Stop subscribing to discovery updates to not trigger when we clear the
# discovery topic
self._cleanup_discovery_on_remove()

# Clear the discovery topic so the entity is not rediscovered after a restart
Comment on lines +526 to +530
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record: This fixes an old bug which surfaced due to changed timing when MqttSensor and MqttBinarySensor extend RestoreEntity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug fix also applies for other platforms like MqttSwitch

discovery_topic = self._discovery_data[ATTR_DISCOVERY_TOPIC]
publish(self.hass, discovery_topic, "", retain=True)

Expand Down
41 changes: 40 additions & 1 deletion homeassistant/components/mqtt/sensor.py
Expand Up @@ -23,12 +23,15 @@
CONF_NAME,
CONF_UNIT_OF_MEASUREMENT,
CONF_VALUE_TEMPLATE,
STATE_UNAVAILABLE,
STATE_UNKNOWN,
)
from homeassistant.core import HomeAssistant, callback
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity_platform import AddEntitiesCallback
from homeassistant.helpers.event import async_track_point_in_utc_time
from homeassistant.helpers.reload import async_setup_reload_service
from homeassistant.helpers.restore_state import RestoreEntity
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType
from homeassistant.util import dt as dt_util

Expand Down Expand Up @@ -140,7 +143,7 @@ async def _async_setup_entity(
async_add_entities([MqttSensor(hass, config, config_entry, discovery_data)])


class MqttSensor(MqttEntity, SensorEntity):
class MqttSensor(MqttEntity, SensorEntity, RestoreEntity):
"""Representation of a sensor that can be updated using MQTT."""

_entity_id_format = ENTITY_ID_FORMAT
Expand All @@ -160,6 +163,42 @@ def __init__(self, hass, config, config_entry, discovery_data):

MqttEntity.__init__(self, hass, config, config_entry, discovery_data)

async def async_added_to_hass(self) -> None:
"""Restore state for entities with expire_after set."""
await super().async_added_to_hass()
if (
(expire_after := self._config.get(CONF_EXPIRE_AFTER)) is not None
and expire_after > 0
and (last_state := await self.async_get_last_state()) is not None
and last_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]
):
expiration_at = last_state.last_changed + timedelta(seconds=expire_after)
if expiration_at < (time_now := dt_util.utcnow()):
# Skip reactivating the sensor
_LOGGER.debug("Skip state recovery after reload for %s", self.entity_id)
return
self._expired = False
self._state = last_state.state

self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self._value_is_expired, expiration_at
)
_LOGGER.debug(
"State recovered after reload for %s, remaining time before expiring %s",
self.entity_id,
expiration_at - time_now,
)

async def async_will_remove_from_hass(self) -> None:
"""Remove exprire triggers."""
# Clean up expire triggers
if self._expiration_trigger:
_LOGGER.debug("Clean up expire after trigger for %s", self.entity_id)
self._expiration_trigger()
self._expiration_trigger = None
self._expired = False
await MqttEntity.async_will_remove_from_hass(self)

@staticmethod
def config_schema():
"""Return the config schema."""
Expand Down
91 changes: 90 additions & 1 deletion tests/components/mqtt/test_binary_sensor.py
Expand Up @@ -36,6 +36,7 @@
help_test_entity_device_info_with_identifier,
help_test_entity_id_update_discovery_update,
help_test_entity_id_update_subscriptions,
help_test_reload_with_config,
help_test_reloadable,
help_test_setting_attribute_via_mqtt_json_message,
help_test_setting_attribute_with_template,
Expand All @@ -44,7 +45,11 @@
help_test_update_with_json_attrs_not_dict,
)

from tests.common import async_fire_mqtt_message, async_fire_time_changed
from tests.common import (
assert_setup_component,
async_fire_mqtt_message,
async_fire_time_changed,
)

DEFAULT_CONFIG = {
binary_sensor.DOMAIN: {
Expand Down Expand Up @@ -872,3 +877,87 @@ async def test_reloadable(hass, mqtt_mock, caplog, tmp_path):
domain = binary_sensor.DOMAIN
config = DEFAULT_CONFIG[domain]
await help_test_reloadable(hass, mqtt_mock, caplog, tmp_path, domain, config)


async def test_cleanup_triggers_and_restoring_state(
hass, mqtt_mock, caplog, tmp_path, freezer
):
"""Test cleanup old triggers at reloading and restoring the state."""
domain = binary_sensor.DOMAIN
config1 = copy.deepcopy(DEFAULT_CONFIG[domain])
config1["name"] = "test1"
config1["expire_after"] = 30
config1["state_topic"] = "test-topic1"
config2 = copy.deepcopy(DEFAULT_CONFIG[domain])
config2["name"] = "test2"
config2["expire_after"] = 5
config2["state_topic"] = "test-topic2"

freezer.move_to("2022-02-02 12:01:00+01:00")

assert await async_setup_component(
hass,
binary_sensor.DOMAIN,
{binary_sensor.DOMAIN: [config1, config2]},
)
await hass.async_block_till_done()
async_fire_mqtt_message(hass, "test-topic1", "ON")
state = hass.states.get("binary_sensor.test1")
assert state.state == "on"

async_fire_mqtt_message(hass, "test-topic2", "ON")
state = hass.states.get("binary_sensor.test2")
assert state.state == "on"

freezer.move_to("2022-02-02 12:01:10+01:00")

await help_test_reload_with_config(
hass, caplog, tmp_path, domain, [config1, config2]
)
assert "Clean up expire after trigger for binary_sensor.test1" in caplog.text
assert "Clean up expire after trigger for binary_sensor.test2" not in caplog.text
assert (
"State recovered after reload for binary_sensor.test1, remaining time before expiring"
in caplog.text
)
assert "State recovered after reload for binary_sensor.test2" not in caplog.text

state = hass.states.get("binary_sensor.test1")
assert state.state == "on"

state = hass.states.get("binary_sensor.test2")
assert state.state == STATE_UNAVAILABLE

async_fire_mqtt_message(hass, "test-topic1", "OFF")
state = hass.states.get("binary_sensor.test1")
assert state.state == "off"

async_fire_mqtt_message(hass, "test-topic2", "OFF")
state = hass.states.get("binary_sensor.test2")
assert state.state == "off"


async def test_skip_restoring_state_with_over_due_expire_trigger(
hass, mqtt_mock, caplog, freezer
):
"""Test restoring a state with over due expire timer."""

freezer.move_to("2022-02-02 12:02:00+01:00")
domain = binary_sensor.DOMAIN
config3 = copy.deepcopy(DEFAULT_CONFIG[domain])
config3["name"] = "test3"
config3["expire_after"] = 10
config3["state_topic"] = "test-topic3"
fake_state = ha.State(
"binary_sensor.test3",
"on",
{},
last_changed=datetime.fromisoformat("2022-02-02 12:01:35+01:00"),
)
with patch(
"homeassistant.helpers.restore_state.RestoreEntity.async_get_last_state",
return_value=fake_state,
), assert_setup_component(1, domain):
assert await async_setup_component(hass, domain, {domain: config3})
await hass.async_block_till_done()
assert "Skip state recovery after reload for binary_sensor.test3" in caplog.text
36 changes: 22 additions & 14 deletions tests/components/mqtt/test_common.py
Expand Up @@ -1525,6 +1525,25 @@ async def help_test_publishing_with_custom_encoding(
mqtt_mock.async_publish.reset_mock()


async def help_test_reload_with_config(hass, caplog, tmp_path, domain, config):
"""Test reloading with supplied config."""
new_yaml_config_file = tmp_path / "configuration.yaml"
new_yaml_config = yaml.dump({domain: config})
new_yaml_config_file.write_text(new_yaml_config)
assert new_yaml_config_file.read_text() == new_yaml_config

with patch.object(hass_config, "YAML_CONFIG_FILE", new_yaml_config_file):
await hass.services.async_call(
"mqtt",
SERVICE_RELOAD,
{},
blocking=True,
)
await hass.async_block_till_done()

assert "<Event event_mqtt_reloaded[L]>" in caplog.text


async def help_test_reloadable(hass, mqtt_mock, caplog, tmp_path, domain, config):
"""Test reloading an MQTT platform."""
# Create and test an old config of 2 entities based on the config supplied
Expand All @@ -1549,21 +1568,10 @@ async def help_test_reloadable(hass, mqtt_mock, caplog, tmp_path, domain, config
new_config_2["name"] = "test_new_2"
new_config_3 = copy.deepcopy(config)
new_config_3["name"] = "test_new_3"
new_yaml_config_file = tmp_path / "configuration.yaml"
new_yaml_config = yaml.dump({domain: [new_config_1, new_config_2, new_config_3]})
new_yaml_config_file.write_text(new_yaml_config)
assert new_yaml_config_file.read_text() == new_yaml_config

with patch.object(hass_config, "YAML_CONFIG_FILE", new_yaml_config_file):
await hass.services.async_call(
"mqtt",
SERVICE_RELOAD,
{},
blocking=True,
)
await hass.async_block_till_done()

assert "<Event event_mqtt_reloaded[L]>" in caplog.text
await help_test_reload_with_config(
hass, caplog, tmp_path, domain, [new_config_1, new_config_2, new_config_3]
)

assert len(hass.states.async_all(domain)) == 3

Expand Down