Use the '+' wildcard for MQTT rather than '#' #4528
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…opics one nesting level deep
ianmcorvidae
commented
Aug 22, 2024
| @@ -376,12 +376,12 @@ void MQTT::sendSubscriptions() | |||
| const auto &ch = channels.getByIndex(i); | |||
| if (ch.settings.downlink_enabled) { | |||
| hasDownlink = true; | |||
| std::string topic = cryptTopic + channels.getGlobalId(i) + "/#"; | |||
| std::string topic = cryptTopic + channels.getGlobalId(i) + "/+"; | |||
| LOG_INFO("Subscribing to %s\n", topic.c_str()); | |||
| pubSub.subscribe(topic.c_str(), 1); // FIXME, is QOS 1 right? | |||
| #ifndef ARCH_NRF52 // JSON is not supported on nRF52, see issue #2804 | |||
| if (moduleConfig.mqtt.json_enabled == true) { | |||
There was a problem hiding this comment.
Re: my comment about subscribing only to the right channel for MQTT JSON message-sending, I think this is the conditional it'd need to be added to. In my dynamic language homelands I'd add && channels.getGlobalId(i) == "mqtt" but I think that doesn't work here. Probably easy for one of you, though, haha.
geeksville
pushed a commit
to geeksville/Meshtastic-esp32
that referenced
this pull request
Aug 28, 2024
…opics one nesting level deep (meshtastic#4528)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This means we'll then be subscribing only to topics one nesting level deep, rather than any amount of nesting through the end of the topic, as
#does.Discussed briefly with some of y'all firmware devs but I believe this should be fine and limit what messages are getting sent to firmware a little bit. We're not publishing anything deeper than e.g.
msh/2/e/ChannelName/!12345678, meaningmsh/2/e/ChannelName/+will pick that up. Without this change, you could for example set your root topic tomsh/2/e/ChannelName, which would then publish messages tomsh/2/e/ChannelName/2/e/DifferentChannelName/!abcdef01and it would go to clients with justmshconfigured as the prefix and a downlink on theChannelNamechannel.I think that there's another improvement to be made in that we should probably only be subscribing to the json topic if and when the channel name is equal to
mqtt, rather than subscribing to it on every channel and then throwing out everything that's not on themqtttopic. But comparing strings in C++ is a project for a different day, for me :)