Accept wildcards in mqtt subscription prefix #1497
Conversation
mfalkvidd
requested review from
tekka007,
henrikekblad,
marceloaqno,
scalz,
tbowmo,
user2684,
Yveaux and
a team
| message.setSender(GATEWAY_ADDRESS); | ||
| message.setLast(GATEWAY_ADDRESS); | ||
| message.setEcho(false); | ||
| for (str = strtok_r(topic + strlen(MY_MQTT_SUBSCRIBE_TOPIC_PREFIX) + 1, "/", &p); | ||
|
|
||
| // The subscription prefix can contain wildcards, but only the + wildcard. |
There was a problem hiding this comment.
Why is only the single-level + wildcard supported?
If we add support for wildcards, users will expect to be able to use multi-level # wildcards too.
If you parse right-to-left for the mysensors message field you could support any wildcard.
There was a problem hiding this comment.
The MQTT multi-level wildcard (#) is only allowed at the end of the topic. Because the prefix is always followed by other levels it cannot contain the # wildcard, although "my" code wouldn't complain about it.
The code only checks the number of / characters.
The syntactical check is left to the MQTT broker. That was already the case and I think that is right. I have browsed the MySensors code and checked all code where the PREFIX is used and none of needed a change.
core/MyProtocol.cpp
Outdated
| if (!strPrefix) { | ||
| index++; | ||
| } | ||
| //GATEWAY_DEBUG(PSTR("GWT:TPS:Index=%d, mqttstr=%s, prefixtoken=%s\n"), index, str,strPrefix); |
| // Make a copy of the subscribe prefix because strtok is allowed to change the string. | ||
| char subscribeTopicPrefix[strlen(MY_MQTT_SUBSCRIBE_TOPIC_PREFIX) + 1]; | ||
| strcpy(subscribeTopicPrefix,MY_MQTT_SUBSCRIBE_TOPIC_PREFIX); | ||
| char *strPrefix, *savePointerPrefix; |
There was a problem hiding this comment.
What's the increase in ram/flash for this change?
There was a problem hiding this comment.
The savepointer (4 bytes?) is stored on stack I guess.
The increase in ram (stack use?) for the copy of the prefix string will be the length of the prefix plus 1.
The default value for the prefix is mygateway1-in, length is 13, so it will use 14 bytes on the stack. Your comment made me think about this memory usage. Actually I think malloc/free would be safer. Or maybe choose in runtime between stack and malloc depending on the prefix length? The length of the string is known at compile time, but I don't think that will help. I am not an expert in this subject.
Flash use? Is that the code length? The answer is: I don't know.
I compiled and ran it only on a raspberry pi 2, not on an Arduino.
There was a problem hiding this comment.
In core/MyGatewayTransportMQTTClient.cpp, method reconnectMQTT, there is also a local variable inTopic with a length of the prefix plus 10. So I think it is acceptable to have it as a stack variable.
No description provided.