Add unique_id to zwave node entity#14201
Conversation
| """Unique ID of Z-wave node.""" | ||
| return '{}-{}-{}'.format( | ||
| self.node_id, | ||
| slugify(self._manufacturer_name), |
There was a problem hiding this comment.
Don't manufacturer name and product name need to be fetched and thus it might be that a device gets a new unique ID after restart? Also, why wouldn't node id be enough ? And what about the devices like the 5-in-1 sensor, do they share a node id ?
There was a problem hiding this comment.
There are 2 options:
- Use node_id only: The unique_id will indeed be stable, but the default entity_id will be "_" (as both manufacturer and product are empty). So this bad entity_id will be kept even after restart.
- Use node_id, manufacturer and product. unique_id changes after restart getting a proper entity_id, but leaving a pollution in entity registry.
I think the best solution (out of scope of this PR) would be for zwave support for removing + re-adding the entity upon a change in manufacturer/product. This will allow properly adding zwave devices without restart.
There was a problem hiding this comment.
We should just depend on node id.
I don't like the pollution of the entity registry because what if a user starts configuring the devices? Renaming it, changing entity id (Future pr). It restarts and everything is lost.
How long does it take for Z-Wave to be aware of the manufacturer and product name? Wouldn't it make more sense to not add the entity to Home Assistant until manufacturer and product name are loaded? We could show a notification in the meanwhile to make sure that we notify the user that a device is pending.
There was a problem hiding this comment.
I should be quite fast (a few seconds) in the regular case of adding 1 powered device to an otherwise "ready" network.
There is also a case of losing zw_cfg file and restarting without it - in that case it would be a while: minutes, depending on network size, hours if waiting for unpowered devices to wake up.
The problem is I have trouble testing this (wait for data fetch) as my network zwave is quite large and each restart takes ages.
Maybe zwave should only report unique_id if manufacturer and product are non-empty? This will prevent pollution and keep things as-is (without registry) until data is properly fetched.
There was a problem hiding this comment.
Can we do a mix of both? We wait up to 30 seconds to see if manufacturer/product name arrive before adding it to Home Assistant. After 30 seconds we add it but it won't have a unique ID.
andrey-git
changed the title
|
Implemented. Marked as WIP because I didn't test it on a real system yet. |
| def _add_node_to_component(): | ||
| name = node_name(node) | ||
| generated_id = generate_entity_id(DOMAIN + '.{}', name, []) | ||
| node_config = device_config.get(generated_id) |
There was a problem hiding this comment.
(I realize it's existing logic)
This is not entirely correct anymore, as based on the unique ID we might choose to give them a different entity id. The correct implementation would be to leverage the disabling of entities via the entity registry. If an entity is disabled, we will never call async_added_to_hass (where subscriptions should happen)
There was a problem hiding this comment.
Yes, this bug is what got me working on zwave again in the first place.
I'll will address this in a future PR.
In any case, is the registry the right place for ignoring entities? Do other platforms use it for "ignore"?
Also - imaging a zombie zwave node that can't be parsed at all. This means it won't get unique_id, so it can't be ignored via the registry.
I propose to add a a list of ignored IDs to (root) zwave config to handle this case.
As for ignoring specific values - while entity registry would work here, I find it very convenient to ignore via zwave config globs, i.e:
sensor.*energy*:
ignored: true
There was a problem hiding this comment.
Btw a useful subset of globs can be used from the UI - a checkbox "ignore energy meters", "ignore power meters", etc.
|
|
||
| def _compute_unique_id(self): | ||
| if self._manufacturer_name and self._product_name: | ||
| return '{}-{}-{}'.format(self.node_id, |
There was a problem hiding this comment.
Can't we just use self.node_id now ?
There was a problem hiding this comment.
You mean wait for manufacturer & product and then use only node_id?
We could, but I think that if we have the extra data using it will make the id more unique and more readable for manual edits.
There was a problem hiding this comment.
The entity registry is not made for manual editing, we should do it all via the UI.
There was a problem hiding this comment.
I thin it is better to use more info in any case. I don't feel strongly about this.
There was a problem hiding this comment.
I think that we should limit unique ID to the least amount of unique data that we can have. btw, should we incorporate values too ?
There was a problem hiding this comment.
values have their own entities which already have unique_id based on node_id+value_id.
I need to check (in a separate PR in any case) whether the 30s wait is needed for values.
… attributes on node update.
|
I updated the unique_id to be |
andrey-git
changed the title
3 participants
Description:
Add unique_id to zwave node entity.
Based on nodeID
If manufacturer name or and product name is missing when the node is discovered wait up to 30 seconds before adding the node.
Update name, manufacturer name, and product name attributes on node update.
Checklist:
tox. Your PR cannot be merged unless tests pass