search

adafruit / Adafruit_CircuitPython_MiniMQTT

MQTT Client Library for CircuitPython
Other
80 stars 49 forks source link

Fetch all messages in loop, non-blocking mode #122

Closed calcut closed 1 year ago

calcut commented 2 years ago

First PR, so bear with me!

Basically, I've found issues with callling loop() too frequently or too infrequently, that I'm trying to address here. Calling too frequently (with timeout >0) can lead to a nasty MemoryError. https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/issues/101 Calling too infrequently can cause a build up of messages on the broker. https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/issues/108

I've changed the default timeout to 0 (and added error handling of EAGAIN) This might be a controversial change - but it means the function is really non-blocking as the comments say it should be. (Perhaps there is a history of why it was set to 1 by default?, I'd be happy keeping it at 1 if it prevents issues for others)

I've also made it call _wait_for_msg() in a while loop, similar to how the ping() function works. I've used the 10s _recv_timeout to make sure it can't get stuck in the loop. This means that all waiting messages are received, not just the next in the queue.

tekktrik commented 2 years ago

Do you have any code that can reproduce the issues you listed? I'm going to try to review this and didn't know if you had something like that already.

calcut commented 2 years ago

There is code in the 2 issues I linked. The MemoryError may be harder to reproduce (e.g. if it depends on connection latency etc).

I can try to help further if it still isn't clear

FoamyGuy commented 2 years ago

I'm not super familiar with the specifics of MQTT so I can't comment much on the details of the change. I did try it out on a device though.

Using code posted in the issue on a Feather ESP32-S2 8.0.0beta.1.

Can confirm I observed the same issue with currently released libraries, mqtt time "falling behind" due to only getting one callback per loop call and request getting out of retries problem. Eventually it "catches up" with a bunch of callbacks at once (in my case it was dozens, I didn't count exactly)

The new version of this library from the PR does seem to resolve it. With this version I am getting fairly consistently 5 mqtt callbacks per loop with the final one matching the time given by the http request.

I inclined to approve, but we should probably try out a few of the learn guide projects that use minimqtt to ensure the differing functionality with callbacks does not have adverse effects in them. Assuming no issues with them, I think this is good.

calcut commented 2 years ago

Regarding the "catching up" I figured out that happens because of the keep_alive feature. where it runs the ping() function periodically. The ping function ends up fetching all messages in a loop. So my change is to make the loop() function behave the same way