fix: use prefixed node.id in mqtt discovery topic

[tmaroschik]

If spaces are used within the node name, the mqtt discovery topic contains spaces as well.
This is not supported by a lot of MQTT clients, like homeassistant.

[zwave-js-assistant]

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

[coveralls]

Pull Request Test Coverage Report for Build 672967745

• 4 of 6 (66.67%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.006%) to 18.209%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/Gateway.js	4	6	66.67%

Files with Coverage Reduction	New Missed Lines	%
lib/Gateway.js	1	19.66%

Totals
Change from base Build 663906245:	0.006%
Covered Lines:	2026
Relevant Lines:	11410

---

💛 - Coveralls

[robertsLando]

@tmaroschik This could break existing instances. I would prefer to sanitize node name instead using the sinitize function you can find in utils

[tmaroschik]

I don't think so, because the topic in HASS discovery doesn't have a meaning. At least during testing it didn't make a difference upon changing.

[robertsLando]

So you can confirm that if someone had a node previously discovered using the node name instead the id this would not break his instances? Where does that node id is used?

[tmaroschik]

Yes I can confirm this by reading the manual: https://www.home-assistant.io/docs/mqtt/discovery/#discovery_prefix

<node_id> (Optional): ID of the node providing the topic, this is not used by Home Assistant but may be used to structure the MQTT topic.

[robertsLando]

Thanks @tmaroschik

[embcla]

This was a breaking change for me as I run multiple instances of zwavejs2mqtt: now there are unavoidable collisions whenever there is the same node ID in more than one instance.
Anyone with multiple instances will need something different than just node ID, or the nodes from different instances will be mixed up.

[blhoward2]

@robertsLando I think this may need to be reverted or reformulated. See my note on Slack. Is this also breaking node naming generally?

[blhoward2]

Maybe the docs are inaccurate. I wonder if in testing this PR mqtt didn't really rediscover the device? From reports it does seem to alter the default entity naming in HA. And this user makes a good point about colliding names with multiple instances.

[robertsLando]

Totally forgot about this, this is the reason that other user was expecting issue with discovery for sure. Should I revert this and release a new patch for now then we think about how to make things work in both cases?

BTW I had this feeling see my comment but I'm not an hass user and by reading docs it seems it should nont bereak anything. BTW I could add homeId before the node id, would this fix the problem?

[tmaroschik]

@embcla If its breaking the multi-instance scenario, then maybe make the prefix configurable, or prefix the node with the home id as well? Otherwise the name has to be sanitized so it doesn't contain any spaces or other special charcters.

[tmaroschik]

@blhoward2 The node name itself is derived in hass from the json payload, not the mqtt topic. You could even drop the whole topic segment and things would still work.

[tmaroschik]

@robertsLando In Hass, the node_id is used to form the discovery_id, which becomes the discovery_hash together with the component prefix. That tells the discovery that it has seen this object already and updates it. https://github.com/home-assistant/core/blob/f86beed7b047563c58cdddae71d55abde298ac85/homeassistant/components/mqtt/discovery.py#L136

It doesn't form any other identifier or name, visible in the UI. Throughout the flow, it is clear, that only the payload gets converted finally to the config, like here https://github.com/home-assistant/core/blob/99f9f8dec031d6e8d5f3f5443950d7980fceb739/homeassistant/components/mqtt/mixins.py#L154
In the end, you land back in the discovery module and the config entry gets added to Hass Data.

I'm no Python expert, so I might oversee some stuff. Do you know somebody, that could clarify this finally?

[robertsLando]

Some guys from hass maybe @balloob @marcelveldt @MartinHjelmare ?

[MartinHjelmare]

What is it that you want clarified?

[robertsLando]

@MartinHjelmare how does the node_id part in the discovery topic is used by hass?

[MartinHjelmare]

The whole discovery topic needs to be unique for Home Assistant to be able to distinguish between devices and its entities.

https://www.home-assistant.io/docs/mqtt/discovery/

It is recommended to make the object_id part of the topic a unique id.

[tmaroschik]

So the current implementation is incomplete, as I can freely define names that are either, using not allowed chars or duplicates.

Solutions options:

• use the location and name of the device to form a maybe unique identifier and pay attention to sanitisation
• use the home id and node id in conjunction

[embcla]

Is the homeID unique and does it match the limitations on acceptable characters?

Personally I'd go for location-deviceName sanitised

[blhoward2]

The homeId is long and sort of messy. It partially defeats the benefit of using the node name as the topic (more human readable, can stay the same after changing sticks, etc). I'd just sanitize it.

[robertsLando]

so what? We could eventually use object_id as suggested by @MartinHjelmare , or the sanitized location + node name, just tell me what you prefer

[blhoward2]

I personally think users like the location and name because it keeps it organized and human readable.