Warn user if Tasmota devices are configured with invalid MQTT topics (#77640)

homeassistant/components/tasmota/discovery.py

@@ -3,7 +3,9 @@ from __future__ import annotations
 
 from collections.abc import Awaitable, Callable
 import logging
+from typing import TypedDict, cast
 
+from hatasmota import const as tasmota_const
 from hatasmota.discovery import (
     TasmotaDiscovery,
     get_device_config as tasmota_get_device_config,
@@ -17,11 +19,16 @@ from hatasmota.entity import TasmotaEntityConfig
 from hatasmota.models import DiscoveryHashType, TasmotaDeviceConfig
 from hatasmota.mqtt import TasmotaMQTTClient
 from hatasmota.sensor import TasmotaBaseSensorConfig
+from hatasmota.utils import get_topic_command, get_topic_stat
 
 from homeassistant.components import sensor
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant
-from homeassistant.helpers import device_registry as dr, entity_registry as er
+from homeassistant.helpers import (
+    device_registry as dr,
+    entity_registry as er,
+    issue_registry as ir,
+)
 from homeassistant.helpers.dispatcher import async_dispatcher_send
 from homeassistant.helpers.entity_registry import async_entries_for_device
 
@@ -30,10 +37,13 @@ from .const import DOMAIN, PLATFORMS
 _LOGGER = logging.getLogger(__name__)
 
 ALREADY_DISCOVERED = "tasmota_discovered_components"
+DISCOVERY_DATA = "tasmota_discovery_data"
 TASMOTA_DISCOVERY_ENTITY_NEW = "tasmota_discovery_entity_new_{}"
 TASMOTA_DISCOVERY_ENTITY_UPDATED = "tasmota_discovery_entity_updated_{}_{}_{}_{}"
 TASMOTA_DISCOVERY_INSTANCE = "tasmota_discovery_instance"
 
+MQTT_TOPIC_URL = "https://tasmota.github.io/docs/Home-Assistant/#tasmota-integration"
+
 SetupDeviceCallback = Callable[[TasmotaDeviceConfig, str], Awaitable[None]]
 
 
@@ -52,7 +62,64 @@ def set_discovery_hash(hass: HomeAssistant, discovery_hash: DiscoveryHashType) -
     hass.data[ALREADY_DISCOVERED][discovery_hash] = {}
 
 
-async def async_start(
+def warn_if_topic_duplicated(
+    hass: HomeAssistant,
+    command_topic: str,
+    own_mac: str | None,
+    own_device_config: TasmotaDeviceConfig,
+) -> bool:
+    """Log and create repairs issue if several devices share the same topic."""
+    duplicated = False
+    offenders = []
+    for other_mac, other_config in hass.data[DISCOVERY_DATA].items():
+        if own_mac and other_mac == own_mac:
+            continue
+        if command_topic == get_topic_command(other_config):
+            offenders.append((other_mac, tasmota_get_device_config(other_config)))
+    issue_id = f"topic_duplicated_{command_topic}"
+    if offenders:
+        if own_mac:
+            offenders.append((own_mac, own_device_config))
+        offender_strings = [
+            f"'{cfg[tasmota_const.CONF_NAME]}' ({cfg[tasmota_const.CONF_IP]})"
+            for _, cfg in offenders
+        ]
+        _LOGGER.warning(
+            "Multiple Tasmota devices are sharing the same topic '%s'. Offending devices: %s",
+            command_topic,
+            ", ".join(offender_strings),
+        )
+        ir.async_create_issue(
+            hass,
+            DOMAIN,
+            issue_id,
+            data={
+                "key": "topic_duplicated",
+                "mac": " ".join([mac for mac, _ in offenders]),
+                "topic": command_topic,
+            },
+            is_fixable=False,
+            learn_more_url=MQTT_TOPIC_URL,
+            severity=ir.IssueSeverity.ERROR,
+            translation_key="topic_duplicated",
+            translation_placeholders={
+                "topic": command_topic,
+                "offenders": "\n\n* " + "\n\n* ".join(offender_strings),
+            },
+        )
+        duplicated = True
+    return duplicated
+
+
+class DuplicatedTopicIssueData(TypedDict):
+    """Typed result dict."""
+
+    key: str
+    mac: str
+    topic: str
+
+
+async def async_start(  # noqa: C901
     hass: HomeAssistant,
     discovery_topic: str,
     config_entry: ConfigEntry,
@@ -121,9 +188,72 @@ async def async_start(
         tasmota_device_config = tasmota_get_device_config(payload)
         await setup_device(tasmota_device_config, mac)
 
+        hass.data[DISCOVERY_DATA][mac] = payload
+
+        add_entities = True
+
+        command_topic = get_topic_command(payload) if payload else None
+        state_topic = get_topic_stat(payload) if payload else None
+
+        # Create or clear issue if topic is missing prefix
+        issue_id = f"topic_no_prefix_{mac}"
+        if payload and command_topic == state_topic:
+            _LOGGER.warning(
+                "Tasmota device '%s' with IP %s doesn't set %%prefix%% in its topic",
+                tasmota_device_config[tasmota_const.CONF_NAME],
+                tasmota_device_config[tasmota_const.CONF_IP],
+            )
+            ir.async_create_issue(
+                hass,
+                DOMAIN,
+                issue_id,
+                data={"key": "topic_no_prefix"},
+                is_fixable=False,
+                learn_more_url=MQTT_TOPIC_URL,
+                severity=ir.IssueSeverity.ERROR,
+                translation_key="topic_no_prefix",
+                translation_placeholders={
+                    "name": tasmota_device_config[tasmota_const.CONF_NAME],
+                    "ip": tasmota_device_config[tasmota_const.CONF_IP],
+                },
+            )
+            add_entities = False
+        else:
+            ir.async_delete_issue(hass, DOMAIN, issue_id)
+
+        # Clear previous issues caused by duplicated topic
+        issue_reg = ir.async_get(hass)
+        tasmota_issues = [
+            issue for key, issue in issue_reg.issues.items() if key[0] == DOMAIN
+        ]
+        for issue in tasmota_issues:
+            if issue.data and issue.data["key"] == "topic_duplicated":
+                issue_data: DuplicatedTopicIssueData = cast(
+                    DuplicatedTopicIssueData, issue.data
+                )
+                macs = issue_data["mac"].split()
+                if mac not in macs:
+                    continue
+                if payload and command_topic == issue_data["topic"]:
+                    continue
+                if len(macs) > 2:
+                    # This device is no longer duplicated, update the issue
+                    warn_if_topic_duplicated(hass, issue_data["topic"], None, {})
+                    continue
+                ir.async_delete_issue(hass, DOMAIN, issue.issue_id)
+
         if not payload:
             return
 
+        # Warn and add issues if there are duplicated topics
+        if warn_if_topic_duplicated(hass, command_topic, mac, tasmota_device_config):
+            add_entities = False
+
+        if not add_entities:
+            # Add the device entry so the user can identify the device, but do not add
+            # entities or triggers
+            return
+
         tasmota_triggers = tasmota_get_triggers(payload)
         for trigger_config in tasmota_triggers:
             discovery_hash: DiscoveryHashType = (
@@ -194,6 +324,7 @@ async def async_start(
                 entity_registry.async_remove(entity_id)
 
     hass.data[ALREADY_DISCOVERED] = {}
+    hass.data[DISCOVERY_DATA] = {}
 
     tasmota_discovery = TasmotaDiscovery(discovery_topic, tasmota_mqtt)
     await tasmota_discovery.start_discovery(

homeassistant/components/tasmota/manifest.json

@@ -3,7 +3,7 @@
   "name": "Tasmota",
   "config_flow": true,
   "documentation": "https://www.home-assistant.io/integrations/tasmota",
-  "requirements": ["hatasmota==0.6.0"],
+  "requirements": ["hatasmota==0.6.1"],
   "dependencies": ["mqtt"],
   "mqtt": ["tasmota/discovery/#"],
   "codeowners": ["@emontnemery"],

homeassistant/components/tasmota/strings.json

@@ -16,5 +16,15 @@
     "error": {
       "invalid_discovery_topic": "Invalid discovery topic prefix."
     }
+  },
+  "issues": {
+    "topic_duplicated": {
+      "title": "Several Tasmota devices are sharing the same topic",
+      "description": "Several Tasmota devices are sharing the topic {topic}.\n\n Tasmota devices with this problem: {offenders}."
+    },
+    "topic_no_prefix": {
+      "title": "Tasmota device {name} has an invalid MQTT topic",
+      "description": "Tasmota device {name} with IP {ip} does not include `%prefix%` in its fulltopic.\n\nEntities for this devices are disabled until the configuration has been corrected."
+    }
   }
 }

homeassistant/components/tasmota/translations/en.json

@@ -16,5 +16,15 @@
                 "description": "Do you want to set up Tasmota?"
             }
         }
+    },
+    "issues": {
+        "topic_duplicated": {
+            "description": "Several Tasmota devices are sharing the topic {topic}.\n\n Tasmota devices with this problem: {offenders}.",
+            "title": "Several Tasmota devices are sharing the same topic"
+        },
+        "topic_no_prefix": {
+            "description": "Tasmota device {name} with IP {ip} does not include `%prefix%` in its fulltopic.\n\nEntities for this devices are disabled until the configuration has been corrected.",
+            "title": "Tasmota device {name} has an invalid MQTT topic"
+        }
     }
 }

requirements_all.txt

@@ -830,7 +830,7 @@ hass-nabucasa==0.55.0
 hass_splunk==0.1.1
 
 # homeassistant.components.tasmota
-hatasmota==0.6.0
+hatasmota==0.6.1
 
 # homeassistant.components.jewish_calendar
 hdate==0.10.4

requirements_test_all.txt

@@ -613,7 +613,7 @@ hangups==0.4.18
 hass-nabucasa==0.55.0
 
 # homeassistant.components.tasmota
-hatasmota==0.6.0
+hatasmota==0.6.1
 
 # homeassistant.components.jewish_calendar
 hdate==0.10.4

tests/components/tasmota/test_discovery.py

@@ -5,7 +5,11 @@ from unittest.mock import ANY, patch
 
 from homeassistant.components.tasmota.const import DEFAULT_PREFIX
 from homeassistant.components.tasmota.discovery import ALREADY_DISCOVERED
-from homeassistant.helpers import device_registry as dr
+from homeassistant.helpers import (
+    device_registry as dr,
+    entity_registry as er,
+    issue_registry as ir,
+)
 from homeassistant.setup import async_setup_component
 
 from .conftest import setup_tasmota_helper
@@ -495,3 +499,179 @@ async def test_entity_duplicate_removal(hass, mqtt_mock, caplog, setup_tasmota):
     async_fire_mqtt_message(hass, f"{DEFAULT_PREFIX}/{mac}/config", json.dumps(config))
     await hass.async_block_till_done()
     assert "Removing entity: switch" not in caplog.text
+
+
+async def test_same_topic(
+    hass, mqtt_mock, caplog, device_reg, entity_reg, setup_tasmota
+):
+    """Test detecting devices with same topic."""
+    configs = [
+        copy.deepcopy(DEFAULT_CONFIG),
+        copy.deepcopy(DEFAULT_CONFIG),
+        copy.deepcopy(DEFAULT_CONFIG),
+    ]
+    configs[0]["rl"][0] = 1
+    configs[1]["rl"][0] = 1
+    configs[2]["rl"][0] = 1
+    configs[0]["mac"] = "000000000001"
+    configs[1]["mac"] = "000000000002"
+    configs[2]["mac"] = "000000000003"
+
+    for config in configs[0:2]:
+        async_fire_mqtt_message(
+            hass,
+            f"{DEFAULT_PREFIX}/{config['mac']}/config",
+            json.dumps(config),
+        )
+    await hass.async_block_till_done()
+
+    # Verify device registry entries are created for both devices
+    for config in configs[0:2]:
+        device_entry = device_reg.async_get_device(
+            set(), {(dr.CONNECTION_NETWORK_MAC, config["mac"])}
+        )
+        assert device_entry is not None
+        assert device_entry.configuration_url == f"http://{config['ip']}/"
+        assert device_entry.manufacturer == "Tasmota"
+        assert device_entry.model == config["md"]
+        assert device_entry.name == config["dn"]
+        assert device_entry.sw_version == config["sw"]
+
+    # Verify entities are created only for the first device
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[0]["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 1
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[1]["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 0
+
+    # Verify a repairs issue was created
+    issue_id = "topic_duplicated_tasmota_49A3BC/cmnd/"
+    issue_registry = ir.async_get(hass)
+    issue = issue_registry.async_get_issue("tasmota", issue_id)
+    assert issue.data["mac"] == " ".join(config["mac"] for config in configs[0:2])
+
+    # Discover a 3rd device with same topic
+    async_fire_mqtt_message(
+        hass,
+        f"{DEFAULT_PREFIX}/{configs[2]['mac']}/config",
+        json.dumps(configs[2]),
+    )
+    await hass.async_block_till_done()
+
+    # Verify device registry entries was created
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[2]["mac"])}
+    )
+    assert device_entry is not None
+    assert device_entry.configuration_url == f"http://{configs[2]['ip']}/"
+    assert device_entry.manufacturer == "Tasmota"
+    assert device_entry.model == configs[2]["md"]
+    assert device_entry.name == configs[2]["dn"]
+    assert device_entry.sw_version == configs[2]["sw"]
+
+    # Verify no entities were created
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[2]["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 0
+
+    # Verify the repairs issue has been updated
+    issue = issue_registry.async_get_issue("tasmota", issue_id)
+    assert issue.data["mac"] == " ".join(config["mac"] for config in configs[0:3])
+
+    # Rediscover 3rd device with fixed config
+    configs[2]["t"] = "unique_topic_2"
+    async_fire_mqtt_message(
+        hass,
+        f"{DEFAULT_PREFIX}/{configs[2]['mac']}/config",
+        json.dumps(configs[2]),
+    )
+    await hass.async_block_till_done()
+
+    # Verify entities are created also for the third device
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[2]["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 1
+
+    # Verify the repairs issue has been updated
+    issue = issue_registry.async_get_issue("tasmota", issue_id)
+    assert issue.data["mac"] == " ".join(config["mac"] for config in configs[0:2])
+
+    # Rediscover 2nd device with fixed config
+    configs[1]["t"] = "unique_topic_1"
+    async_fire_mqtt_message(
+        hass,
+        f"{DEFAULT_PREFIX}/{configs[1]['mac']}/config",
+        json.dumps(configs[1]),
+    )
+    await hass.async_block_till_done()
+
+    # Verify entities are created also for the second device
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, configs[1]["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 1
+
+    # Verify the repairs issue has been removed
+    assert issue_registry.async_get_issue("tasmota", issue_id) is None
+
+
+async def test_topic_no_prefix(
+    hass, mqtt_mock, caplog, device_reg, entity_reg, setup_tasmota
+):
+    """Test detecting devices with same topic."""
+    config = copy.deepcopy(DEFAULT_CONFIG)
+    config["rl"][0] = 1
+    config["ft"] = "%topic%/blah/"
+
+    async_fire_mqtt_message(
+        hass,
+        f"{DEFAULT_PREFIX}/{config['mac']}/config",
+        json.dumps(config),
+    )
+    await hass.async_block_till_done()
+
+    # Verify device registry entry is created
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, config["mac"])}
+    )
+    assert device_entry is not None
+    assert device_entry.configuration_url == f"http://{config['ip']}/"
+    assert device_entry.manufacturer == "Tasmota"
+    assert device_entry.model == config["md"]
+    assert device_entry.name == config["dn"]
+    assert device_entry.sw_version == config["sw"]
+
+    # Verify entities are not created
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, config["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 0
+
+    # Verify a repairs issue was created
+    issue_id = "topic_no_prefix_00000049A3BC"
+    issue_registry = ir.async_get(hass)
+    assert ("tasmota", issue_id) in issue_registry.issues
+
+    # Rediscover device with fixed config
+    config["ft"] = "%topic%/%prefix%/"
+    async_fire_mqtt_message(
+        hass,
+        f"{DEFAULT_PREFIX}/{config['mac']}/config",
+        json.dumps(config),
+    )
+    await hass.async_block_till_done()
+
+    # Verify entities are created
+    device_entry = device_reg.async_get_device(
+        set(), {(dr.CONNECTION_NETWORK_MAC, config["mac"])}
+    )
+    assert len(er.async_entries_for_device(entity_reg, device_entry.id, True)) == 1
+
+    # Verify the repairs issue has been removed
+    issue_registry = ir.async_get(hass)
+    assert ("tasmota", issue_id) not in issue_registry.issues