Avoid removing zwave_js devices for non-ready nodes

[kpine]

Proposed change

If the zwave_js integration receives a "node added" event for a node that is not ready but matches an already created device, it will remove the device and re-create it, even if the node is the correct/original one. Removing the device loses all of the user device and entity customization (naming, etc.). Normally, this kind of event is only seen when a node has been included for the first time. It is also emitted for the following scenarios:

1. A cache loss or corruption, where all the node info must be rediscovered
2. Replacing a failed node

Additionally, a third case also causes a device removal: a product name is changed in the driver config file. The device replacement logic mistakenly believes a different node has replaced the original since the labels no longer match.

To correct this, the device registration logic has two main changes:

1. Skip removing devices which don't have a manufacturer id / product id / product type. In this state it's impossible to determine if a node is being replaced or not. This fixes the first two issues listed.
2. Register the node's manufacturer id / product id / product type fields as an additional device ID and use that to determine whether a node is replacing an existing one or not. These IDs are static and take the place of the device labels. This fixes the second and third issue.

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

Additional information

• This PR fixes or closes issue: fixes Replacing a failed node removes the device and all customizations (zwave_js) #58874
• This PR is related to issue:
• Link to documentation pull request:

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.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @home-assistant/z-wave, mind taking a look at this pull request as it has been labeled with an integration (zwave_js) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[kpine]

@MartinHjelmare I'm still planning to complete this, just haven't had much time to do so recently.

I can confirm that your comments about the Value references are correct. After replacing the node, the entity states do not change when the device does something. That's something I didn't pay attention to earlier, good catch.

[kpine]

Latest commits are working, when a node is replaced all of the entities go unavailable until the node becomes ready. After ready state, the entities are all working again, including the node status sensor.

When a node is replace, the driver will send "value removed" and a corresponding "metadata updated" event for each value (I'm not sure why the metadata updated event is there). I'm using that event to trigger an entity to remove itself. It might be more appropriate for the node removal event to also trigger entity removals (or instead of), in case some value was missed. I wasn't sure what the correct way is to tell all entities to remove themselves.

The node status sensor is special, which no associated value Id, so I'm using the event dispatcher to send a message. It's a little awkward to send the event, as it relies on knowing how the entity's unique ID is made.

Looks like I need to add some tests as well, I'll do that after I get some feedback, since I'm not so sure about this approach.

[MartinHjelmare]

Sounds good. I think it's ok to use the value removed event to trigger an entity remove.

[kpine]

Sorry for the rebase, I had an dev environment issue that I thought might be related to being out of date. Also resolved a merge conflict in the test code.

[MartinHjelmare]

Great!

[raman325]

this issue seems to be coming up more and more often - as a result I've tagged this for a patch release