1technophile / OpenMQTTGateway

MQTT gateway for ESP8266 or ESP32 with bidirectional 433mhz/315mhz/868mhz, Infrared communications, BLE, Bluetooth, beacons detection, mi flora, mi jia, LYWSD02, LYWSD03MMC, Mi Scale, TPMS, BBQ thermometer compatibility & LoRa.
https://docs.openmqttgateway.com
GNU General Public License v3.0
3.55k stars 784 forks source link

Display delay could potentially lead to queue overflow #2023

Closed puterboy closed 2 weeks ago

puterboy commented 2 weeks ago

Devices like the LILYGO LoRa32 display the MQTT message on their internal OLED display. The corresponding code in ZdisplaySSD1306 hard-codes a 2 second delay to allow the message to display.

I believe the current MQTT queue length is set to 18, meaning that when the queue is full it will take at least 36 seconds to empty it due to the display delays. So if you need to send on average more than 1 MQTT every 2 seconds, then the queue will eventually fill and overflow, no matter how large it is. This is not a crazy edge case if you (and your neighbors) have lots of random rtl_433 devices (and at least in my neighborhood, there are lots of them).

It might make sense to have this delay be adaptive as the MQTT queue fills up. Say:

(or any other similar schedule)

Also, is it safe to assume that the code is written so that so long as the queue is not full, then messages are not missed while the display waits?

1technophile commented 2 weeks ago

We made the choice when adding the WebUI and display to keep the messages during a bit long for visibility, without it, it was difficult to read the messages. They were going fast. @NorthernMan54 may have an idea for this one

puterboy commented 2 weeks ago

That's why I proposed my solution as a compromise. Keep the visibility long but shorten it when the queue is filling because then you risk skipping (i.e. losing) messages entirely which is certainly worse case. With my solution plus/minus tuning on the percentages you get the best of both worlds. I

I will code it and try it and share as PR.

On August 25, 2024 4:20:33 PM EDT, Florian @.***> wrote:

We made the choice when adding the WebUI and display to keep the messages during a bit long for visibility, without it, it was difficult to read the messages. They were going fast. @NorthernMan54 may have an idea for this one

-- Reply to this email directly or view it on GitHub: https://github.com/1technophile/OpenMQTTGateway/issues/2023#issuecomment-2308980770 You are receiving this because you authored the thread.

Message ID: @.***>

Sent from my Pixel5a with K-9 Mail

1technophile commented 2 weeks ago

An alternative could be to optimize the screen area usage, but not sure if there is too much room for this

puterboy commented 2 weeks ago

Perhaps a better approach would be to add to-be-displayed messages to a queue that would be checked once per main loop to see if 2 seconds (or whatever interval you select) has elapsed since the last display. If so, the next message is popped from the queue and displayed.

I say this because in general one presumably wants updated MQTT messages to be published as near real-time as possible whereas now if you have multiple sensors sending data, the delay could be several tens of seconds as the device waits idly for 2 seconds.

For example if I am monitoring voltage and current spikes, I may want to see it now, not delayed by an arbitrary amount. If I have a security alarm, I want it now, not in some arbitrary delay amount of time.

In most if not all cases, presumably the display is a secondary feature and it doesn't seem to make sense to have it gate the MQTT publishing which is what you really care about. Conversely, if the display wait is no longer a bottleneck to the main threads, then one can set the display time as long as you want subject to some limit to make sure the queue doesn't get too big...

Also, if the display is no longer a bottleneck, one may be able to remove the enqueueJsonObject mechanism entirely and just MQTT publish all messages as soon as they appear (with the Mutex protection I added in a recent PR to prevent conflicts)

One could make this display queue relatively short, saving some memory since it's less critical that we plan for the worst case to display every possible message.

NorthernMan54 commented 2 weeks ago

@puterboy Take a look at the code behind the webui and ssd1306 module. They are implemented this way.

When we implemented the json message display for WebUI and SSD1306, we had included logic to throw away messages once the queue is full. Thought was that in a high volume situation, like your example, the user wouldn't be using the screen anyway. Personally I think of it as a parlour trick, looks interesting, makes good YouTube videos, but in reality is not a practical method of sharing information.

puterboy commented 2 weeks ago

Ahhh. Great. So the display runs as a loop in a separate thread defined in ZdisplaySSD1306.ino rather than causing everything to wait -- that's perfect! (assuming I am understanding this correctly)

I was though and remain confused by the call in ZgatewayRTL_433.ino topubOled which I only see mentioned as a blank declaration in User_config.h

#define pubOled(...)        // display the published message onto the OLED display

But now I see that the actual display is being done by MQTTtoSSD1306() which is called from receivingMQTT() in main.ino. If I am now understanding this correctly, what is the purpose of the do-nothing pubOled call?

NorthernMan54 commented 2 weeks ago

pubOled is a remnant of the original approach, that has been replaced with the messages going thru the webui, then to ssd1306.

puterboy commented 2 weeks ago

Ahhh makes sense. It's only referenced a couple of times in the entire codebase. May make sense to delete it as it does nothing :)

puterboy commented 2 weeks ago

Should I submit a PR to delete pubOled or do you want to keep it there?

NorthernMan54 commented 2 weeks ago

Your okay to remove it

NorthernMan54 commented 2 weeks ago

Tks for your recent efforts, much appreciated

puterboy commented 2 weeks ago

Your very welcome - least I could do given all the work you guys have put in.