Compatible socket

[dhalbert]

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

• Include fix recv_into for larger fetches #166 changes: socket.recv_into() fixes.
• Fix one small documentation error.
• Remove deprecated and non-standard methods from socket: read(), write(), readline().
• Make other non-standard methods be internal, named with a preceding underscore.
• Remove recv reading as as much as possible when bufsize=0. This is not part of CPython socket.recv().
• Remove WSGI server code and example. This should be done on top of adafruit_requests, using https://github.com/adafruit/Adafruit_CircuitPython_WSGI or similar. I checked the Learn Guide repo, and there is no use of this. The WSGI server is the only code I found that uses the deprecated and non-standard socket methods.

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]

Would it make sense to move these examples over to https://github.com/adafruit/Adafruit_CircuitPython_WSGI/tree/main/examples and modify if needed to work with that library?

I'll test out this version of the library using the examples and a few of the basic projects from learn.

[dhalbert]

We could do that, but I think that library could use some work as well. I don't see it as that high a priority. I don't think the old WSGI library here is used much.

I would be most interested in a subset of some standard or semi-standard CPython WSGI library, in the long run.

[FoamyGuy]

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"

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]

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

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:

[dhalbert]

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]

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]

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]

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

[dhalbert]

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

[FoamyGuy]

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]

I made version that reverts this change for the special bufsize=0 behavior and was able to get an accurate image downloaded with that version.

Perhaps something else is relying on this differing behavior somehow.

[dhalbert]

That would make sense, though it's unfortunate, because the "read as much as possible" behavior is not what CPython does, assuming I am interpreting the doc correctly.

[tekktrik]

Revisiting this, could we make socket the subset of CPython (which my understanding is the same as @dhalbert's that 0 would not be "read all"), and just have dependents have functionality for reading all? Basically, move this up so that PortalBase would handle reading all available.

[FoamyGuy]

@tekktrik I do think it should be possible to change the behavior in portalbase so that it can have the standard socket behavior here.

[FoamyGuy]

In my prior comment I had assumed that the if bufsize == 0: logic was responsible for the difference in behavior.

But I've been digging back into this now and I have found that this bit here is likely the part that is resulting in the difference in behavior leading those downloads to come out "stair step chunky".

I have noticed that with the currently released socket behavior we occasionally return less than the chunk_size from iter_content and in the iterations where that occurs it's because this logic is causing it to break.

But with the socket behavior from this PR, it seems that we always return exactly the chunk size. We never have those iterations that return a smaller than chunk_size amount.

I am thinking that in those iterations we're reading "too much" data which is ultimately coming from the next "row" down on the image which is leading to that stair stepped chunky effect.

Still working on a solution, but wanted to document my findings here.

[FoamyGuy]

I've continued digging around and I think I have a decent hold on where all of the code involved in this file download is located. However I'm at a bit of a loss currently in figuring out where to try to make a change to fix this.

Presumably we need to take this logic that was removed here and put it somewhere else outside of the socket API, but I'm having trouble figuring out where it would move to. The candidates as far as I can tell are adafruit_requests or adafruit_portalbase. But neither of those "feel" correct because both libraries are also used by FunHouse library. And I have confirmed that funhouse.network.wget() is able to download the image successfully without the stairstep chunky problem. So any change that we made inside of either of those places seems like it would result in changed behavior if the funhouse were to call wget() which I'm guessing would cause it to break, since it behaves correctly right now.

I have made this more simplified reproducer that downloads a single image that exhibits the chunky stair step issue:

import board
from adafruit_pyportal import PyPortal
import displayio
display = board.DISPLAY
aio_api_key="<aoi_api_key>"
url = f"https://io.adafruit.com/api/v2/Foamyguy/integrations/image-formatter?x-aio-key={aio_api_key}&width=480&height=305&output=BMP16&url=https://openaccess-cdn.clevelandart.org/2008.290/2008.290_web.jpg"
pyportal = PyPortal(debug=False)
pyportal.network.connect()
pyportal.wget(url, filename="/sd/downloaded.bmp", chunk_size=512)
print("after wget")
main_group = displayio.Group()
odb = displayio.OnDiskBitmap("/sd/downloaded.bmp")
tg = displayio.TileGrid(bitmap=odb, pixel_shader=odb.pixel_shader)
main_group.append(tg)

display.show(main_group)

while True:
    pass

(it could be further simplified by hosting a static copy of the bmp that the aio API returns and pointing this script to it instead.)

And I tested a very similar version of this on a FunHouse to confirm that when run under ESP32-S2 that wget does download the full file successfully without the problem.

[FoamyGuy]

Okay, I think I got this figured out now.

adafruit/Adafruit_CircuitPython_Requests#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]

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]

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.

[FoamyGuy]

I think is looking good at this point.

The following PRs include changes that are needed in other libraries to work correctly with the compatible socket from changes in this PR. They should get merged / released at the same time as this one is in order to keep everything in sync and working with released versions in the bundle:

adafruit/Adafruit_CircuitPython_WSGI#20
adafruit/Adafruit_CircuitPython_Requests#135

I have tested the changes with the following:

• The Simpletest and AIO_Post examples from this repo
• The relevant example scripts from MiniMQTT Library, with the changes from loop() timeout parameter should be absolute Adafruit_CircuitPython_MiniMQTT#169 applied
• https://learn.adafruit.com/pyportal-github-stars-trophy (uses pyportal library)
• Learn Projects that use the Requests library:
  • https://learn.adafruit.com/pyportal-weather-station/
  • https://learn.adafruit.com/pyportal-smart-thermometer-with-analog-devices-adt7410-adafruit-io-and-circuitpython
  • https://learn.adafruit.com/cleveland-museum-of-art-pyportal-frame (this is the one that had problems with the "chunky" downloads. It now works correctly)
• Learn Projects that use MiniMQTT Library:
  • https://learn.adafruit.com/iot-pyportal-data-logger-adafruitio-circuitpython
  • https://learn.adafruit.com/adafruit-io-hub-with-the-adafruit-funhouse/neopixel-remote-example

The majority of testing was carried out on PyPortal with 8.2.0-rc.0.

For the neopixel remote project I tested with a PyPortal Titano as the remote, and a Feather ESP32-S3 + Airlift Featherwing as the "receiver" with neopixels attached that changed colors when triggered by the remote.

This should be a major release number increase when the release is made.

[tannewt]

Thank you! Not merging so you can time it right.

[dhalbert]

In Python, socket.timeout is a deprecated alias of another exception, mostly recently now TimeoutError. See https://docs.python.org/3/library/socket.html#socket.timeout. Do we reference a timeout exception in other libraries? Could we use an existing defined exception? I don't know whether we want to preserve the deprecated name either.