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.

Sign up for GitHub

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

Enable shared subscription for MQTTv5 #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −0
Open

Enable shared subscription for MQTTv5 #107

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/EspMQTTClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,14 @@ bool EspMQTTClient::mqttTopicMatch(const String &topic1, const String &topic2)
if(topic2.substring(t1a.length(), topic2.length()-t1b.length()).indexOf('/') == -1)
return true;
}
}
else if ((i = topic1.indexOf('$')) >= 0)
{
// shared subscription for MQTTv5
if (topic1.startsWith("$share"))
{
return topic1.endsWith(topic2);
}
Comment on lines +696 to +702
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havnt read about shared subscriptions yet, only looked at the code.

Why is it not directly the second if statement? Why first the indexOf? According to the second if the $ is the first character anyway so startsWith should have better performance then too. Am I missing something here?

Suggested change
else if ((i = topic1.indexOf('$')) >= 0)
{
// shared subscription for MQTTv5
if (topic1.startsWith("$share"))
{
return topic1.endsWith(topic2);
}
else if (topic1.startsWith("$share")) // shared subscription for MQTTv5
{
return topic1.endsWith(topic2);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look a the man page here: https://mosquitto.org/man/mosquitto-8.html. Under "Broker Status", it is possible to have a topic starting with "$SYS".

"Clients can find information about the broker by subscribing to topics in the $SYS hierarchy as follows."

So I only implemented "$share" at this point, as I selfishly only need the round-robin shared subscription for two esp32 to take turns in solving a task.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$SYS is irrelevant here as its unrelated to shared topics?

As far as I understand the shared topics, the topic is always like this:
$share/groupid/topic (example: $share/my-shared-subscriber-group/myhome/groundfloor/+/temperature)

So shared topics always start with "$share". The first if is irrelevant as the second if is both more precise and better performance wise?

Also something I noticed: shared topic subscriptions also support wildcards (+ and #) which this implementation currently isn't. For a first version not a problem but should be noted as a known issue I guess.

Shared topics look interesting, I should look into them more.

}
else
{
Expand Down