Avoid removing zwave_js devices for non-ready nodes (#59964)

* Only replace a node if the mfgr id / prod id / prod type differ

* Prefer original device name for unready node

* move register_node_in_dev_reg into async_setup_entry

* simplify get_device_id_ext

* Don't need hex ids

* Revert "move register_node_in_dev_reg into async_setup_entry"

This reverts commit f900e5f.

* Revert Callable change

* Revert device backup name

* Add test fixtures

* Update existing not ready test with new fixture data

* Check device properties after node added event

* Add entity check

* Check for extended device id

* better device info checks

* Use receive_event to properly setup components

* Cleanup tests

* improve test_replace_different_node

* improve test_replace_same_node

* add test test_node_model_change

* Clean up long comments and strings

* Format

* Reload integration to detect node device config changes

* update assertions

* Disable entities on "value removed" event

* Disable node status sensor on node replacement

* Add test for disabling entities on remove value event

* Add test for disabling node status sensor on node replacement

* disable entity -> remove entity

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

homeassistant/components/zwave_js/__init__.py

@@ -88,7 +88,12 @@
     async_discover_node_values,
     async_discover_single_value,
 )
-from .helpers import async_enable_statistics, get_device_id, get_unique_id
+from .helpers import (
+    async_enable_statistics,
+    get_device_id,
+    get_device_id_ext,
+    get_unique_id,
+)
 from .migrate import async_migrate_discovered_value
 from .services import ZWaveServices
 
@@ -116,17 +121,27 @@ def register_node_in_dev_reg(
 ) -> device_registry.DeviceEntry:
     """Register node in dev reg."""
     device_id = get_device_id(client, node)
-    # If a device already exists but it doesn't match the new node, it means the node
-    # was replaced with a different device and the device needs to be removeed so the
-    # new device can be created. Otherwise if the device exists and the node is the same,
-    # the node was replaced with the same device model and we can reuse the device.
-    if (device := dev_reg.async_get_device({device_id})) and (
-        device.model != node.device_config.label
-        or device.manufacturer != node.device_config.manufacturer
+    device_id_ext = get_device_id_ext(client, node)
+    device = dev_reg.async_get_device({device_id})
+
+    # Replace the device if it can be determined that this node is not the
+    # same product as it was previously.
+    if (
+        device_id_ext
+        and device
+        and len(device.identifiers) == 2
+        and device_id_ext not in device.identifiers
     ):
         remove_device_func(device)
+        device = None
+
+    if device_id_ext:
+        ids = {device_id, device_id_ext}
+    else:
+        ids = {device_id}
+
     params = {
-        ATTR_IDENTIFIERS: {device_id},
+        ATTR_IDENTIFIERS: ids,
         ATTR_SW_VERSION: node.firmware_version,
         ATTR_NAME: node.name
         or node.device_config.description
@@ -338,7 +353,14 @@ def async_on_node_removed(event: dict) -> None:
         device = dev_reg.async_get_device({dev_id})
         # We assert because we know the device exists
         assert device
-        if not replaced:
+        if replaced:
+            discovered_value_ids.pop(device.id, None)
+
+            async_dispatcher_send(
+                hass,
+                f"{DOMAIN}_{client.driver.controller.home_id}.{node.node_id}.node_status_remove_entity",
+            )
+        else:
             remove_device(device)
 
     @callback

homeassistant/components/zwave_js/entity.py

@@ -20,6 +20,7 @@
 LOGGER = logging.getLogger(__name__)
 
 EVENT_VALUE_UPDATED = "value updated"
+EVENT_VALUE_REMOVED = "value removed"
 EVENT_DEAD = "dead"
 EVENT_ALIVE = "alive"
 
@@ -99,6 +100,10 @@ async def async_added_to_hass(self) -> None:
         self.async_on_remove(
             self.info.node.on(EVENT_VALUE_UPDATED, self._value_changed)
         )
+        self.async_on_remove(
+            self.info.node.on(EVENT_VALUE_REMOVED, self._value_removed)
+        )
+
         for status_event in (EVENT_ALIVE, EVENT_DEAD):
             self.async_on_remove(
                 self.info.node.on(status_event, self._node_status_alive_or_dead)
@@ -171,7 +176,7 @@ def _node_status_alive_or_dead(self, event_data: dict) -> None:
 
     @callback
     def _value_changed(self, event_data: dict) -> None:
-        """Call when (one of) our watched values changes.
+        """Call when a value associated with our node changes.
 
         Should not be overridden by subclasses.
         """
@@ -193,6 +198,25 @@ def _value_changed(self, event_data: dict) -> None:
         self.on_value_update()
         self.async_write_ha_state()
 
+    @callback
+    def _value_removed(self, event_data: dict) -> None:
+        """Call when a value associated with our node is removed.
+
+        Should not be overridden by subclasses.
+        """
+        value_id = event_data["value"].value_id
+
+        if value_id != self.info.primary_value.value_id:
+            return
+
+        LOGGER.debug(
+            "[%s] Primary value %s is being removed",
+            self.entity_id,
+            value_id,
+        )
+
+        self.hass.async_create_task(self.async_remove())
+
     @callback
     def get_zwave_value(
         self,

homeassistant/components/zwave_js/helpers.py

@@ -66,6 +66,19 @@ def get_device_id(client: ZwaveClient, node: ZwaveNode) -> tuple[str, str]:
     return (DOMAIN, f"{client.driver.controller.home_id}-{node.node_id}")
 
 
+@callback
+def get_device_id_ext(client: ZwaveClient, node: ZwaveNode) -> tuple[str, str] | None:
+    """Get extended device registry identifier for Z-Wave node."""
+    if None in (node.manufacturer_id, node.product_type, node.product_id):
+        return None
+
+    domain, dev_id = get_device_id(client, node)
+    return (
+        domain,
+        f"{dev_id}-{node.manufacturer_id}:{node.product_type}:{node.product_id}",
+    )
+
+
 @callback
 def get_home_and_node_id_from_device_id(device_id: tuple[str, ...]) -> list[str]:
     """

homeassistant/components/zwave_js/sensor.py

@@ -502,4 +502,11 @@ async def async_added_to_hass(self) -> None:
                 self.async_poll_value,
             )
         )
+        self.async_on_remove(
+            async_dispatcher_connect(
+                self.hass,
+                f"{DOMAIN}_{self.unique_id}_remove_entity",
+                self.async_remove,
+            )
+        )
         self.async_write_ha_state()

tests/components/zwave_js/conftest.py

@@ -479,6 +479,18 @@ def fortrezz_ssa3_siren_state_fixture():
     return json.loads(load_fixture("zwave_js/fortrezz_ssa3_siren_state.json"))
 
 
+@pytest.fixture(name="zp3111_not_ready_state", scope="session")
+def zp3111_not_ready_state_fixture():
+    """Load the zp3111 4-in-1 sensor not-ready node state fixture data."""
+    return json.loads(load_fixture("zwave_js/zp3111-5_not_ready_state.json"))
+
+
+@pytest.fixture(name="zp3111_state", scope="session")
+def zp3111_state_fixture():
+    """Load the zp3111 4-in-1 sensor node state fixture data."""
+    return json.loads(load_fixture("zwave_js/zp3111-5_state.json"))
+
+
 @pytest.fixture(name="client")
 def mock_client_fixture(controller_state, version_state, log_config_state):
     """Mock a client."""
@@ -919,3 +931,19 @@ def fortrezz_ssa3_siren_fixture(client, fortrezz_ssa3_siren_state):
 def firmware_file_fixture():
     """Return mock firmware file stream."""
     return io.BytesIO(bytes(10))
+
+
+@pytest.fixture(name="zp3111_not_ready")
+def zp3111_not_ready_fixture(client, zp3111_not_ready_state):
+    """Mock a zp3111 4-in-1 sensor node in a not-ready state."""
+    node = Node(client, copy.deepcopy(zp3111_not_ready_state))
+    client.driver.controller.nodes[node.node_id] = node
+    return node
+
+
+@pytest.fixture(name="zp3111")
+def zp3111_fixture(client, zp3111_state):
+    """Mock a zp3111 4-in-1 sensor node."""
+    node = Node(client, copy.deepcopy(zp3111_state))
+    client.driver.controller.nodes[node.node_id] = node
+    return node

tests/components/zwave_js/fixtures/zp3111-5_not_ready_state.json

@@ -0,0 +1,68 @@
+{
+    "nodeId": 22,
+    "index": 0,
+    "status": 1,
+    "ready": false,
+    "isListening": false,
+    "isRouting": true,
+    "isSecure": "unknown",
+    "interviewAttempts": 1,
+    "endpoints": [
+        {
+            "nodeId": 22,
+            "index": 0,
+            "deviceClass": {
+                "basic": {
+                    "key": 4,
+                    "label": "Routing Slave"
+                },
+                "generic": {
+                    "key": 7,
+                    "label": "Notification Sensor"
+                },
+                "specific": {
+                    "key": 1,
+                    "label": "Notification Sensor"
+                },
+                "mandatorySupportedCCs": [],
+                "mandatoryControlledCCs": []
+            }
+        }
+    ],
+    "values": [],
+    "isFrequentListening": false,
+    "maxDataRate": 100000,
+    "supportedDataRates": [
+        40000,
+        100000
+    ],
+    "protocolVersion": 3,
+    "supportsBeaming": true,
+    "supportsSecurity": false,
+    "nodeType": 1,
+    "deviceClass": {
+        "basic": {
+            "key": 4,
+            "label": "Routing Slave"
+        },
+        "generic": {
+            "key": 7,
+            "label": "Notification Sensor"
+        },
+        "specific": {
+            "key": 1,
+            "label": "Notification Sensor"
+        },
+        "mandatorySupportedCCs": [],
+        "mandatoryControlledCCs": []
+    },
+    "commandClasses": [],
+    "interviewStage": "ProtocolInfo",
+    "statistics": {
+        "commandsTX": 0,
+        "commandsRX": 0,
+        "commandsDroppedRX": 0,
+        "commandsDroppedTX": 0,
+        "timeoutResponse": 0
+    }
+}