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

FIX: "magic numbers" for MQTT topic buffer length in mqtt.cpp and button.cpp (use already defined MQTT_MAX_TOPIC_LEN) #4295

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

FIX: "magic numbers" for MQTT topic buffer length in mqtt.cpp and button.cpp (use already defined MQTT_MAX_TOPIC_LEN) #4295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0160e3f
Use MQTT_MAX_TOPIC_LEN in places where it was not used before to avoi…
WouterGritter Nov 20, 2024
cec8978
Fix comment alignment
WouterGritter Nov 20, 2024
0db47a8
Add comment warning about modification of MQTT_MAX_TOPIC_LEN
WouterGritter Nov 21, 2024
8f8afd9
Replace comment with compile-time error and warning
WouterGritter Nov 21, 2024
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: 4 additions & 4 deletions wled00/button.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ void shortPressAction(uint8_t b)
#ifndef WLED_DISABLE_MQTT
// publish MQTT message
if (buttonPublishMqtt && WLED_MQTT_CONNECTED) {
char subuf[64];
char subuf[MQTT_MAX_TOPIC_LEN + 32];
WouterGritter marked this conversation as resolved.
Show resolved Hide resolved
sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b);
mqtt->publish(subuf, 0, false, "short");
}
Expand Down Expand Up @@ -62,7 +62,7 @@ void longPressAction(uint8_t b)
#ifndef WLED_DISABLE_MQTT
// publish MQTT message
if (buttonPublishMqtt && WLED_MQTT_CONNECTED) {
char subuf[64];
char subuf[MQTT_MAX_TOPIC_LEN + 32];
WouterGritter marked this conversation as resolved.
Show resolved Hide resolved
sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b);
mqtt->publish(subuf, 0, false, "long");
}
Expand All @@ -83,7 +83,7 @@ void doublePressAction(uint8_t b)
#ifndef WLED_DISABLE_MQTT
// publish MQTT message
if (buttonPublishMqtt && WLED_MQTT_CONNECTED) {
char subuf[64];
char subuf[MQTT_MAX_TOPIC_LEN + 32];
WouterGritter marked this conversation as resolved.
Show resolved Hide resolved
sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b);
mqtt->publish(subuf, 0, false, "double");
}
Expand Down Expand Up @@ -151,7 +151,7 @@ void handleSwitch(uint8_t b)
#ifndef WLED_DISABLE_MQTT
// publish MQTT message
if (buttonPublishMqtt && WLED_MQTT_CONNECTED) {
char subuf[64];
char subuf[MQTT_MAX_TOPIC_LEN + 32];
WouterGritter marked this conversation as resolved.
Show resolved Hide resolved
if (buttonType[b] == BTN_TYPE_PIR_SENSOR) sprintf_P(subuf, PSTR("%s/motion/%d"), mqttDeviceTopic, (int)b);
else sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b);
mqtt->publish(subuf, 0, false, !buttonPressedBefore[b] ? "off" : "on");
Expand Down
26 changes: 15 additions & 11 deletions wled00/mqtt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
#ifndef WLED_DISABLE_MQTT
#define MQTT_KEEP_ALIVE_TIME 60 // contact the MQTT broker every 60 seconds

#if MQTT_MAX_TOPIC_LEN > 32
#warning "MQTT topics length > 32 is not recommended for compatibility with usermods!"
#endif

static void parseMQTTBriPayload(char* payload)
{
if (strstr(payload, "ON") || strstr(payload, "on") || strstr(payload, "true")) {bri = briLast; stateUpdated(CALL_MODE_DIRECT_CHANGE);}
Expand All @@ -23,24 +27,24 @@ static void parseMQTTBriPayload(char* payload)
static void onMqttConnect(bool sessionPresent)
{
//(re)subscribe to required topics
char subuf[38];
char subuf[MQTT_MAX_TOPIC_LEN + 6];

if (mqttDeviceTopic[0] != 0) {
strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
mqtt->subscribe(subuf, 0);
strcat_P(subuf, PSTR("/col"));
mqtt->subscribe(subuf, 0);
strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/api"));
mqtt->subscribe(subuf, 0);
}

if (mqttGroupTopic[0] != 0) {
strlcpy(subuf, mqttGroupTopic, 33);
strlcpy(subuf, mqttGroupTopic, MQTT_MAX_TOPIC_LEN + 1);
mqtt->subscribe(subuf, 0);
strcat_P(subuf, PSTR("/col"));
mqtt->subscribe(subuf, 0);
strlcpy(subuf, mqttGroupTopic, 33);
strlcpy(subuf, mqttGroupTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/api"));
mqtt->subscribe(subuf, 0);
}
Expand Down Expand Up @@ -158,27 +162,27 @@ void publishMqtt()

#ifndef USERMOD_SMARTNEST
char s[10];
char subuf[48];
char subuf[MQTT_MAX_TOPIC_LEN + 16];

sprintf_P(s, PSTR("%u"), bri);
strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/g"));
mqtt->publish(subuf, 0, retainMqttMsg, s); // optionally retain message (#2263)

sprintf_P(s, PSTR("#%06X"), (col[3] << 24) | (col[0] << 16) | (col[1] << 8) | (col[2]));
strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/c"));
mqtt->publish(subuf, 0, retainMqttMsg, s); // optionally retain message (#2263)

strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/status"));
mqtt->publish(subuf, 0, true, "online"); // retain message for a LWT

// TODO: use a DynamicBufferList. Requires a list-read-capable MQTT client API.
DynamicBuffer buf(1024);
bufferPrint pbuf(buf.data(), buf.size());
XML_response(pbuf);
strlcpy(subuf, mqttDeviceTopic, 33);
strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(subuf, PSTR("/v"));
mqtt->publish(subuf, 0, retainMqttMsg, buf.data(), pbuf.size()); // optionally retain message (#2263)
#endif
Expand Down Expand Up @@ -211,7 +215,7 @@ bool initMqtt()
if (mqttUser[0] && mqttPass[0]) mqtt->setCredentials(mqttUser, mqttPass);

#ifndef USERMOD_SMARTNEST
strlcpy(mqttStatusTopic, mqttDeviceTopic, 33);
strlcpy(mqttStatusTopic, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1);
strcat_P(mqttStatusTopic, PSTR("/status"));
mqtt->setWill(mqttStatusTopic, 0, true, "offline"); // LWT message
#endif
Expand Down
8 changes: 4 additions & 4 deletions wled00/wled.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,10 @@ WLED_GLOBAL unsigned long lastMqttReconnectAttempt _INIT(0); // used for other
#endif
WLED_GLOBAL AsyncMqttClient *mqtt _INIT(NULL);
WLED_GLOBAL bool mqttEnabled _INIT(false);
WLED_GLOBAL char mqttStatusTopic[40] _INIT(""); // this must be global because of async handlers
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac)
WLED_GLOBAL char mqttGroupTopic[MQTT_MAX_TOPIC_LEN+1] _INIT("wled/all"); // second MQTT topic (for example to group devices)
WLED_GLOBAL char mqttServer[MQTT_MAX_SERVER_LEN+1] _INIT(""); // both domains and IPs should work (no SSL)
WLED_GLOBAL char mqttStatusTopic[MQTT_MAX_TOPIC_LEN + 8] _INIT(""); // this must be global because of async handlers
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN + 1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac)
WLED_GLOBAL char mqttGroupTopic[MQTT_MAX_TOPIC_LEN + 1] _INIT("wled/all"); // second MQTT topic (for example to group devices)
WLED_GLOBAL char mqttServer[MQTT_MAX_SERVER_LEN + 1] _INIT(""); // both domains and IPs should work (no SSL)
WLED_GLOBAL char mqttUser[41] _INIT(""); // optional: username for MQTT auth
WLED_GLOBAL char mqttPass[65] _INIT(""); // optional: password for MQTT auth
WLED_GLOBAL char mqttClientID[41] _INIT(""); // override the client ID
Expand Down
4 changes: 4 additions & 0 deletions wled00/wled_eeprom.cpp
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
#include <EEPROM.h>
#include "wled.h"

#if defined(WLED_ENABLE_MQTT) && MQTT_MAX_TOPIC_LEN < 32
#error "MQTT topics length < 32 is not supported by the EEPROM module!"
#endif

/*
* DEPRECATED, do not use for new settings
* Only used to restore config from pre-0.11 installations using the deEEP() methods
Expand Down