Support abbreviations in discovery topic #16635

emontnemery · 2018-09-15T11:10:29Z

Description:

Support abbreviations in MQTT discovery messages. Purpose is to avoid out-of-memory situations on memory constrained devices when sending discovery topic.
Note: This is item 4 from home-assistant/architecture#70

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6294

Checklist:

The code change is tested and works locally.
Local tests pass with tox.

fabaff

A change like this would also require a test.

homeassistant/components/mqtt/discovery.py

emontnemery · 2018-09-16T07:06:08Z

@fabaff Thanks for review!
OK, Test case added to test the expansion
Also, the list of abbreviations should now cover all words in the configuration variables for supported devices.

houndci-bot · 2018-09-16T13:50:59Z

homeassistant/components/mqtt/discovery.py

+
+            def expand(matchobj):
+            """Helper to expand a possibly abbreviated word."""
+                abbreviation = matchobj.group(0)


unexpected indentation

houndci-bot · 2018-09-16T13:50:59Z

homeassistant/components/mqtt/discovery.py

+            pattern = r'(?:(?<=^)|(?<=_))[^\W_]+(?=_|$)'
+
+            def expand(matchobj):
+            """Helper to expand a possibly abbreviated word."""


expected an indented block
IndentationError: expected an indented block

emontnemery · 2018-09-16T15:47:59Z

Btw, I didn't add words relevant for Alarm control panel, HVAC and Lock (home-assistant/home-assistant.io#6295). I'll add those and and push a new commit.

balloob · 2018-09-17T09:29:14Z

We should wait with this PR until we finalized our discussion in the architecture repo?

emontnemery · 2018-09-17T09:59:46Z

@balloob Definitely, I added WIP tag

houndci-bot · 2018-09-19T10:54:41Z

tests/components/mqtt/test_discovery.py


-from tests.common import async_fire_mqtt_message, mock_coro
-
+from tests.common import (


'tests.common.mock_mqtt_component' imported but unused
'tests.common.fire_mqtt_message' imported but unused
'tests.common.get_test_home_assistant' imported but unused

houndci-bot · 2018-09-19T10:54:42Z

tests/components/mqtt/test_discovery.py

@@ -1,12 +1,15 @@
 """The tests for the MQTT discovery."""
 import asyncio
+import unittest


'unittest' imported but unused

emontnemery · 2018-09-19T11:02:06Z

@fabaff Please have a look at the updated version:

Test case added
Support for topic prefix added as discussed in MQTT Discovery improvement proposals architecture#70
Abbreviations of all words used in configuration variables added
(My idea initially was to only support abbreviations of the longest words as well as of the most frequently used words)

balloob · 2018-09-21T10:03:12Z

homeassistant/components/mqtt/discovery.py

+        if TOPIC_PREFIX in payload:
+            prefix = payload[TOPIC_PREFIX]
+            for key in payload.keys():
+                payload[key] = payload[key].replace(TOPIC_PREFIX, prefix)


We shouldn't do the replace like this, as it would also replace something like hello/~paulus. Instead, do this:

for key, value in payload.items(): if key[0] == TOPIC_PREFIX: payload[key] = "{}{}".format(TOPIC_PREFIX, value[1:])

OK, and that limits the topic prefix to the very beginning, maybe that's a reasonable limitation?
Perhaps it also makes sense to only do the replacement if the configuration variable ends with _topic, so something like:

for key, value in payload.items(): if value[0] == TOPIC_PREFIX and key.endswith('_topic'): payload[key] = "{}{}".format(TOPIC_PREFIX, value[1:])

Fixed in latest commit.

balloob · 2018-09-24T13:02:43Z

homeassistant/components/mqtt/discovery.py

@@ -62,6 +142,31 @@
            return

        payload = dict(payload)
+
+        for key in list(payload.keys()):


Wait, how does this work, I was under the impression that we had a fixed list of abbreviation -> full key, which would not require a regex. If we can, let's avoid regex.

It's a (long) fixed list of abbreviated words, the purpose of the regex was to split the configuration variable name in words and attempt to expand word by word.
Regex replaced with string.split()

balloob · 2018-09-24T13:03:51Z

homeassistant/components/mqtt/discovery.py

+        if TOPIC_PREFIX in payload:
+            prefix = payload[TOPIC_PREFIX]
+            for key, value in payload.items():
+                if value.startswith(TOPIC_PREFIX) and key.endswith('_topic'):


value[0] will be faster.

balloob · 2018-09-24T13:04:30Z

homeassistant/components/mqtt/discovery.py

+            for key, value in payload.items():
+                if value.startswith(TOPIC_PREFIX) and key.endswith('_topic'):
+                    payload[key] = "{}{}".format(prefix,
+                                                 value[len(TOPIC_PREFIX):])


I don't think we'll ever change the prefix, I think just using 1: would be more readable.

emontnemery · 2018-09-28T09:07:37Z

@balloob Yes, exactly. I'm not sure what the rationale for the "backwards" default topic scheme is, maybe to make it easier to subscribe to all state updates as you suggest.
But the point is that it is too restrictive if the "prefix" introduced by this PR must be at the start of the configuration value. Do you consider my updated suggestion acceptable?

balloob · 2018-10-01T10:39:11Z

Why can't we have nice things 😢

Looks like in Tasmota the topics can be configured to have the tasmota "prefix" be last: https://github.com/arendst/Sonoff-Tasmota/wiki/MQTT-Features#mqtt-topic-definition

emontnemery · 2018-10-01T11:03:36Z

@balloob Yes, exactly, and "prefix" last is even the default setting.

Hence, can I update the PR by replacing:

# Replace TOPIC_PREFIX at the start of configuration value
if TOPIC_PREFIX in payload:
    prefix = payload[TOPIC_PREFIX]
    for key, value in payload.items():
        if value[0] == TOPIC_PREFIX and key.endswith('_topic'):
            payload[key] = "{}{}".format(prefix, value[1:])

with:

# Replace BASE_TOPIC in configuration value
if BASE_TOPIC in payload:
    base_topic = payload[BASE_TOPIC]
    for key in payload.keys():
        if key.endswith('_topic'):
            payload = payload.split('/')
            payload = [x if x != BASE_TOPIC else base_topic for x in payload]
            payload[key] = '/'.join(payload)

homeassistant/components/mqtt/discovery.py

Add missing docstring.

…r long

emontnemery · 2018-10-06T14:19:22Z

@fabaff, @balloob Are any more changes required?

balloob · 2018-10-08T14:03:09Z

But if default prefix is last, then why would we have to update to support the base topic in every slot? Couldn't we get the user to just configure it properly?

emontnemery · 2018-10-10T13:46:23Z

@balloob OK, how about supporting base topic either first (this makes most sense) or last (since it's Tasmota defualt) then?

balloob · 2018-10-10T20:59:05Z

Let's do it. I don't like it though 😢

emontnemery · 2018-10-11T15:05:50Z

@balloob Done!

emontnemery requested a review from a team as a code owner September 15, 2018 11:10

homeassistant added integration: mqtt core cla-signed labels Sep 15, 2018

ghost added the in progress label Sep 15, 2018

fabaff requested changes Sep 15, 2018

View reviewed changes

homeassistant/components/mqtt/discovery.py Outdated Show resolved Hide resolved

homeassistant/components/mqtt/discovery.py Show resolved Hide resolved

fabaff mentioned this pull request Sep 15, 2018

MQTT Discovery improvement proposals home-assistant/architecture#70

Closed

homeassistant added the cla-signed label Sep 16, 2018

houndci-bot reviewed Sep 16, 2018

View reviewed changes

emontnemery force-pushed the mqtt_discovery_shortened branch from 8e1bb40 to c287260 Compare September 16, 2018 13:52

homeassistant added the cla-signed label Sep 16, 2018

emontnemery force-pushed the mqtt_discovery_shortened branch from c287260 to 1d39883 Compare September 16, 2018 14:46

homeassistant added the cla-signed label Sep 16, 2018

emontnemery mentioned this pull request Sep 16, 2018

Documentation for abbreviations in discovery topic home-assistant/home-assistant.io#6294

Merged

2 tasks

emontnemery changed the title ~~Support abbreviations in discovery topic~~ WIP - Support abbreviations in discovery topic Sep 17, 2018

homeassistant added the cla-signed label Sep 18, 2018

houndci-bot reviewed Sep 19, 2018

View reviewed changes

emontnemery force-pushed the mqtt_discovery_shortened branch from b757c07 to eed9472 Compare September 19, 2018 10:58

homeassistant added the cla-signed label Sep 19, 2018

balloob reviewed Sep 21, 2018

View reviewed changes

homeassistant added the cla-signed label Sep 22, 2018

balloob reviewed Sep 24, 2018

View reviewed changes

emontnemery force-pushed the mqtt_discovery_shortened branch from ded3945 to 8fc9c96 Compare September 24, 2018 17:06

fabaff reviewed Oct 1, 2018

View reviewed changes

homeassistant/components/mqtt/discovery.py Outdated Show resolved Hide resolved

emontnemery added 12 commits October 2, 2018 16:21

Support abbreviations in discovery topic

939a5e9

Add abbreviations for all words. Add testcase.

d510e78

Add missing docstring.

Add missing abbreviations

dc1e60e

Support topic prefix

681ea54

Update test case

82d8fb6

Restrict topic prefix

be8cebd

Fix merge

baec2db

Simplify abbreviations expanding, assume TOPIC_PREFIX is one characte…

094b148

…r long

Support abbreviated keys instead of words

a47e350

Remove redundant abbreviations

b961d20

Remove extra spaces in abbreviation list

fb1d9aa

Make topic prefix less restrictive

7a2d892

emontnemery force-pushed the mqtt_discovery_shortened branch from be57bcb to 7a2d892 Compare October 2, 2018 14:40

emontnemery changed the title ~~WIP - Support abbreviations in discovery topic~~ Support abbreviations in discovery topic Oct 2, 2018

Make topix prefix a bit more restrictive again

a8d04b3

balloob approved these changes Oct 12, 2018

View reviewed changes

balloob merged commit b2789d9 into home-assistant:dev Oct 12, 2018

ghost removed the in progress label Oct 12, 2018

emontnemery deleted the mqtt_discovery_shortened branch October 12, 2018 13:13

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support abbreviations in discovery topic #16635

Support abbreviations in discovery topic #16635

emontnemery commented Sep 15, 2018 •

edited

Loading

fabaff left a comment

emontnemery commented Sep 16, 2018 •

edited

Loading

houndci-bot Sep 16, 2018

houndci-bot Sep 16, 2018

emontnemery commented Sep 16, 2018

balloob commented Sep 17, 2018

emontnemery commented Sep 17, 2018

houndci-bot Sep 19, 2018

houndci-bot Sep 19, 2018

emontnemery commented Sep 19, 2018

balloob Sep 21, 2018

emontnemery Sep 22, 2018

emontnemery Sep 22, 2018

balloob Sep 24, 2018

emontnemery Sep 24, 2018 •

edited

Loading

balloob Sep 24, 2018

emontnemery Sep 24, 2018 •

edited

Loading

balloob Sep 24, 2018

emontnemery Sep 24, 2018 •

edited

Loading

emontnemery commented Sep 28, 2018

balloob commented Oct 1, 2018

emontnemery commented Oct 1, 2018

emontnemery commented Oct 6, 2018

balloob commented Oct 8, 2018

emontnemery commented Oct 10, 2018

balloob commented Oct 10, 2018

emontnemery commented Oct 11, 2018


		from tests.common import async_fire_mqtt_message, mock_coro

		from tests.common import (

Support abbreviations in discovery topic #16635

Support abbreviations in discovery topic #16635

Conversation

emontnemery commented Sep 15, 2018 • edited Loading

Description:

Checklist:

fabaff left a comment

Choose a reason for hiding this comment

emontnemery commented Sep 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery commented Sep 16, 2018

balloob commented Sep 17, 2018

emontnemery commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

emontnemery commented Sep 28, 2018

balloob commented Oct 1, 2018

emontnemery commented Oct 1, 2018

emontnemery commented Oct 6, 2018

balloob commented Oct 8, 2018

emontnemery commented Oct 10, 2018

balloob commented Oct 10, 2018

emontnemery commented Oct 11, 2018

emontnemery commented Sep 15, 2018 •

edited

Loading

emontnemery commented Sep 16, 2018 •

edited

Loading

emontnemery Sep 24, 2018 •

edited

Loading

emontnemery Sep 24, 2018 •

edited

Loading

emontnemery Sep 24, 2018 •

edited

Loading