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

Enable shared subscription for MQTTv5 #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbench757
Copy link

I've tested this on an ESP32 C3 with mosquitto mqtt broker. This allows shared subscription topic to find the correct callback for processing.

I've tested this on an ESP32 C3 with mosquitto mqtt broker.   This allows shared subscription topic to find the correct callback for processing.
Comment on lines +696 to +702
else if ((i = topic1.indexOf('$')) >= 0)
{
// shared subscription for MQTTv5
if (topic1.startsWith("$share"))
{
return topic1.endsWith(topic2);
}
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.

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

Successfully merging this pull request may close these issues.

2 participants