adafruit / Adafruit_CircuitPython_ESP32SPI

ESP32 as wifi with SPI interface
MIT License
103 stars 75 forks source link

Compatible socket #167

Closed dhalbert closed 1 year ago

dhalbert commented 2 years ago

This is meant to be merged after #166. It changes the API, so it will be a major version change.

The idea here is to stop making this being a standalone library for doing network code with ESP32SPI. Instead, it is an implementation library to be used by other code such as adafruit_requests. Its socket interface should be compatible with other socket implementations.

FoamyGuy commented 2 years ago

I tested this version out with some of the basic examples from this repo and the requests repo and everything worked as expected.

I tried this cleveland art project: https://learn.adafruit.com/cleveland-museum-of-art-pyportal-frame/overview and am running into trouble with this version of the library though.

The images are getting "jumbled up when downloaded"

image

Sometimes the colors are jumbled others times it seems like chunks of the image have been duplicated or moved the incorrect location.

I tried running the same project using the currently released version of this library and it seems to always get the images downloaded correctly but the modified one always has them jumbled up like this.

The visual artifacts seem to always include horizontal lines. It looks to me like the download is occurring in chunks but there order of the chunks is not getting re-assembled correct in the same order that it came from the server. This is a guess based on the visual artifacts though I have not inspected the traffic or specific data in the file that gets downloaded.

FoamyGuy commented 2 years ago

Here is another example of an image it downloaded using this version: image

The stripes and "stair-stepping" pattern seems to occur commonly, but sometimes the size of the horizontal row height is different

Here is one more with the narrower row height and color disruption: image

dhalbert commented 2 years ago

I tried running the same project using the currently released version of this library and it seems to always get the images downloaded correctly but the modified one always has them jumbled up like this.

I am confused by this because the API difference between 4.2.2 and this should be minimal. Is it 4.2.2 that works correctly?

FoamyGuy commented 2 years ago

I'm not certain of the version that worked correctly. It would be whatever was frozen in to this build. I'll check more specifically and report back the versions.

dhalbert commented 2 years ago

It would be whatever was frozen in to this build.

That would be older than 4.2.2. I haven't updated the frozen libraries that recently. 4.2.2 is only five days old right now.

FoamyGuy commented 2 years ago

I tested 4.2.2 by putting it in the root of CIRCUITPY and it does seem to download the images correctly.

dhalbert commented 2 years ago

The assembly of chunked requests is done by adafruit_requests, so now I'm pretty confused, hmm.

FoamyGuy commented 2 years ago

I retested both using the current adafruit_requests from today's bundle. Unsure which version I had previously, but behavior is still the same now this branch gets that offset effect and 4.2.2 gets accurate image download.

FoamyGuy commented 1 year ago

Okay, I think I got this figured out now.

https://github.com/adafruit/Adafruit_CircuitPython_Requests/pull/135 is the change that goes along with this in order to get the wget downloads working successfully

As far as I can tell this behavior is what the backwards comatibility was referring to. I noticed that ESP32SPI had _backwards_compatibility True, and builtin ESP32-S2 had it False.

With one tweak to the FakeSSLSocket to bind recv_into and then removing the backwards compatibility behavior it allows the version in this PR to successfully download the file with wget and get the correct version.

I merged main into this branch and resolved the conflicts.

Both this and the requests change could use some more thorough testing, thus far I've only been testing with the reproducer code above. I'll try to complete further testing in the next few days.

FoamyGuy commented 1 year ago

I added a commit to change another aspect of the behavior to match CPython: Make recv_into raise a TimeoutError if the timeout time has elapsed. Also store the 'custom' TimeoutError exception type in timeout property at the root level so that the code here will recognize it: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/40b90968fb744ebaffce6b09ca27b5a7e747bbff/adafruit_minimqtt/adafruit_minimqtt.py#L1028-L1032

I confirmed this behavior matches CPython using these two CPython scripts:

# echo-server.py

import socket

HOST = "127.0.0.1"  # Standard loopback interface address (localhost)
PORT = 65432  # Port to listen on (non-privileged ports are > 1023)

with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s.bind((HOST, PORT))
    s.listen()
    conn, addr = s.accept()
    with conn:
        print(f"Connected by {addr}")
        while True:
            data = conn.recv(1024)
            if not data:
                break
            conn.sendall(data)

and

import socket

HOST = "127.0.0.1"  # The server's hostname or IP address
PORT = 65432  # The port used by the server

with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s.settimeout(1)
    print(s.timeout)
    s.connect((HOST, PORT))

    # ... don't send anything...

    buf = bytearray(1024)
    s.recv_into(buf, 1024)

print(f"Received {buf!r}")

Running them results in:

Traceback (most recent call last):
  File "echo_client.py", line 13, in <module>
    s.recv_into(buf, 1024)
TimeoutError: timed out
FoamyGuy commented 1 year ago

I have completed more thorough testing of requests and MiniMQTT with the latest version and thus far everything appears to be working successfully.

I noticed that https://github.com/adafruit/Adafruit_CircuitPython_WSGI was trying to use adafruit_esp32spi_wsgiserver.py from here which has been removed with this PR since it was using the non-standard behavior. That means that the WSGI library would not work anymore unless re-written if this PR gets merged.

We also found last week that the HTTPServer library cannot be used with this version of ESP32SPI because it's wanting to use the bind() function which it turns out is not exposed by the underlying firmware that is in the airlift processor.

If I understand correctly it means that this would leave us without a way to do any sort of simple http server with ESP32SPI. I think it would be great if we can somehow retain that functionality. Perhaps it could be re-worked so that the non-standard behavior can reside outside of the socket or maybe everything needed could be moved into the WSGI library so that it encompasses everything needed to make the server work, but the socket and things here can still be changed to have standard behavior.