-
-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MQTT Sensor unique_id #13318
Add MQTT Sensor unique_id #13318
Conversation
Not sure if I understand this right: All this currently does is expose a |
@tinloaf When specifying the devices manually in For example, esphomelib has support for ds18b20 sensors that all have an individual 64-bit address. In the library I could then send this known address together with the discovery MQTT payload like this: {
"name": "Boiler Temperatur Oben",
"platform": "mqtt",
"state_topic": "heatpump/sensor/boiler_temperatur_oben/state",
"availability_topic": "heatpump/status",
"unit_of_measurement": "°C",
"expire_after": 1500,
"unique_id": "DallasComponent#0x01031663650aff28"
} The advantage would then be that I don't have to change the |
Sure - if unique IDs can be discovered from the devices, I'm all for it. I'm just not sure if it's a good idea to expose this in the config. It adds additional complexity without a benefit (that I can see?). My proposal would be: Add unique IDs where they can be discovered, but don't ask the user to manually configure unique IDs. |
Yes that was also my initial concern. I like your idea of only enabling it through MQTT discovery 👍 I will take a look at that. |
So I think that this is fine. Since MQTT is a transport instead of an actual device, we can leave it up to the user to decide what unique ID should be. To wrap up this PR:
|
|
About the test: With the test that I created previously I discovered #13316 - so it was a failing test; I now changed the test a bit so that it still passes and does at least part of the check it's supposed to do. Should we do something special with the home-assistant.io docs too? Is something like this good as a description:
|
That's a horrible thing to put in the docs haha. If 2 sensors have the same unique id, Home Assistant will raise an exception. |
This is amazing, really slick! With the right sketch on the MQTT device, this will allow adding MQTT devices (like Sonoffs and ESP8266) without the need to edit configuration.yaml at all. ESP.getChipId(); can be used to generate a unique_id on the backend. Could you please also enable this for binary_sensor and switch types? |
@thepotoo To make clear: This feature should only be used with sensors that have a laser-engraved serial number and only do one thing. I'll work on this more tomorrow and I'll limit this PR to sensors for now. If someone finds a valid use-case for binary sensors too, they can of course open a PR for that too. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is a PR to add a unique ID to MQTT sensors. Please do not discuss other topics. |
Oh, sorry! I have made a new PR for this, here: #13719
…On Fri, Apr 6, 2018 at 10:14 AM, Paulus Schoutsen ***@***.***> wrote:
This is a PR to add a unique ID to MQTT sensors. Please do not discuss
other topics.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13318 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AeFn9GCERJioaeDqego_KdIOxjtNUfmcks5tl3hZgaJpZM4SvRzG>
.
|
The From my point of view should the |
Is there ever a time when you would want to have a separate entity id and
unique id? I can't think of one, so I would fine with generating a
unique_id from the node id.
The whole point of doing this is to enable me to
1) use one firmware for all devices
and
2) be able to rename objects from within the UI
as long as I can do that, I'm happy.
…On Fri, Apr 6, 2018 at 1:10 PM, Fabian Affolter ***@***.***> wrote:
The discovery part already contains the option to set a device ID with
<node_id> as part of the topic. Something like home/sensor/xyz1234/
temperature/config would lead to an entity ID of xyz1234_temperature.
Looks pretty unique to me.
For my point of view should the node_id, if provided, go into the
unique_id. It's just a guess but I think that node_id was not only used
used to simplify the subscription of topics but also to identify devices.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13318 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AeFn9KCeO4nDgHH3LiDLj5nN5_m60Gaiks5tl6F0gaJpZM4SvRzG>
.
|
@fabaff At least speaking for esphomelib, it's not really unique, because the discovery topic is generated based on the name of the entity. So for example if a light is called "Living Room Lights", the object id will be "living_room_lights" for discovery. The problem with using The nice thing about having it as a configuration option is that it's more opt-in. So only the entities that really do have a unique ID (like dallas sensors), will actually receive a unique id. Enabling the unique_id feature for too many integrations will just lead to lots of user confusion. @thepotoo Just to explain, the problem with switch wasn't the code for me. It was more that I would first like to see how users are using this option. It's always easier to remove an option again from one integration than many at once. |
OK. I have been using this for a couple weeks now and really love the
feature. What can I do to help get it implemented for switches?
…On Fri, Apr 6, 2018 at 1:23 PM, Otto Winter ***@***.***> wrote:
@fabaff <https://github.com/fabaff> At least speaking for esphomelib,
it's not really unique, because the discovery topic is generated based on
the name of the entity. So for example if a light is called "Living Room
Lights", the object id will be "living_room_lights" for discovery. The
problem with using <node_id>_<object_id> for a *unique* id I think is
that discovery users *have* to specify an object id. Discovery just needs
the object id for topic names to work correctly. Unique IDs are supposed to
be unique for eternity and not depend on something like the friendly name
or user-defined string.
The nice thing about having it as a configuration option is that it's more
opt-in. So only the entities that really do have a unique ID (like dallas
sensors), will actually receive a unique id. Enabling the unique_id feature
for too many integrations will just lead to lots of user confusion.
@thepotoo <https://github.com/thepotoo> Just to explain, the problem with
switch wasn't the code for me. It was more that I would first like to see
how users are using this option. It's always easier to remove an option
again from one integration than many at once.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13318 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AeFn9AlOE1K3YM-tT3e2iQbE_cfiMno9ks5tl6SHgaJpZM4SvRzG>
.
|
This is just an implementation detail of esphomelib 😉
If you combine the address/ID/what-ever (
At the end of the day putting the |
Yes, I'm aware of that. I was just using it to hopefully make the point about forcing object_id for the unique_id not being that great of an idea IMO. If there's another way to make it work, I'm sure I would be able to get it to work in esphomelib too. However, there's really not much any system can base truly unique
Yes, that unique ID would be unique, but only in one direction (🎵🤦). You're right that every time an But the I mean I only started around the time unique ids were really enabled by the entity registry, so the latter "property" of unique ids (bi-directional uniqueness) is not required. |
So first off, MQTT is different for unique ID than other platforms. It's different because MQTT topics don't represent devices, it's up to the user to decide what is what. Does the topic represent the name, the value etc. Because unique ID has such a big impact, we should not try to be smart and auto detect it, we should only allow it to be set via the config. That's also why this PR has the comment in the voluptuous schema to be cautious. |
7701a91
to
2b8fc59
Compare
Ok, I've updated the code now and created a docs PR: home-assistant/home-assistant.io#5131. Is there something else that I should do? (I'd still be interested whether |
Yeah, it should be bi-directionally. That's why we try to aim to use serial etc. MQTT is a bit more loose because it's up to the user to decide what is a device. |
* Add MQTT Sensor unique_id * Add test * Update comment
Description:
Add
unique_id
capabilities to MQTT Sensors. Some sensors (like the ds18b20) have serial numbers that are burnt into the chip at production time. When coupled with MQTT discovery (and esphomelib 🤓) this could allow those sensors to be added to the entity registry.Very much an RFC and I have a couple of open questions about this:
unique_id
has not really been exposed to users, only to developers. This is the main reason I'm not sure about this.Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5131
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices: