From 01f3a5a97cd39715f13666a24ecf6dd79f51e837 Mon Sep 17 00:00:00 2001 From: Jan Bouwhuis Date: Sat, 25 May 2024 01:29:43 +0200 Subject: [PATCH] Consequently ignore empty MQTT state payloads and set state to `unknown` on "None" payload (#117813) * Consequently ignore empty MQTT state payloads and set state to `unknown` on "None" payload * Do not change preset mode behavior * Add device tracker ignoring empty state * Ignore empty state for lock * Resolve merge errors --- .../components/mqtt/alarm_control_panel.py | 11 +++++ homeassistant/components/mqtt/climate.py | 11 +++-- homeassistant/components/mqtt/cover.py | 13 +++-- .../components/mqtt/device_tracker.py | 10 ++++ homeassistant/components/mqtt/lock.py | 15 ++++-- homeassistant/components/mqtt/select.py | 7 +++ homeassistant/components/mqtt/valve.py | 15 ++++-- homeassistant/components/mqtt/water_heater.py | 17 ++++++- .../mqtt/test_alarm_control_panel.py | 8 ++++ tests/components/mqtt/test_climate.py | 47 +++++++++++++++---- tests/components/mqtt/test_cover.py | 5 ++ tests/components/mqtt/test_device_tracker.py | 5 ++ tests/components/mqtt/test_lock.py | 6 +++ tests/components/mqtt/test_select.py | 14 ++++++ tests/components/mqtt/test_valve.py | 6 +++ tests/components/mqtt/test_water_heater.py | 19 +++++++- 16 files changed, 183 insertions(+), 26 deletions(-) diff --git a/homeassistant/components/mqtt/alarm_control_panel.py b/homeassistant/components/mqtt/alarm_control_panel.py index b569a32c1be..e341d54e349 100644 --- a/homeassistant/components/mqtt/alarm_control_panel.py +++ b/homeassistant/components/mqtt/alarm_control_panel.py @@ -40,6 +40,7 @@ from .const import ( CONF_RETAIN, CONF_STATE_TOPIC, CONF_SUPPORTED_FEATURES, + PAYLOAD_NONE, ) from .mixins import MqttEntity, async_setup_entity_entry_helper from .models import MqttCommandTemplate, MqttValueTemplate, ReceiveMessage @@ -176,6 +177,16 @@ class MqttAlarm(MqttEntity, alarm.AlarmControlPanelEntity): def _state_message_received(self, msg: ReceiveMessage) -> None: """Run when new MQTT message has been received.""" payload = self._value_template(msg.payload) + if not payload.strip(): # No output from template, ignore + _LOGGER.debug( + "Ignoring empty payload '%s' after rendering for topic %s", + payload, + msg.topic, + ) + return + if payload == PAYLOAD_NONE: + self._attr_state = None + return if payload not in ( STATE_ALARM_DISARMED, STATE_ALARM_ARMED_HOME, diff --git a/homeassistant/components/mqtt/climate.py b/homeassistant/components/mqtt/climate.py index 4b57290bc0a..b09ee17af68 100644 --- a/homeassistant/components/mqtt/climate.py +++ b/homeassistant/components/mqtt/climate.py @@ -709,13 +709,16 @@ class MqttClimate(MqttTemperatureControlEntity, ClimateEntity): def _handle_action_received(self, msg: ReceiveMessage) -> None: """Handle receiving action via MQTT.""" payload = self.render_template(msg, CONF_ACTION_TEMPLATE) - if not payload or payload == PAYLOAD_NONE: + if not payload: _LOGGER.debug( "Invalid %s action: %s, ignoring", [e.value for e in HVACAction], payload, ) return + if payload == PAYLOAD_NONE: + self._attr_hvac_action = None + return try: self._attr_hvac_action = HVACAction(str(payload)) except ValueError: @@ -733,8 +736,10 @@ class MqttClimate(MqttTemperatureControlEntity, ClimateEntity): """Handle receiving listed mode via MQTT.""" payload = self.render_template(msg, template_name) - if payload not in self._config[mode_list]: - _LOGGER.error("Invalid %s mode: %s", mode_list, payload) + if payload == PAYLOAD_NONE: + setattr(self, attr, None) + elif payload not in self._config[mode_list]: + _LOGGER.warning("Invalid %s mode: %s", mode_list, payload) else: setattr(self, attr, payload) diff --git a/homeassistant/components/mqtt/cover.py b/homeassistant/components/mqtt/cover.py index 692d9eb9b26..d741f602670 100644 --- a/homeassistant/components/mqtt/cover.py +++ b/homeassistant/components/mqtt/cover.py @@ -62,6 +62,7 @@ from .const import ( DEFAULT_POSITION_CLOSED, DEFAULT_POSITION_OPEN, DEFAULT_RETAIN, + PAYLOAD_NONE, ) from .mixins import MqttEntity, async_setup_entity_entry_helper from .models import MqttCommandTemplate, MqttValueTemplate, ReceiveMessage @@ -350,9 +351,13 @@ class MqttCover(MqttEntity, CoverEntity): self._attr_supported_features = supported_features @callback - def _update_state(self, state: str) -> None: + def _update_state(self, state: str | None) -> None: """Update the cover state.""" - self._attr_is_closed = state == STATE_CLOSED + if state is None: + # Reset the state to `unknown` + self._attr_is_closed = None + else: + self._attr_is_closed = state == STATE_CLOSED self._attr_is_opening = state == STATE_OPENING self._attr_is_closing = state == STATE_CLOSING @@ -376,7 +381,7 @@ class MqttCover(MqttEntity, CoverEntity): _LOGGER.debug("Ignoring empty state message from '%s'", msg.topic) return - state: str + state: str | None if payload == self._config[CONF_STATE_STOPPED]: if self._config.get(CONF_GET_POSITION_TOPIC) is not None: state = ( @@ -398,6 +403,8 @@ class MqttCover(MqttEntity, CoverEntity): state = STATE_OPEN elif payload == self._config[CONF_STATE_CLOSED]: state = STATE_CLOSED + elif payload == PAYLOAD_NONE: + state = None else: _LOGGER.warning( ( diff --git a/homeassistant/components/mqtt/device_tracker.py b/homeassistant/components/mqtt/device_tracker.py index b0887ff8932..519af19ac16 100644 --- a/homeassistant/components/mqtt/device_tracker.py +++ b/homeassistant/components/mqtt/device_tracker.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections.abc import Callable +import logging from typing import TYPE_CHECKING import voluptuous as vol @@ -42,6 +43,8 @@ from .models import MqttValueTemplate, ReceiveMessage, ReceivePayloadType from .schemas import MQTT_ENTITY_COMMON_SCHEMA from .util import valid_subscribe_topic +_LOGGER = logging.getLogger(__name__) + CONF_PAYLOAD_HOME = "payload_home" CONF_PAYLOAD_NOT_HOME = "payload_not_home" CONF_SOURCE_TYPE = "source_type" @@ -125,6 +128,13 @@ class MqttDeviceTracker(MqttEntity, TrackerEntity): def message_received(msg: ReceiveMessage) -> None: """Handle new MQTT messages.""" payload = self._value_template(msg.payload) + if not payload.strip(): # No output from template, ignore + _LOGGER.debug( + "Ignoring empty payload '%s' after rendering for topic %s", + payload, + msg.topic, + ) + return if payload == self._config[CONF_PAYLOAD_HOME]: self._location_name = STATE_HOME elif payload == self._config[CONF_PAYLOAD_NOT_HOME]: diff --git a/homeassistant/components/mqtt/lock.py b/homeassistant/components/mqtt/lock.py index 940e1fd24a3..3dfd2b2e6d2 100644 --- a/homeassistant/components/mqtt/lock.py +++ b/homeassistant/components/mqtt/lock.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections.abc import Callable +import logging import re from typing import Any @@ -50,6 +51,8 @@ from .models import ( ) from .schemas import MQTT_ENTITY_COMMON_SCHEMA +_LOGGER = logging.getLogger(__name__) + CONF_CODE_FORMAT = "code_format" CONF_PAYLOAD_LOCK = "payload_lock" @@ -205,9 +208,15 @@ class MqttLock(MqttEntity, LockEntity): ) def message_received(msg: ReceiveMessage) -> None: """Handle new lock state messages.""" - if (payload := self._value_template(msg.payload)) == self._config[ - CONF_PAYLOAD_RESET - ]: + payload = self._value_template(msg.payload) + if not payload.strip(): # No output from template, ignore + _LOGGER.debug( + "Ignoring empty payload '%s' after rendering for topic %s", + payload, + msg.topic, + ) + return + if payload == self._config[CONF_PAYLOAD_RESET]: # Reset the state to `unknown` self._attr_is_locked = None elif payload in self._valid_states: diff --git a/homeassistant/components/mqtt/select.py b/homeassistant/components/mqtt/select.py index 6619e7f6464..05df697764d 100644 --- a/homeassistant/components/mqtt/select.py +++ b/homeassistant/components/mqtt/select.py @@ -122,6 +122,13 @@ class MqttSelect(MqttEntity, SelectEntity, RestoreEntity): def message_received(msg: ReceiveMessage) -> None: """Handle new MQTT messages.""" payload = str(self._value_template(msg.payload)) + if not payload.strip(): # No output from template, ignore + _LOGGER.debug( + "Ignoring empty payload '%s' after rendering for topic %s", + payload, + msg.topic, + ) + return if payload.lower() == "none": self._attr_current_option = None return diff --git a/homeassistant/components/mqtt/valve.py b/homeassistant/components/mqtt/valve.py index a491b1edfda..89a60eef852 100644 --- a/homeassistant/components/mqtt/valve.py +++ b/homeassistant/components/mqtt/valve.py @@ -59,6 +59,7 @@ from .const import ( DEFAULT_POSITION_CLOSED, DEFAULT_POSITION_OPEN, DEFAULT_RETAIN, + PAYLOAD_NONE, ) from .debug_info import log_messages from .mixins import ( @@ -220,13 +221,16 @@ class MqttValve(MqttEntity, ValveEntity): self._attr_supported_features = supported_features @callback - def _update_state(self, state: str) -> None: + def _update_state(self, state: str | None) -> None: """Update the valve state properties.""" self._attr_is_opening = state == STATE_OPENING self._attr_is_closing = state == STATE_CLOSING if self.reports_position: return - self._attr_is_closed = state == STATE_CLOSED + if state is None: + self._attr_is_closed = None + else: + self._attr_is_closed = state == STATE_CLOSED @callback def _process_binary_valve_update( @@ -242,7 +246,9 @@ class MqttValve(MqttEntity, ValveEntity): state = STATE_OPEN elif state_payload == self._config[CONF_STATE_CLOSED]: state = STATE_CLOSED - if state is None: + elif state_payload == PAYLOAD_NONE: + state = None + else: _LOGGER.warning( "Payload received on topic '%s' is not one of " "[open, closed, opening, closing], got: %s", @@ -263,6 +269,9 @@ class MqttValve(MqttEntity, ValveEntity): state = STATE_OPENING elif state_payload == self._config[CONF_STATE_CLOSING]: state = STATE_CLOSING + elif state_payload == PAYLOAD_NONE: + self._attr_current_valve_position = None + return if state is None or position_payload != state_payload: try: percentage_payload = ranged_value_to_percentage( diff --git a/homeassistant/components/mqtt/water_heater.py b/homeassistant/components/mqtt/water_heater.py index af16c93e78c..07d94429854 100644 --- a/homeassistant/components/mqtt/water_heater.py +++ b/homeassistant/components/mqtt/water_heater.py @@ -63,6 +63,7 @@ from .const import ( CONF_TEMP_STATE_TEMPLATE, CONF_TEMP_STATE_TOPIC, DEFAULT_OPTIMISTIC, + PAYLOAD_NONE, ) from .mixins import async_setup_entity_entry_helper from .models import MqttCommandTemplate, MqttValueTemplate, ReceiveMessage @@ -259,10 +260,22 @@ class MqttWaterHeater(MqttTemperatureControlEntity, WaterHeaterEntity): @callback def _handle_current_mode_received(self, msg: ReceiveMessage) -> None: """Handle receiving operation mode via MQTT.""" + payload = self.render_template(msg, CONF_MODE_STATE_TEMPLATE) - if payload not in self._config[CONF_MODE_LIST]: - _LOGGER.error("Invalid %s mode: %s", CONF_MODE_LIST, payload) + if not payload.strip(): # No output from template, ignore + _LOGGER.debug( + "Ignoring empty payload '%s' for current operation " + "after rendering for topic %s", + payload, + msg.topic, + ) + return + + if payload == PAYLOAD_NONE: + self._attr_current_operation = None + elif payload not in self._config[CONF_MODE_LIST]: + _LOGGER.warning("Invalid %s mode: %s", CONF_MODE_LIST, payload) else: if TYPE_CHECKING: assert isinstance(payload, str) diff --git a/tests/components/mqtt/test_alarm_control_panel.py b/tests/components/mqtt/test_alarm_control_panel.py index ff78d96d37e..35fb6841aa3 100644 --- a/tests/components/mqtt/test_alarm_control_panel.py +++ b/tests/components/mqtt/test_alarm_control_panel.py @@ -209,6 +209,14 @@ async def test_update_state_via_state_topic( async_fire_mqtt_message(hass, "alarm/state", state) assert hass.states.get(entity_id).state == state + # Ignore empty payload (last state is STATE_ALARM_TRIGGERED) + async_fire_mqtt_message(hass, "alarm/state", "") + assert hass.states.get(entity_id).state == STATE_ALARM_TRIGGERED + + # Reset state on `None` payload + async_fire_mqtt_message(hass, "alarm/state", "None") + assert hass.states.get(entity_id).state == STATE_UNKNOWN + @pytest.mark.parametrize("hass_config", [DEFAULT_CONFIG]) async def test_ignore_update_state_if_unknown_via_state_topic( diff --git a/tests/components/mqtt/test_climate.py b/tests/components/mqtt/test_climate.py index 821a3f911b7..ba5c15bd4ff 100644 --- a/tests/components/mqtt/test_climate.py +++ b/tests/components/mqtt/test_climate.py @@ -32,7 +32,7 @@ from homeassistant.components.mqtt.climate import ( MQTT_CLIMATE_ATTRIBUTES_BLOCKED, VALUE_TEMPLATE_KEYS, ) -from homeassistant.const import ATTR_TEMPERATURE, UnitOfTemperature +from homeassistant.const import ATTR_TEMPERATURE, STATE_UNKNOWN, UnitOfTemperature from homeassistant.core import HomeAssistant from homeassistant.exceptions import ServiceValidationError @@ -245,11 +245,11 @@ async def test_set_operation_pessimistic( await mqtt_mock_entry() state = hass.states.get(ENTITY_CLIMATE) - assert state.state == "unknown" + assert state.state == STATE_UNKNOWN await common.async_set_hvac_mode(hass, "cool", ENTITY_CLIMATE) state = hass.states.get(ENTITY_CLIMATE) - assert state.state == "unknown" + assert state.state == STATE_UNKNOWN async_fire_mqtt_message(hass, "mode-state", "cool") state = hass.states.get(ENTITY_CLIMATE) @@ -259,6 +259,16 @@ async def test_set_operation_pessimistic( state = hass.states.get(ENTITY_CLIMATE) assert state.state == "cool" + # Ignored + async_fire_mqtt_message(hass, "mode-state", "") + state = hass.states.get(ENTITY_CLIMATE) + assert state.state == "cool" + + # Reset with `None` + async_fire_mqtt_message(hass, "mode-state", "None") + state = hass.states.get(ENTITY_CLIMATE) + assert state.state == STATE_UNKNOWN + @pytest.mark.parametrize( "hass_config", @@ -1011,11 +1021,7 @@ async def test_handle_action_received( """Test getting the action received via MQTT.""" await mqtt_mock_entry() - # Cycle through valid modes and also check for wrong input such as "None" (str(None)) - async_fire_mqtt_message(hass, "action", "None") - state = hass.states.get(ENTITY_CLIMATE) - hvac_action = state.attributes.get(ATTR_HVAC_ACTION) - assert hvac_action is None + # Cycle through valid modes # Redefine actions according to https://developers.home-assistant.io/docs/core/entity/climate/#hvac-action actions = ["off", "preheating", "heating", "cooling", "drying", "idle", "fan"] assert all(elem in actions for elem in HVACAction) @@ -1025,6 +1031,18 @@ async def test_handle_action_received( hvac_action = state.attributes.get(ATTR_HVAC_ACTION) assert hvac_action == action + # Check empty payload is ignored (last action == "fan") + async_fire_mqtt_message(hass, "action", "") + state = hass.states.get(ENTITY_CLIMATE) + hvac_action = state.attributes.get(ATTR_HVAC_ACTION) + assert hvac_action == "fan" + + # Check "None" payload is resetting the action + async_fire_mqtt_message(hass, "action", "None") + state = hass.states.get(ENTITY_CLIMATE) + hvac_action = state.attributes.get(ATTR_HVAC_ACTION) + assert hvac_action is None + @pytest.mark.parametrize("hass_config", [DEFAULT_CONFIG]) async def test_set_preset_mode_optimistic( @@ -1170,6 +1188,10 @@ async def test_set_preset_mode_pessimistic( state = hass.states.get(ENTITY_CLIMATE) assert state.attributes.get("preset_mode") == "comfort" + async_fire_mqtt_message(hass, "preset-mode-state", "") + state = hass.states.get(ENTITY_CLIMATE) + assert state.attributes.get("preset_mode") == "comfort" + async_fire_mqtt_message(hass, "preset-mode-state", "None") state = hass.states.get(ENTITY_CLIMATE) assert state.attributes.get("preset_mode") == "none" @@ -1449,11 +1471,16 @@ async def test_get_with_templates( state = hass.states.get(ENTITY_CLIMATE) assert state.attributes.get("hvac_action") == "cooling" - # Test ignoring null values - async_fire_mqtt_message(hass, "action", "null") + # Test ignoring empty values + async_fire_mqtt_message(hass, "action", "") state = hass.states.get(ENTITY_CLIMATE) assert state.attributes.get("hvac_action") == "cooling" + # Test resetting with null values + async_fire_mqtt_message(hass, "action", "null") + state = hass.states.get(ENTITY_CLIMATE) + assert state.attributes.get("hvac_action") is None + @pytest.mark.parametrize( "hass_config", diff --git a/tests/components/mqtt/test_cover.py b/tests/components/mqtt/test_cover.py index b2b1d1bd9c6..4b46f49c629 100644 --- a/tests/components/mqtt/test_cover.py +++ b/tests/components/mqtt/test_cover.py @@ -123,6 +123,11 @@ async def test_state_via_state_topic( state = hass.states.get("cover.test") assert state.state == STATE_OPEN + async_fire_mqtt_message(hass, "state-topic", "None") + + state = hass.states.get("cover.test") + assert state.state == STATE_UNKNOWN + @pytest.mark.parametrize( "hass_config", diff --git a/tests/components/mqtt/test_device_tracker.py b/tests/components/mqtt/test_device_tracker.py index 4a159b8f9b5..80fbd754d2c 100644 --- a/tests/components/mqtt/test_device_tracker.py +++ b/tests/components/mqtt/test_device_tracker.py @@ -325,6 +325,11 @@ async def test_setting_device_tracker_value_via_mqtt_message( state = hass.states.get("device_tracker.test") assert state.state == STATE_NOT_HOME + # Test an empty value is ignored and the state is retained + async_fire_mqtt_message(hass, "test-topic", "") + state = hass.states.get("device_tracker.test") + assert state.state == STATE_NOT_HOME + async def test_setting_device_tracker_value_via_mqtt_message_and_template( hass: HomeAssistant, diff --git a/tests/components/mqtt/test_lock.py b/tests/components/mqtt/test_lock.py index 4d76b44bb66..c9c2928f991 100644 --- a/tests/components/mqtt/test_lock.py +++ b/tests/components/mqtt/test_lock.py @@ -148,6 +148,12 @@ async def test_controlling_non_default_state_via_topic( state = hass.states.get("lock.test") assert state.state is lock_state + # Empty state is ignored + async_fire_mqtt_message(hass, "state-topic", "") + + state = hass.states.get("lock.test") + assert state.state is lock_state + @pytest.mark.parametrize( ("hass_config", "payload", "lock_state"), diff --git a/tests/components/mqtt/test_select.py b/tests/components/mqtt/test_select.py index e5e1352abb7..b8c55dd2ffb 100644 --- a/tests/components/mqtt/test_select.py +++ b/tests/components/mqtt/test_select.py @@ -3,6 +3,7 @@ from collections.abc import Generator import copy import json +import logging from typing import Any from unittest.mock import patch @@ -91,11 +92,15 @@ def _test_run_select_setup_params( async def test_run_select_setup( hass: HomeAssistant, mqtt_mock_entry: MqttMockHAClientGenerator, + caplog: pytest.LogCaptureFixture, topic: str, ) -> None: """Test that it fetches the given payload.""" await mqtt_mock_entry() + state = hass.states.get("select.test_select") + assert state.state == STATE_UNKNOWN + async_fire_mqtt_message(hass, topic, "milk") await hass.async_block_till_done() @@ -110,6 +115,15 @@ async def test_run_select_setup( state = hass.states.get("select.test_select") assert state.state == "beer" + if caplog.at_level(logging.DEBUG): + async_fire_mqtt_message(hass, topic, "") + await hass.async_block_till_done() + + assert "Ignoring empty payload" in caplog.text + + state = hass.states.get("select.test_select") + assert state.state == "beer" + @pytest.mark.parametrize( "hass_config", diff --git a/tests/components/mqtt/test_valve.py b/tests/components/mqtt/test_valve.py index 7a69af36ff8..2efa30d096a 100644 --- a/tests/components/mqtt/test_valve.py +++ b/tests/components/mqtt/test_valve.py @@ -131,6 +131,11 @@ async def test_state_via_state_topic_no_position( state = hass.states.get("valve.test") assert state.state == asserted_state + async_fire_mqtt_message(hass, "state-topic", "None") + + state = hass.states.get("valve.test") + assert state.state == STATE_UNKNOWN + @pytest.mark.parametrize( "hass_config", @@ -197,6 +202,7 @@ async def test_state_via_state_topic_with_template( ('{"position":100}', STATE_OPEN), ('{"position":50.0}', STATE_OPEN), ('{"position":0}', STATE_CLOSED), + ('{"position":null}', STATE_UNKNOWN), ('{"position":"non_numeric"}', STATE_UNKNOWN), ('{"ignored":12}', STATE_UNKNOWN), ], diff --git a/tests/components/mqtt/test_water_heater.py b/tests/components/mqtt/test_water_heater.py index ee0aa1c0949..8cba3fb9f67 100644 --- a/tests/components/mqtt/test_water_heater.py +++ b/tests/components/mqtt/test_water_heater.py @@ -25,7 +25,12 @@ from homeassistant.components.water_heater import ( STATE_PERFORMANCE, WaterHeaterEntityFeature, ) -from homeassistant.const import ATTR_TEMPERATURE, STATE_OFF, UnitOfTemperature +from homeassistant.const import ( + ATTR_TEMPERATURE, + STATE_OFF, + STATE_UNKNOWN, + UnitOfTemperature, +) from homeassistant.core import HomeAssistant from homeassistant.util.unit_conversion import TemperatureConverter @@ -200,7 +205,7 @@ async def test_set_operation_pessimistic( await mqtt_mock_entry() state = hass.states.get(ENTITY_WATER_HEATER) - assert state.state == "unknown" + assert state.state == STATE_UNKNOWN await common.async_set_operation_mode(hass, "eco", ENTITY_WATER_HEATER) state = hass.states.get(ENTITY_WATER_HEATER) @@ -214,6 +219,16 @@ async def test_set_operation_pessimistic( state = hass.states.get(ENTITY_WATER_HEATER) assert state.state == "eco" + # Empty state ignored + async_fire_mqtt_message(hass, "mode-state", "") + state = hass.states.get(ENTITY_WATER_HEATER) + assert state.state == "eco" + + # Test None payload + async_fire_mqtt_message(hass, "mode-state", "None") + state = hass.states.get(ENTITY_WATER_HEATER) + assert state.state == STATE_UNKNOWN + @pytest.mark.parametrize( "hass_config",