Provide ability to send a status msg via MQTT #14

mattbnz · 2022-09-02T04:48:37Z

Currently used for a couple of basic purposes:

to send count of connection attempts on connection (somewhat useful
for determining if a connection is a reboot/physical issue or
network).
Log start of OTA updates.

Neither are super-important, mostly just to demo/test the functionality

more concretely, I plan to follow this up for more completed OTA
status reporting (e.g. error cases), but that requires
Reporting status during the update process chrisjoyce911/esp32FOTA#87 to be addressed
first. Outside of OTA status, I have a few other features planned that
would also benefit from being able to report this type of status back.

Currently used for a couple of basic purposes: 1) to send count of connection attempts on connection (somewhat useful for determining if a connection is a reboot/physical issue or network). 2) Log start of OTA updates. Neither are super-important, mostly just to demo/test the functionality - more concretely, I plan to follow this up for more completed OTA status reporting (e.g. error cases), but that requires chrisjoyce911/esp32FOTA#87 to be addressed first. Outside of OTA status, I have a few other features planned that would also benefit from being able to report this type of status back.

oseiler2 · 2022-09-05T04:25:20Z

src/mqtt.cpp

+  void sendStatus(const char *newmsg) {
+    MqttMessage msg;
+    msg.cmd = X_CMD_PUBLISH_STATUSMSG;
+    sprintf(statusmsg, "%s", newmsg);


There's a rare chance that a sendStatusInteral call is in progress while a sentStatus call is made from another thread which then overwrites/corrupts the statusmsg. Can we avoid that e.g. by passing the status string (or a constant that gets resolved to a string) with the message? How long do you anticipate the statue message to get?

Not sure if there is a much better solution, and if its worth increasing the queue message size which would reserve a larger chunk of memory. If we could keep the status message short then adding it to the queue message could work, otherwise not so sure.

I originally was storing the string in the message, but couldn't make it work through the queue, so fell back to the global object, but you're right it's messy... I took another look and found the bug that was breaking the queue passing (wrong size being passed to the createQueue method).

Just wondering - wouldn't the same approach of sending a pointer to the JsonDocument have worked for a String pointer as well as long as the publishStatusInternal method deleted it to free the memory?
I do like the idea of having an option to publish structured data, but it feels a little over kill for publishing a simple status message. What do you think, and do you have future changes that would make use of a generic publish for structured data

oseiler2 · 2022-09-05T04:28:45Z

src/mqtt.cpp

@@ -269,8 +302,8 @@ namespace mqtt {
        mqtt_client->subscribe(buf);
        sprintf(buf, "%s/down/#", config.mqttTopic);
        mqtt_client->subscribe(buf);
-        sprintf(buf, "%s/%u/up/status", config.mqttTopic, config.deviceId);
-        mqtt_client->publish(buf, "{\"online\":true}");
+        sprintf(statusmsg, "connected after %d attempts", connectionAttempts);


Do you mind passing the reconnect count as a separate property instead of injecting it into the string?
I've got a mqtt_client->publish(buf, "{\"online\":false}"); message in the (so far unpublished) branch for the portable monitor to signal the device going to disconnect before sleep.

OK, see what you think about this reworked code... allows an arbitrary JSON doc to be provided as the status message (as a pointer, which the queue takes ownership of to free after serialization).

This allows sending structured data (like offline:false, and connectionAttempts) but still makes it pretty simple to provide a nice wrapper for sending a simple message.

Only 4 more bytes on the queue in the general case (e.g. other messages), but when providing a doc, the caller has control over how much memory to allocate.

xQueueCreate takes the size of the item to store on the queue, which is used by xQueueSendToBack to copy that number of bytes from the provided pointer at queing time. So this should be sizeof(MqttMessage) not the size of a pointer to MqttMessage (we can't store a pointer, because the queued object is on the stack, not the heap)... Despite the bug, this currently works because sizeof(MqttMessage) == sizeof(MqttMessgae *) atm - both are 4 bytes. But that won't be true if/when more fields are added... (in the next commit).

With a helper to enable the simple usecase of just publishing a status string (OTA use-case), but also allowing other use-cases (e.g. reconnection) to send structured data rather than embedding # of reconnects in a string. This also avoids a race condition/potential loss of data if sendStatus is called consecutively before the queue is processed.

oseiler2

Good find (oops) sizeof(struct MqttMessage*)

src/mqtt.cpp

oseiler2 · 2022-09-08T06:15:57Z

src/mqtt.cpp

+  void sendStatus(const char *newmsg) {
+    MqttMessage msg;
+    msg.cmd = X_CMD_PUBLISH_STATUSMSG;
+    sprintf(statusmsg, "%s", newmsg);


Just wondering - wouldn't the same approach of sending a pointer to the JsonDocument have worked for a String pointer as well as long as the publishStatusInternal method deleted it to free the memory?
I do like the idea of having an option to publish structured data, but it feels a little over kill for publishing a simple status message. What do you think, and do you have future changes that would make use of a generic publish for structured data

mattbnz · 2022-09-08T08:59:42Z

(can't reply directly on your comment above since I pushed the latest commit... weird)

Just wondering - wouldn't the same approach of sending a pointer to the JsonDocument have worked for a String pointer as well as long as the publishStatusInternal method deleted it to free the memory?

Yes, the issue was simply the size argument, so we could pass whatever we like.

I do like the idea of having an option to publish structured data, but it feels a little over kill for publishing a simple status message. What do you think, and do you have future changes that would make use of a generic publish for structured data

I don't have any other use-cases (other than the reconnection count that uses it here, which I do want) for structured data other than a string message immediately in mind.

I don't feel strongly about reverting back to only a string, but then we have to put the reconnection count back into a string, etc.

mattbnz · 2022-09-19T22:53:23Z

aeaec2a doesn't quite preserve the same semantics that were intended here - specifically it doesn't take ownership of the passed status message and does not free it after it is sent, so you're options are either to:

Cause a race condition - (as demonstrated in the csr branch - e.g. lines 51-62 of certs.cpp) - if the free runs before the mqtt queue is processed (likely), publishStatusMsgInternal is going to be called on a pointer to memory that has been already freed.
Leak memory - e.g. don't call free, and let it leak
Only every pass static strings that don't need freeing.

None of those are very attractive... I tink given the asynchronous nature of the queue, there isn't an alternative for this API other than for it to take ownership of the string and free it itself.

oseiler2 reviewed Sep 5, 2022

View reviewed changes

mattbnz added 2 commits September 6, 2022 13:09

oseiler2 reviewed Sep 8, 2022

View reviewed changes

oseiler2 requested changes Sep 8, 2022

View reviewed changes

Fix missing deletes on error paths.

010f631

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide ability to send a status msg via MQTT #14

Provide ability to send a status msg via MQTT #14

mattbnz commented Sep 2, 2022

oseiler2 Sep 5, 2022

oseiler2 Sep 5, 2022

mattbnz Sep 6, 2022

oseiler2 Sep 8, 2022

oseiler2 Sep 5, 2022

mattbnz Sep 6, 2022

oseiler2 left a comment •

edited

Loading

oseiler2 Sep 8, 2022

mattbnz commented Sep 8, 2022

mattbnz commented Sep 19, 2022

Provide ability to send a status msg via MQTT #14

Are you sure you want to change the base?

Provide ability to send a status msg via MQTT #14

Conversation

mattbnz commented Sep 2, 2022

oseiler2 Sep 5, 2022

Choose a reason for hiding this comment

oseiler2 Sep 5, 2022

Choose a reason for hiding this comment

mattbnz Sep 6, 2022

Choose a reason for hiding this comment

oseiler2 Sep 8, 2022

Choose a reason for hiding this comment

oseiler2 Sep 5, 2022

Choose a reason for hiding this comment

mattbnz Sep 6, 2022

Choose a reason for hiding this comment

oseiler2 left a comment • edited Loading

Choose a reason for hiding this comment

oseiler2 Sep 8, 2022

Choose a reason for hiding this comment

mattbnz commented Sep 8, 2022

mattbnz commented Sep 19, 2022

oseiler2 left a comment •

edited

Loading