Add MQTT WS command to remove device #31989

emontnemery · 2020-02-19T15:53:51Z

Proposed change

Add MQTT WS command to remove a device from device registry.

When removing the device:

Any entities belonging to the device are removed from entity registry
Any MQTT device trigger is forgotten
Retained discovery topics are cleared
- As a result of clearing the discovery topics, states are also removed

Possible extensions (in this or separate follow-up PRs):

It's already possible to remove a discovered MQTT entity from the state machine by sending an empty discovery topic. Maybe an empty discovery topic should also trigger removal from the entity registry, and if there are no more entities or MQTT device triggers, remove the device as well?
Remove the state from the state machine when an entity is removed from the entity registry

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

probot-home-assistant · 2020-02-19T15:54:02Z

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (mqtt) you are listed as a codeowner for? Thanks!

emontnemery · 2020-02-19T15:55:09Z

homeassistant/components/mqtt/__init__.py

@@ -1247,6 +1265,18 @@ def device_info(self):
        return device_info_from_config(self._device_config)


+@websocket_api.websocket_command(
+    {vol.Required("type"): "mqtt/device/remove", vol.Required("device_id"): str}


There's nothing specific to MQTT in the implementation, should this maybe be moved elsewhere?

emontnemery · 2020-02-19T15:56:26Z

homeassistant/components/mqtt/__init__.py

+    async def async_removed_from_registry(self) -> None:
+        """Clear retained discovery topic in broker."""
+        async_publish(
+            self.hass, self._discovery_topic, "", retain=True,


This will also cause the state to be removed from the state machine.

emontnemery · 2020-02-19T15:57:42Z

homeassistant/helpers/entity.py

@@ -534,6 +540,9 @@ def async_on_remove(self, func: CALLBACK_TYPE) -> None:
    async def _async_registry_updated(self, event):
        """Handle entity registry update."""
        data = event.data
+        if data["action"] == "remove" and data["entity_id"] == self.entity_id:
+            await self.async_removed_from_registry()
+


We could call self.async_remove() here as well to remove the state from the state machine, but that should probably be done in a separate PR.

So can you document/explain to me how delete changes cascade throughout the MQTT system?

These two things can happen:

A user indicates that an MQTT device needs to be removed

An empty discovery configuration is pushed to a topic that we previously got a configuration from.

These two cases need to be handled and we should document this in the MQTT integration somewhere how this works.

Creation / Discovery of MQTT entities / devices by MQTT:

Handled by https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/mqtt/discovery.py
When receiving a discovery message, a state is created in the Hass state machine, or updated if it already exists.
(In case of an MQTT device trigger, no state is created).
If unique_id is set in the config data, an entity registry entry will be created, or updated if it already exists.
If device dict is set in the config data, a device registry will be created, or updated if it already exists.

Discovered MQTT enitities will listen for discovery updates - which must be sent on the same MQTT topic - with help of the MqttDiscoveryUpdated mixin:
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1159-L1160

Update of MQTT entities / devices by MQTT:

Handled by the MqttDiscoveryUpdated mixin:
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1187-L1190

Removal of MQTT entities / devices by MQTT:

If an empty discovery message is received, this will result in:

Removal of the state from the state machine
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1182-L1184

Entity registry entry will NOT be removed
This should maybe be changed such that entity registry entries are removed?

Device registry entry will NOT be removed
This should maybe be changed such that device registry entries are removed?

Removal of MQTT entities / devices by WS:

A WS helper exists to fully remove an MQTT device, which will result in:

Removal of the states from the state machine (as a result of clearing the discovery topics)

Entity registry entries related to the device will be removed

Device registry entry will be removed, including MQTT device triggers

Any retained MQTT discovery topics for entities are cleared from the broker
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1199-L1204

Any retained MQTT discovery topics for triggers are cleared from the broker
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/device_trigger.py#L233-L241

balloob · 2020-02-19T17:57:49Z

homeassistant/components/mqtt/__init__.py

+        self._discovery_hash = (
+            discovery_data[ATTR_DISCOVERY_HASH] if discovery_data else None
+        )
+        self._discovery_topic = (


Why keep two variables around if you can just store discovery_data ?

You're right, cleaned up.

balloob · 2020-02-19T17:58:46Z

homeassistant/components/mqtt/__init__.py

+    {vol.Required("type"): "mqtt/device/remove", vol.Required("device_id"): str}
+)
+@websocket_api.async_response
+async def websocket_remove_device(hass, connection, msg):


We should also remove the retained discovery config from MQTT.

This is done:

Any retained MQTT discovery topics for entities are cleared from the broker
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1199-L1204

Any retained MQTT discovery topics for triggers are cleared from the broker
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/device_trigger.py#L233-L241

balloob · 2020-02-19T18:00:18Z

homeassistant/components/mqtt/__init__.py

+    """Delete device."""
+    device_id = msg["device_id"]
+    dev_registry = await get_dev_reg(hass)
+    dev_registry.async_remove_device(device_id)


This can raise a KeyError. We should also only delete devices that are part of MQTT.

I think it's fine if it throws, this will result in an error response to the WS call?
Added check to only delete devices that are part of MQTT:
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/homeassistant/components/mqtt/__init__.py#L1277-L1279

balloob · 2020-02-19T18:00:55Z

homeassistant/components/mqtt/alarm_control_panel.py

@@ -98,15 +98,14 @@

    async def async_discover(discovery_payload):
        """Discover and add an MQTT alarm control panel."""
+        discovery_data = discovery_payload.__discovery_data__


Why are there double underscores before/after the name ? Those are reserved for system values.

You can add _discovery_data_. But why not make it _discovery_data_?

Yeah, OK, changed to discovery_data.

homeassistant/components/mqtt/__init__.py

balloob · 2020-02-20T17:18:20Z

homeassistant/components/mqtt/__init__.py

+        if config_entry.domain == DOMAIN:
+            dev_registry.async_remove_device(device_id)
+            connection.send_message(websocket_api.result_message(msg["id"]))
+            break


You're not sending an error if you can't find the device .

Right, but isn't it enough to not send success?
Trying to remove a non existing device is tested in this test case:
https://github.com/home-assistant/home-assistant/blob/e170a11547b716de8cfe46a99473f4dbfcb7f62b/tests/components/mqtt/test_init.py#L881

No. This will cause the test to never finish if you're awaiting a response.

codecov · 2020-02-20T18:36:45Z

Codecov Report

Merging #31989 into dev will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev   #31989   +/-   ##
=======================================
  Coverage   94.69%   94.70%           
=======================================
  Files         766      766           
  Lines       55567    55613   +46     
=======================================
+ Hits        52619    52667   +48     
+ Misses       2948     2946    -2

Impacted Files	Coverage Δ
homeassistant/components/template/cover.py	`96.34% <0.00%> (-1.37%)`	⬇️
homeassistant/bootstrap.py	`75.40% <0.00%> (ø)`	⬆️
homeassistant/components/vizio/config_flow.py	`100.00% <0.00%> (ø)`	⬆️
...omeassistant/components/homematicip_cloud/cover.py	`100.00% <0.00%> (ø)`	⬆️
...meassistant/components/homematicip_cloud/sensor.py	`100.00% <0.00%> (ø)`	⬆️
...sistant/components/input_number/reproduce_state.py	`100.00% <0.00%> (ø)`	⬆️
homeassistant/__init__.py	`89.02% <0.00%> (+0.56%)`	⬆️
homeassistant/components/uk_transport/sensor.py	`94.20% <0.00%> (+0.72%)`	⬆️
setup.py	`88.02% <0.00%> (+1.79%)`	⬆️
...meassistant/components/websocket_api/connection.py	`97.22% <0.00%> (+2.77%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32cd58e...1d6c5e1. Read the comment docs.

Add MQTT WS command to remove device

24973f8

emontnemery requested a review from a team as a code owner February 19, 2020 15:53

homeassistant added the cla-signed label Feb 19, 2020

probot-home-assistant bot added core has-tests integration: mqtt labels Feb 19, 2020

project-bot bot added this to Needs review in Dev Feb 19, 2020

emontnemery commented Feb 19, 2020

View reviewed changes

balloob reviewed Feb 19, 2020

View reviewed changes

Review comments, fix test

e170a11

balloob reviewed Feb 20, 2020

View reviewed changes

homeassistant/components/mqtt/__init__.py Show resolved Hide resolved

balloob reviewed Feb 20, 2020

View reviewed changes

Fix tests

1d6c5e1

balloob approved these changes Feb 25, 2020

View reviewed changes

Dev automation moved this from Needs review to Reviewer approved Feb 25, 2020

balloob merged commit 7e387f9 into home-assistant:dev Feb 25, 2020

Dev automation moved this from Reviewer approved to Done Feb 25, 2020

lock bot locked and limited conversation to collaborators Feb 28, 2020

emontnemery deleted the mqtt_remove_device branch March 19, 2020 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MQTT WS command to remove device #31989

Add MQTT WS command to remove device #31989

emontnemery commented Feb 19, 2020

probot-home-assistant bot commented Feb 19, 2020

emontnemery Feb 19, 2020

emontnemery Feb 19, 2020

emontnemery Feb 19, 2020

balloob Feb 19, 2020

emontnemery Feb 20, 2020

balloob Feb 19, 2020

emontnemery Feb 20, 2020

balloob Feb 19, 2020

emontnemery Feb 20, 2020

balloob Feb 19, 2020

emontnemery Feb 20, 2020

balloob Feb 19, 2020

balloob Feb 19, 2020

emontnemery Feb 20, 2020

balloob Feb 20, 2020

emontnemery Feb 20, 2020 •

edited

balloob Feb 25, 2020

codecov bot commented Feb 20, 2020

Add MQTT WS command to remove device #31989

Add MQTT WS command to remove device #31989

Conversation

emontnemery commented Feb 19, 2020

Proposed change

Type of change

Checklist

probot-home-assistant bot commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Creation / Discovery of MQTT entities / devices by MQTT:

Update of MQTT entities / devices by MQTT:

Removal of MQTT entities / devices by MQTT:

Removal of MQTT entities / devices by WS:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Feb 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2020

Codecov Report

emontnemery Feb 20, 2020 •

edited