search

adafruit / Adafruit_CircuitPython_MiniMQTT

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

loop() timeout parameter should be absolute #169

Closed vladak closed 1 year ago

vladak commented 1 year ago

This changes the way how loop() deals with the timeout parameter. I think the original idea was to wait for the timeout and then return what was received in the mean time, regardless of whether anything was actually received. This means that the loop() will always return after the timeout, i.e. not earlier (assuming no fatal error happened).

Inspired by https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/issues/68#issue-805498443 and the discussion in PR #168.

FoamyGuy commented 1 year ago

I'm doing some testing on https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/pull/167 which is aiming to make the esp32spi socket match the CPython socket behavior as close as possible, that led me to this problem and solution proposed here.

@brentru From my testing I think I can answer these:

  1. Do we need to change any example files to reflect the modifications to loop()
  2. Have you tested this on hardware yet?

1: The examples do not need to change as a result of the change in this PR because there is a default provided for timeout=0 and all of the examples call loop() without passing anything so they'll use this default value, which results in the expected behavior now as far as I understand it.

2: I have tested the change here in #169 successfully in 3 contexts:

The asterisks for ESP32SPI is because my testing was performed on the version of ESP32SPI from https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/pull/167 rather than the currently stock released version.

When using that modified ESP32SPI + The version from this PR the example scripts are working as intended and the blocking (or lack thereof) behavior of loop() appears to work correct to me.

On a somewhat related note, I do think it would be good to tweak the time value here: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/40b90968fb744ebaffce6b09ca27b5a7e747bbff/examples/esp32spi/minimqtt_pub_sub_nonblocking_esp32spi.py#L112 to be a little bigger. During testing I found that after a few dozen or so successful publishes it stops working and seems to fail silently. The publish code is called but the feed on aio doesn't get the new data, and the message callback isn't called.

Im not certain of the root cause, but I started suspecting we were publishing data "too fast" and that was causing it to start failing. I backed that off to a 3 second sleep and it's been running steadily without having the same problem. With regards to question 1 above: this change is not necessarily required, but it does seem good for stability.

brentru commented 1 year ago

1: The examples do not need to change as a result of the change in this PR

EXCELLENT! Thank you for testing.

On a somewhat related note, I do think it would be good to tweak the time value here:

@vladak Are you able to increase the delay value on the line specified by @FoamyGuy? After that, I'd like to go ahead and merge this PR.

vladak commented 1 year ago

This is super awesome if someone comes to the rescue and tests the code in the PR for you ! Thanks @FoamyGuy ! I have to admit I fired off the PR primarily to get the discussion started and skimped on proper testing. That said, I will think about a unit test for this.

vladak commented 1 year ago

Im not certain of the root cause, but I started suspecting we were publishing data "too fast" and that was causing it to start failing. I backed that off to a 3 second sleep and it's been running steadily without having the same problem. With regards to question 1 above: this change is not necessarily required, but it does seem good for stability.

Okay, changed to 3 seconds.

FoamyGuy commented 1 year ago

I'm doing some more testing on this and all the other changes associated with the ESP32SPI socket compatibility and I noticed that the mqtt_client.loop() function was never returning when used with Ethernet Featherwing if there was no data to recieve. It would never timeout successfully which made it effectively blocking until data does get recieved.

I opened: https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k/pull/125 over there to address this and successfully tested the adafruitio ethernet example in this repo with those changes (and the changes from this PR).