Skip to content

Prevent unexpected reboots due to race condition in MQTT timeout handling #3

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

Open
SevenW wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prevent unexpected reboots due to race condition in MQTT timeout handling #3

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fe7af9
solve unexpected restarts of ESP due to race condition in MQTT ping t…
SevenW Nov 30, 2020
f52dfc8
solve unexpected restarts of ESP due to race condition in MQTT ping t…
SevenW Nov 30, 2020
038a515
solve unexpected restarts of ESP due to race condition in MQTT ping t…
SevenW Nov 30, 2020
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
26 changes: 17 additions & 9 deletions src/mqtt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ int mqTopicLen = 0; // strlen(mqTopic)

// keep-alive stuff
#define MQ_TIMEOUT (60*1000) // in milliseconds
static uint32_t mqLast = 0x100000; // when we last received something
static uint32_t mqPing = 0; // when we last sent a ping
static uint32_t mqPingRx = 0; // when we last received a ping response
uint32_t mqPingMs = 0; // timeing of last ping
static volatile uint32_t mqLast = 0x100000; // when we last received something
static volatile uint32_t mqPing = 0; // when we last sent a ping
static volatile uint32_t mqPingRx = 0; // when we last received a ping response
volatile uint32_t mqPingMs = 0; // timeing of last ping

// helper to subscribe to our own pings
static void mqttSubPing() {
Expand Down Expand Up @@ -101,21 +101,29 @@ void mqttConnect() {
}

void mqttLoop() {
if (millis() - mqPingRx > 20*MQ_TIMEOUT) {
printf("*** No MQTT response in %d seconds - resetting\n", (millis()-mqPingRx)/1000);
uint32_t cachemqPingRx = mqPingRx;
Copy link
Author

@SevenW SevenW Nov 30, 2020

Choose a reason for hiding this comment

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

The volatile obviously should be used, but did not solve the problem by itself. The additional variable did the trick.

I also tried swapping the millis() and the MqPingRX in the test, but that did not solve it.

if (millis() - cachemqPingRx > 2 * MQ_TIMEOUT)
{
printf("*** No MQTT response in %lu seconds - resetting\n", (millis() - cachemqPingRx) / 1000);
ESP.restart();
}
if (!WiFi.isConnected()) return;
uint32_t cachemqLast = mqLast;
if (!mqttClient.connected()) {
if (millis() - mqLast > 10000) {
if (millis() - cachemqLast > 10000)
{
mqttConnect();
mqLast = millis();
}
} else if (millis() - mqLast > MQ_TIMEOUT) {
}
else if (millis() - cachemqLast > MQ_TIMEOUT)
{
//printf("Reconnecting to MQTT\n");
mqttConnect();
mqLast = millis();
} else if (millis() - mqLast > MQ_TIMEOUT/2 && millis() - mqPing > MQ_TIMEOUT/2) {
}
else if (millis() - cachemqLast > MQ_TIMEOUT / 2 && millis() - mqPing > MQ_TIMEOUT / 2)
{
char topic[41+6];
strcpy(topic, mqTopic);
strcat(topic, "/ping");
Expand Down