allow wildcards in subscription#12247
Conversation
| if mqtt._match_topic(subscription, topic): | ||
| hass.async_add_job(async_see( | ||
| dev_id=dev_id_lookup[subscription], | ||
| location_name=payload)) |
| async_see(dev_id=dev_id_lookup[topic], location_name=payload)) | ||
| for subscription in dev_id_lookup: | ||
| if mqtt._match_topic(subscription, topic): | ||
| hass.async_add_job(async_see( |
| hass.async_add_job( | ||
| async_see(dev_id=dev_id_lookup[topic], location_name=payload)) | ||
| for subscription in dev_id_lookup: | ||
| if mqtt._match_topic(subscription, topic): |
| hass.async_add_job( | ||
| async_see(dev_id=dev_id_lookup[topic], location_name=payload)) | ||
| for subscription in dev_id_lookup: | ||
| if mqtt.match_topic(subscription, topic): |
There was a problem hiding this comment.
Why not just create a separate callback for each subscription?
There was a problem hiding this comment.
Like this:
for dev_id, topic in devices.items():
@callback
def async_tracker_message_received(topic, payload, qos):
"""Handle received MQTT message."""
hass.async_add_job(
async_see(dev_id=dev_id, location_name=payload))
yield from mqtt.async_subscribe(
hass, topic, async_tracker_message_received, qos)There was a problem hiding this comment.
Is it good style to pass the loop-local dev_id variable into a callback?
What value has the callback internal dev_id?
The one from the loop or something unpredictable?
There was a problem hiding this comment.
That is indeed true, this wouldn't work
But using a callback creator should work:
def make_callback(dev_id):
@callback
def async_tracker_message_received(...):
# [...]
return async_tracker_message_received
for dev_id, topic in devices.items():
yield from mqtt.async_subscribe(hass, topic, make_callback(dev_id), qos)I believe this would still be better than handling the topic matching yourself, since mqtt already handles matching and knows best how to do that.
There was a problem hiding this comment.
According to https://stackoverflow.com/a/3431699/3775041 this should also work:
for dev_id, topic in devices.items():
@callback
def async_tracker_message_received(topic, payload, qos, dev_id=dev_id):
"""Handle received MQTT message."""
hass.async_add_job(
async_see(dev_id=dev_id, location_name=payload))
yield from mqtt.async_subscribe(
hass, topic, async_tracker_message_received, qos)There was a problem hiding this comment.
But you are right, the mqtt-outside handling of topics is not the best idea.
There was a problem hiding this comment.
BTW the old code also uses this strange callback-outside variable style with the dev_id_lookup dictionary.
|
Can you add a test to make sure that it works. |
|
I don't have experiences with test creation. Who can support me? |
|
@escoand Just take a look at test/components/device_tracker/test_mqtt.py. The last test should be easy enough to duplicate and add some tests will publishing to several topics and making sure they all get processed. Test both '+' and '#' |
|
What you have is fine since the initial state will != what you are testing. The test look to be failing though. You can run the test locally using tox. Make sure your updates are in whatever ha is being loaded (ie use a virtual env and you can do pip -e homeassistant to point to your dev copy), an running this will run the test on the one file only |
|
Ok, but now I've fixed my build errors. |
* allow wildcards in subscription * remove whitespaces * make function public * also implement for mqtt_json * avoid mqtt-outside topic matching * add wildcard tests * add not matching wildcard tests * fix not-matching tests
ghost
added
7 participants
Description:
Currently it's possible to use a subscription topic with wildcards but an error is thrown when received because it's not equals the received topic. This PR adds the missing match to the subscription topic.
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass