Skip to content
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

MQTT Discovery improvement proposals #70

Closed
emontnemery opened this issue Sep 12, 2018 · 18 comments
Closed

MQTT Discovery improvement proposals #70

emontnemery opened this issue Sep 12, 2018 · 18 comments

Comments

@emontnemery
Copy link
Contributor

emontnemery commented Sep 12, 2018

A few proposals for changes to MQTT discovery:

  1. Support removal of previously discovered device when a (retained) discovery payload is cleared. This would be highly useful for devices such as "Sonoff Tasmota" which have some default configuration when flashing with official firmware (configured as a single switch) and which might have a completely different functionality after setup (e.g. an RGBWW LED dimmer light ). If acceptable, I'll finish what was started in PR Mqtt discovery remove core#11489.
    PR: Remove discovered MQTT Switch device when discovery topic is cleared core#16605 + follow ups for other platforms

  2. Support reconfiguration of a previously discovered device when the discovery payload is modified. The purpose is very similar to 1).
    PR: Reconfigure MQTT binary_sensor component if discovery info is changed core#18169 + follow ups for other platforms

  3. Do not reject multiple devices with same 'name' in the discovery message, again the purpose is better support for generic device binary without requiring a homeassistant restart after (re)configuration.
    Not an issue with MQTT discovery, this issue was caused by Reconfigure MQTT climate component if discovery info is changed core#18174.

  4. The mqtt discovery protocol is very verbose, which is causing out-of-memory issues for memory constrained devices when sending the discovery message. An example is the "Sonoff-Tasmota" project, where it's not possible to add information about supported effects because it would not be possible to send the resulting very long discovery message.
    A simple solution would be to support some common and documented abbreviations, such as '_topic' -> '_t', '_template'->'_tmp' etc in the discovery message.
    PR: Support abbreviations in discovery topic core#16635

@balloob
Copy link
Member

balloob commented Sep 13, 2018

CC @fabaff as you have been working on discovery.

@balloob
Copy link
Member

balloob commented Sep 13, 2018

I personally think 1-2 are fine. I think that a reconfigure / removal could be accomplished by either having the resulting MQTT entity listen to MQTT directly, or have it listen to updates from discovery via the dispatcher.

  1. how would we know this should be a new device and is not a reconfiguration?

  2. Would be fine.

If you plan on opening PRs to support this, please limit each PR to one of the issues.

@emontnemery
Copy link
Contributor Author

@balloob Thanks for comments! And yeah, I was planning to open PRs.

removal could be accomplished by either having the resulting MQTT entity listen to MQTT directly, or have it listen to updates from discovery via the dispatcher

Can you explain this in some more detail please?
Do you mean the first option is that the resulting MQTT entity itself subscribes to relevant homeassistant/ topic in the same way as it already does to status updates etc?
What about the second option?

@fabaff
Copy link
Member

fabaff commented Sep 14, 2018

  1. This would be an useful addition.
  1. Support reconfiguration of a previously discovered device when the discovery payload is modified.

Reconfiguration of a device means in a lot of cases that the topics will change too because it's no longer a switch but a light (if I understand you right in 1.). Now, comes the tricky part: If the domain is changed (e.g. from switch to light) then the discovery_hash (as implemented in home-assistant/core#11489) will no longer match an existing device, thus you can't update the previous added device.

It will more be "remove an old device and add a new one" with one hiccup: The new device doesn't know the original discovery_hash. In fact the discovery_hash was only valid as the device was around that created the ID.

As long as the device is in the same domain would it be nice to be able to update the attributes and alike.

  1. Do not reject multiple devices with same 'name' in the discovery message, again the purpose is better support for generic device binary without requiring a homeassistant restart after (re)configuration.

Devices with the same name: are not rejected because the <object_id> is the distinction. Check:

$ mosquitto_pub -h 127.0.0.1 -p 1883 -u homeassistant -P test123 -t "home/binary_sensor/garden/config" -m '{"name": "garden", "device_class": "motion"}' -V mqttv311
$ mosquitto_pub -h 127.0.0.1 -p 1883 -u homeassistant -P test123 -t "home/binary_sensor/garden1/config" -m '{"name": "garden", "device_class": "motion"}' -V mqttv311

With this will give you two sensor with the name "garden". name: is an optional key that let the users overwrite the default.

  1. The mqtt discovery protocol is very verbose,...

This sounds like an issue that should be solved by Tasmota and not Home Assistant. It's a common problem that PubSubClient and other MQTT libs are limiting the payload size (see #define MQTT_MAX_PACKET_SIZE 128). I'm wondering what we are winning if we start using abbreviations because the values in the payloads are still the same as before (like cmnd/my_house_living_room/power).

But if there is an easy way with only a little maintenance we can try to use some sort of mapping to make it easier to integrate implementation which are limited or upstream refused to make the needed changes.

@balloob
Copy link
Member

balloob commented Sep 14, 2018

If you're limiting to 128 chars, writing _dc instead of device_class would be a significant change. We could maintain a small mapping dictionary in the discovery topic listener to rewrite payload before passing it on.

@emontnemery
Copy link
Contributor Author

emontnemery commented Sep 14, 2018

a small mapping dictionary in the discovery topic listener to rewrite payload before passing it on

Yes, I hope to push a proposal during the weekend unless someone else wants to do it?
(Tasmota is not limiting MQTT messages to 128 bytes btw, I belive it's around 1500 bytes. Still, that's not enough for an RGBWW light which also supports effects.)

@emontnemery
Copy link
Contributor Author

@fabaff Thanks for comments!

Devices with the same name: are not rejected because the <object_id> is the distinction.

Aha, you're right! I have seen an issue with not all devices being discovered and I though it was related to the name being the same, but it must be something different. I removed 3. from the description.

@fabaff
Copy link
Member

fabaff commented Sep 15, 2018

@emontnemery, can you please post the payload of the discovery message that is send by Sonoff-Tasmota to the config topic? While looking at home-assistant/core#16635 it seems that I didn't get the point.

@emontnemery
Copy link
Contributor Author

@fabaff Sure, here's an example of configuration data for an RGBWW light:
Topic: homeassistant/light/sonoff_9D3B0B_1/config
Payload, 868 bytes in the example.
Note: Line breaks added for readability. The device topic, sonoff_9D3B0Bin this example, may be up to 32 characters + terminating NUL.
(It would be really useful in terms of conserving space if there was some way to tag a commonly used string, in this case tasmota/sonoff_9D3B0B, but maybe that's complicating things too much)

{  
   "name":"Sonoff B1",
   "command_topic":"tasmota/sonoff_9D3B0B/cmnd/POWER",
   "state_topic":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "value_template":"{{value_json.POWER}}",
   "payload_off":"OFF",
   "payload_on":"ON",
   "availability_topic":"tasmota/sonoff_9D3B0B/tele/LWT",
   "payload_available":"Online",
   "payload_not_available":"Offline",
   "brightness_command_topic":"tasmota/sonoff_9D3B0B/cmnd/Dimmer",
   "brightness_state_topic":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "brightness_scale":100,
   "on_command_type":"brightness",
   "brightness_value_template":"{{value_json.Dimmer}}",
   "rgb_command_topic":"tasmota/sonoff_9D3B0B/cmnd/Color",
   "rgb_state_topic":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "rgb_value_template":"{{value_json.Color}}",
   "color_temp_command_topic":"tasmota/sonoff_9D3B0B/cmnd/CT",
   "color_temp_state_topic":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "color_temp_value_template":"{{value_json.CT}}"
}

Example of shortened payload (717 bytes) as per current proposal in home-assistant/core#16635:

{  
   "name":"Sonoff B1",
   "cmd_t":"tasmota/sonoff_9D3B0B/cmnd/POWER",
   "stat_t":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "val_tmp":"{{value_json.POWER}}",
   "pl_off":"OFF",
   "pl_on":"ON",
   "avail_t":"tasmota/sonoff_9D3B0B/tele/LWT",
   "pl_avail":"Online",
   "pl_not_avail":"Offline",
   "bri_cmd_t":"tasmota/sonoff_9D3B0B/cmnd/Dimmer",
   "bri_stat_t":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "bri_scale":100,
   "on_cmd_type":"brightness",
   "bri_val_tmp":"{{value_json.Dimmer}}",
   "rgb_cmd_t":"tasmota/sonoff_9D3B0B/cmnd/Color",
   "rgb_stat_t":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "rgb_val_tmp":"{{value_json.Color}}",
   "color_temp_cmd_t":"tasmota/sonoff_9D3B0B/cmnd/CT",
   "color_temp_stat_t":"tasmota/sonoff_9D3B0B/stat/RESULT",
   "color_temp_val_tmp":"{{value_json.CT}}"
}

@rohankapoorcom
Copy link
Member

rohankapoorcom commented Sep 16, 2018

@emontnemery It sounds like adding a topic prefix, which would be set to tasmota/sonoff_9D3B0B in your example would do a lot of good in terms of shrinking the payload size. I see that repeated in nine different topics in your example. This would allow it to only exist once.

Code would do something like check if the topic prefix exists, prepend it to all of the topics received in the discovery payload. If it doesn't exist, no change.

@OttoWinter
Copy link
Member

In my experience, the limited payload size can easily be fixed on most platforms by using more sophisticated MQTT libraries/solutions. For example, esphomelib streams all its MQTT packets to work around the limited TX buffer size on some MPUs. So for bigger frameworks like tasmota that should be the solution.

Of course, most DIY developers use libraries like pubsubclient that have hard limits on the maximum packet size (for example 128 bytes). But I think most of those people will:

a) not use discovery as implementing discovery is way harder to do than just adding a few lines to your YAML config and
b) will not have very long discovery payloads. For example, I doubt many people developing their own solution will go to the length of implementing availability.

Another thing is that this 128 byte max packet size is only really a problem for one platform: light. As long as you don't have too long MQTT topics (in which case topic prefixes would help either), 128 bytes should be plenty for platforms like sensor.

light, however, usually needs to have much longer discovery packets. As in above case up to 868 bytes. In this case, it's evident that things like shortening payloads or using topic prefixes will never lead to something that fits in 128 bytes. I think a good measure for how much the payload could *roughly be shorter is by running the raw payload through gzip. In my case, running above 868 byte long payload through gzip yields a file with length 290, which still won't fit into the magic 128 bytes.

Btw, tasmota could easily get to 250 bytes by just using the mqtt_json platform ;)

{
  "platform": "mqtt_json",
  "brightness": true,
  "rgb": true,
  "color_temp": true,
  "name": "Sonoff B1",
  "availability_topic": "tasmota/sonoff_9D3B0B/tele/LWT",
  "state_topic": "tasmota/sonoff_9D3B0B/stat/json",
  "command_topic": "tasmota/sonoff_9D3B0B/tele/json"
}

Please correct me if I'm wrong but in my view topic prefixes/abbreviated keys would definitely reduce the payload length, but:

  • in the cases where it wasn't a problem previously, well, it wouldn't make a difference.
  • in the cases where it was a problem (like lights), it would still not fix the problem (because still not <128 or <256 bytes)

And whole frameworks which tend to have bigger discovery payloads can easily work around the problem by choosing the MQTT library more wisely :)

But 1&2 I would be very excited about :)

@balloob
Copy link
Member

balloob commented Sep 17, 2018

Would it be an idea to introduce ~ for topics and it will be replaced with the base topic the device was found on, like homeassistant/light/sonoff_9D3B0B_1 ?

@emontnemery
Copy link
Contributor Author

@balloob That's not a bad idea!
In the tasmota example, base topic is not same as discovery topic, although it is user configurable.

Would something like this be acceptable for more flexibility:
Topic: homeassistant/light/sonoff_9D3B0B_1/config

Payload:
{  
   "~1":"tasmota/sonoff_9D3B0B/cmnd",
   "~2":"tasmota/sonoff_9D3B0B/stat",
   "~3":"tasmota/sonoff_9D3B0B/tele",
   "name":"Sonoff B1",
   "cmd_t":"~1/POWER",
   "stat_t":"~2/RESULT",
   "val_tmp":"{{value_json.POWER}}",
   "pl_off":"OFF",
   "pl_on":"ON",
   "avail_t":"~3/LWT",
   "pl_avail":"Online",
   "pl_not_avail":"Offline",
   "bri_cmd_t":"~1/Dimmer",
   "bri_stat_t":"~2/RESULT",
   "bri_scale":100,
   "on_cmd_type":"brightness",
   "bri_val_tmp":"{{value_json.Dimmer}}",
   "rgb_cmd_t":"~1/Color",
   "rgb_stat_t":"~2/RESULT",
   "rgb_val_tmp":"{{value_json.Color}}",
   "color_temp_cmd_t":"~1/CT",
   "color_temp_stat_t":"~2/RESULT",
   "color_temp_val_tmp":"{{value_json.CT}}"
}

@balloob
Copy link
Member

balloob commented Sep 17, 2018

No, that seems a bit too wild. If anything, maybe just ~.

Can't we have tasmota publish to the topic minus config?

@emontnemery
Copy link
Contributor Author

emontnemery commented Sep 17, 2018

@balloob The main issue is that Tasmota doesn't have the platform type (sensor, light, switch) as a mandatory part of both in- and outgoing topics.

Maybe something like this then:
Topic: homeassistant/light/sonoff_9D3B0B_1/config
Payload:

{  
   "~":"tasmota/sonoff_9D3B0B",
   "name":"Sonoff B1",
   "cmd_t":"~/cmnd/POWER",
   "stat_t":"~/stat/RESULT",
   "val_tmp":"{{value_json.POWER}}",
   "pl_off":"OFF",
   "pl_on":"ON",
   "avail_t":"~/tele/LWT",
   "pl_avail":"Online",
   "pl_not_avail":"Offline",
   "bri_cmd_t":"~/cmnd/Dimmer",
   "bri_stat_t":"~/stat/RESULT",
   "bri_scale":100,
   "on_cmd_type":"brightness",
   "bri_val_tmp":"{{value_json.Dimmer}}",
   "rgb_cmd_t":"~/cmnd/Color",
   "rgb_stat_t":"~/stat/RESULT",
   "rgb_val_tmp":"{{value_json.Color}}",
   "color_temp_cmd_t":"~/cmnd/CT",
   "color_temp_stat_t":"~/stat/RESULT",
   "color_temp_val_tmp":"{{value_json.CT}}"
}

@rohankapoorcom
Copy link
Member

rohankapoorcom commented Sep 18, 2018

@emontnemery that is pretty much what I was envisioning in terms of topic prefixes. I assume we would want to implement them in a backwards compatible way?

I agree with @balloob, we should keep it to one ~.

@emontnemery
Copy link
Contributor Author

OK, updated home-assistant/core#16635 with topic prefix

@emontnemery
Copy link
Contributor Author

Closing this since all the proposals are now implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants