recv_into fix for esp32spi socket implementation

[aaftonbladet]

Resolves https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/issues/148 and https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/issues/165

The esp32spi socket implementations of recv and recv_into do not raise an OSError if the read times out, deviating from the behavior in CPython. In the past we accounted for this by checking whether anything was read when calling recv, if not, raise an OSError ourselves. However, in https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/commit/c334c8157dddc06f67b85b9c299b49d0b29f3031 we started to utilize recv_into rather than recv, if it was available. The new version does not have the "raise OSError ourselves if nothing was read"-workaround, because it was assumed that esp32spi wouldn't have recv_into and the old version would be used.

Ideally, this would be fixed in esp32spi for both functions. However, I think they'd be hesitant to except such a radical change - it'd likely break projects relying on the old implementation. Hence I believe we should take it upon ourselves to fix it.

Maybe it's also appropriate to entirely drop support for socket implementations that lack recv_into. Our alternative implementation utilizing recv is labled "esp32 fix", but esp32spi sockets have had support for recv_into for more than a year. I did not include this change in my PR, but I think it should be considered.

[tekktrik]

After rerunning the CI, it looks like pre-commit wants to reformat the code. You can fix this issue by running pre-commit locally and pushing the changes. You can find information on installing and using pre-commit here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

[brentru]

@vladak As the author of https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/commit/c334c8157dddc06f67b85b9c299b49d0b29f3031, are you able to take a look?

[vladak]

However, in c334c81 we started to utilize recv_into rather than recv, if it was available.

Before that change, recv_into() was already used (in that if branch); my change merely made sure that the return value of recv_into() was used in order to read the full contents (as specified by the bufsize argument). Also, the if condition itself was not changed, merely the contained block of code.

[aaftonbladet]

Before that change, recv_into() was already used [...]

I might've gotten the exact commit mixed up, it could've been https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/commit/d60ab23c375f86a74c7ddaa55018f20c9d5c1563. I tested my way back and forth until I found which version it broke in.

[vladak]

I might've gotten the exact commit mixed up, it could've been d60ab23. I tested my way back and forth until I found which version it broke in.

I think you'd need to dig even further to history. If I remember correctly, the duplicate definition removal did not have any effect, it merely removed dead code.

[alison-gravley]

Is this fixed yet? I am trying to use a Matrix Portal M4 and I can't really update the display if the loop() is blocking all the time, especially if I am not getting messages very often. I messed around with asyncio a little, but it looks like it hangs the whole thing either way.

[FoamyGuy]

@alison-gravley I believe if you are using the absolutely latest version of this library, and the absolute latest version of the ESP32SPI library in conjunction with each other the loop() function is no longer blocking indefinitely under those new versions.

My understanding is that the changes from this PR won't necessarily be needed, because we instead made changes inside of ESP32SPI to bring it's behavior more inline with CPython sockets. So this could pototentially be closed at this point unless I've misunderstood some part of it.

I cannot speak to asyncio support, I did a fair amount of testing when preparing to merge / release the latest changes across many different devices and use-cases, but asyncio is something I didn't touch at all during testing, I'm not sure how it interacts with loop() or any other functionality.

[alison-gravley]

@alison-gravley I believe if you are using the absolutely latest version of this library, and the absolute latest version of the ESP32SPI library in conjunction with each other the loop() function is no longer blocking indefinitely under those new versions.

My understanding is that the changes from this PR won't necessarily be needed, because we instead made changes inside of ESP32SPI to bring it's behavior more inline with CPython sockets. So this could pototentially be closed at this point unless I've misunderstood some part of it.

I cannot speak to asyncio support, I did a fair amount of testing when preparing to merge / release the latest changes across many different devices and use-cases, but asyncio is something I didn't touch at all during testing, I'm not sure how it interacts with loop() or any other functionality.

I should be on latest release of CircuitPython, 8.2.0. Do I need to change to a bleeding edge? I am not sure where I find the version of esp32spi.

From lib folder: adafruit_minmqtt v7.4.0

boot_out.txt says: Adafruit CircuitPython 8.2.0 on 2023-07-05; Adafruit Matrix Portal M4 with samd51j19 Board ID:matrixportal_m4

info_uf2.txt: UF2 Bootloader v3.15.0 SFHWRO Model: Matrix Portal M4 Board-ID: SAMD51J19A-MatrixPortal-v0

[FoamyGuy]

@alison-gravley Ahh, I forgot the detail that esp32spi is frozen into the build. I think there has not been an update in the core to bump to the newest version so the frozen in one is still on the prior release. Which does make it a little bit trickier, but still do-able.

For now the easiest way to get it working is get the latest release of esp32spi from the assets downloads on this release page: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/releases/tag/6.0.0 get the 8.x mpy zip, download and extract it. Inside find the lib folder, and inside of that should be the adafruit_esp32spi/ directory. Copy that directory to the root of your CIRCUITPY drive (unlike normally when you'd put it inside of lib this needs to be specifically in the root directory to "override" the frozen version from the core build).

So your drive should look something like this:

CIRCUITPY/
    adafruit_esp32spi/
    lib/
        adafruit_minimqtt/
    boot_out.txt
    code.py

Putting the latest esp32spi in the root of CIRCUITPY will make it so that this version gets used instead of the one frozen in. At some point the core one will get updated to the latest and then this process will no longer be necessary, but I think for now it is.

[alison-gravley]

@FoamyGuy, thank you so much! I finally have scrolling text and can move on to fun animations.

I actually tried all of this in Arduino first, but ran into a ton of issues between adafruit libraries and changes with esp32 breaking things. I had to do some hacky things to even get the project to compile and run, but I couldn't quite get it to subscribe to the broker without getting an error.

Off-topic here, but I keep hitting a lot of bad luck on when I dig these esp32-s2/s3 boards out. Every time I think it is fun to play with such and such library I run in to a problem and find a recent GitHub post explaining the issue. With both Circuit Python and Arduino. I am just thankful all of the developers have been great and I appreciate all of you for helping your end users who have a mish-mash of capabilities and experience levels.

[toddkrein]

Well, three days down the rabbit hole after being bitten by this on my PyPortal. At least it was educational. Pulling the latest esp32spi and putting it at the top level of CircuitPython has fixed the problem. What's the next step to encourage pulling the updated esp32spi into the CircuitPython release itself?

update: 8.2.3 still doesn't have the updated esp32spi library

[dwadler]

This was painful. After moving my esp32-s2 MQTT app to the PYPortal, I spent hours pulling my hair trying to figure out why loop() was hanging, trying the published AdaFruit examples which hung similarly. I finally found this issue and thanks to the suggested by "Foamguy" above, got it to work. (I was about to start poking around in the mini_mqtt source). Isn't this going to be broken for anyone trying to run the samples on PYPortal with the current code levels?

[toddkrein]

This was painful. After moving my esp32-s2 MQTT app to the PYPortal, I spent hours pulling my hair trying to figure out why loop() was hanging, trying the published AdaFruit examples which hung similarly. I finally found this issue and thanks to the suggested by "Foamguy" above, got it to work. (I was about to start poking around in the mini_mqtt source). Isn't this going to be broken for anyone trying to run the samples on PYPortal with the current code levels?

Wow, you solved in hours... Better than my several days. Yes, it will be an issues for everyone using this level.

[justmobilize]

@aaftonbladet and @dhalbert - I might recommend closing this, and if the esp32spi should raise on a condition like this, a PR should be added there so we don't have special case code in this library

[FoamyGuy]

Closing this for now. I believe it should not be an issue any more. I think the frozen in versions were updated a while ago to support the new way of working.

If anyone else runs into this on newer versions we can re-open and dig further again.