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

Zigbee2mqtt subscribing inefficient? #3102

Closed
svrooij opened this issue Mar 11, 2020 · 13 comments
Closed

Zigbee2mqtt subscribing inefficient? #3102

svrooij opened this issue Mar 11, 2020 · 13 comments
Labels
stale Stale issues

Comments

@svrooij
Copy link

svrooij commented Mar 11, 2020

Bug Report

What happened

I have a different mqtt server then most people use. I use EMQX one of the reasons is that is has a nice visual dashboard to display all sorts of information about the mqtt clients that are connected.

Screenshot 2020-03-11 at 10 56 01

This is a small portion of all the subscriptions zigbee2mqtt creates. It currently has 199 different subscriptions.

What did you expect to happen

I would expect it to use the # wildcard to tell the mqtt server that it wants all topics with this multi-level wildcard.
A single zigbee2mqtt/#/get could replace all these subscriptions

  • zigbee2mqtt/+/get
  • zigbee2mqtt/+/+/get
  • zigbee2mqtt/+/+/+/get
  • zigbee2mqtt/+/+/+/+/get

And maybe it should just subscribe to zigbee2mqtt/# but than it will also receive it's own messages.

How to reproduce it (minimal and precise)

  1. Start zigbee2mqtt
Koenkk added a commit that referenced this issue Mar 11, 2020
@Koenkk
Copy link
Owner

Koenkk commented Mar 11, 2020

Fixed in the latest dev branch, please confirm 😄

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

@Koenkk 82080e3 broke groups and devices with a / in them again.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

Even worse, this breaks everything for me!
Even zigbee2mqtt/0x00158d0001ffaffc/get fails, which should definitely work

@svrooij
Copy link
Author

svrooij commented Mar 11, 2020

Shouldn't devices and groups name's not just deny / in the name?
Or at least in the mqtt topics? It is recommended to only use A-Za-z0-9 and -_.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

I and a few other people use / in the name to provide some order e.g. <floor>/<room>/<device>, this also makes it easy to get only messages for a certain floor/room.

There was some discussion about this a while back as it was broken in a few odd places but they were fix, because suddenly disallowing / would probably break a lot of things for people.

I guess I can spend a few weekend redoing everything to not have / but I am certainly not the only one using this... also since zigbee2mqtt/0x00158d0001ffaffc/get is also broken, I suspect maybe my broker does not support # when used in the middle.

From some testing it is fine if # is the last bit of the subscripting topic only.

zigbee2mqtt/# seems to work fine while zigbee2mqtt/#/get for example doesn't seem to get any message I send it either. I tried with both aedes and mosce.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

It looks to indeed be that none of the brokers I tried support # in the middle of a subscription topic. Aedes is the one I mainly use and I made it print every client that tried to subscribe, all z2m's subcriptions with # was not at the end were returning an empty subscription list!

@Koenkk will we start enforcing certain brokers from now on?

@svrooij
Copy link
Author

svrooij commented Mar 11, 2020

Sorry @Koenkk for causing trouble.

How about using zigbee2mqtt/# as a single subscription? That should allow all possible options.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

@svrooij eh it happens :) we're all trying to make z2m better and your observation is correct that it does add a lot of subs...

Multi Level: #

The multi-level wildcard covers many topic levels.
The hash symbol represents the multi-level wild card in the topic. 
For the broker to determine which topics match, 
the multi-level wildcard must be placed as the 
last character in the topic and preceded by a forward slash.

-- https://www.hivemq.com/blog/mqtt-essentials-part-5-mqtt-topics-best-practices/

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

So looks like both Aedes and Mosce are doing the correct thing here and rejecting those subscriptions.

Koenkk added a commit that referenced this issue Mar 11, 2020
@svrooij
Copy link
Author

svrooij commented Mar 11, 2020

BTW. I haven't tested a subscription with something after the # character for EMQX. So if it's not allowed in the specs that should be followed.

@Koenkk
Copy link
Owner

Koenkk commented Mar 11, 2020

Reverted and only added it to the cases were it can be applied (should save 38 subscriptions).

Regarding: #3102 (comment) in that case Zigbee2mqtt will also start listening to it's own message which I don't think is a good idea. If zigbee2mqtt would e.g. publish to zigbee2mqtt/my_device it would receive this message again leading to unnecessary messages send to zigbee2mqtt (= traffic).

@sjorge
Copy link
Sponsor Contributor

sjorge commented Mar 11, 2020

Looks, good... everything is working again.

@stale
Copy link

stale bot commented May 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues label May 10, 2020
@stale stale bot closed this as completed May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues
Projects
None yet
Development

No branches or pull requests

3 participants