Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compatible socket #167

Merged
merged 5 commits into from
Jul 8, 2023
Merged

Compatible socket #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b5c5c6
Remove non-CPython socket methods; remove wsgi server
dhalbert May 28, 2022
43d6243
Remove non-CPython socket methods; remove wsgi server
dhalbert May 28, 2022
cce919c
merge #166
dhalbert Jun 1, 2022
743afe6
Merge branch 'main' into compatible-socket
FoamyGuy Jun 9, 2023
c5b6203
timeout error property. Raise from recv_into if timeout time has elapsed
FoamyGuy Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adafruit_esp32spi/adafruit_esp32spi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ def socket_available(self, socket_num):
return reply

def socket_read(self, socket_num, size):
"""Read up to 'size' bytes from the socket number. Returns a bytearray"""
"""Read up to 'size' bytes from the socket number. Returns a bytes"""
if self._debug:
print(
"Reading %d bytes from ESP socket with status %d"
Expand Down
113 changes: 23 additions & 90 deletions adafruit_esp32spi/adafruit_esp32spi_socket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,80 +88,15 @@ def send(self, data): # pylint: disable=no-self-use
_the_interface.socket_write(self._socknum, data, conn_mode=conntype)
gc.collect()

def write(self, data):
"""Sends data to the socket.
NOTE: This method is deprecated and will be removed.
"""
self.send(data)

def readline(self, eol=b"\r\n"):
"""Attempt to return as many bytes as we can up to but not including
end-of-line character (default is '\\r\\n')"""

# print("Socket readline")
stamp = time.monotonic()
while eol not in self._buffer:
# there's no line already in there, read some more
avail = self.available()
if avail:
self._buffer += _the_interface.socket_read(self._socknum, avail)
elif self._timeout > 0 and time.monotonic() - stamp > self._timeout:
self.close() # Make sure to close socket so that we don't exhaust sockets.
raise OSError("Didn't receive full response, failing out")
firstline, self._buffer = self._buffer.split(eol, 1)
gc.collect()
return firstline

def recv(self, bufsize=0):
def recv(self, bufsize: int) -> bytes:
"""Reads some bytes from the connected remote address. Will only return
an empty string after the configured timeout.

:param int bufsize: maximum number of bytes to receive
"""
# print("Socket read", bufsize)
if bufsize == 0: # read as much as we can at the moment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while True:
avail = self.available()
if avail:
self._buffer += _the_interface.socket_read(self._socknum, avail)
else:
break
gc.collect()
ret = self._buffer
self._buffer = b""
gc.collect()
return ret
stamp = time.monotonic()

to_read = bufsize - len(self._buffer)
received = []
while to_read > 0:
# print("Bytes to read:", to_read)
avail = self.available()
if avail:
stamp = time.monotonic()
recv = _the_interface.socket_read(self._socknum, min(to_read, avail))
received.append(recv)
to_read -= len(recv)
gc.collect()
elif received:
# We've received some bytes but no more are available. So return
# what we have.
break
Comment on lines -147 to -150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@FoamyGuy FoamyGuy Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if self._timeout > 0 and time.monotonic() - stamp > self._timeout:
break
# print(received)
self._buffer += b"".join(received)

ret = None
if len(self._buffer) == bufsize:
ret = self._buffer
self._buffer = b""
else:
ret = self._buffer[:bufsize]
self._buffer = self._buffer[bufsize:]
gc.collect()
return ret
buf = bytearray(bufsize)
self.recv_into(buf, bufsize)
return bytes(buf)

def recv_into(self, buffer, nbytes: int = 0):
"""Read bytes from the connected remote address into a given buffer.
Expand All @@ -178,7 +113,7 @@ def recv_into(self, buffer, nbytes: int = 0):
num_to_read = len(buffer) if nbytes == 0 else nbytes
num_read = 0
while num_to_read > 0:
num_avail = self.available()
num_avail = self._available()
if num_avail > 0:
last_read_time = time.monotonic()
bytes_read = _the_interface.socket_read(
Expand All @@ -192,33 +127,28 @@ def recv_into(self, buffer, nbytes: int = 0):
break
# No bytes yet, or more bytes requested.
if self._timeout > 0 and time.monotonic() - last_read_time > self._timeout:
break
raise timeout("timed out")
return num_read

def read(self, size=0):
"""Read up to 'size' bytes from the socket, this may be buffered internally!
If 'size' isnt specified, return everything in the buffer.
NOTE: This method is deprecated and will be removed.
"""
return self.recv(size)

def settimeout(self, value):
"""Set the read timeout for sockets, if value is 0 it will block"""
"""Set the read timeout for sockets.
If value is 0 socket reads will block until a message is available.
"""
self._timeout = value

def available(self):
def _available(self):
"""Returns how many bytes of data are available to be read (up to the MAX_PACKET length)"""
if self.socknum != NO_SOCKET_AVAIL:
if self._socknum != NO_SOCKET_AVAIL:
return min(_the_interface.socket_available(self._socknum), MAX_PACKET)
return 0

def connected(self):
def _connected(self):
"""Whether or not we are connected to the socket"""
if self.socknum == NO_SOCKET_AVAIL:
if self._socknum == NO_SOCKET_AVAIL:
return False
if self.available():
if self._available():
return True
status = _the_interface.socket_status(self.socknum)
status = _the_interface.socket_status(self._socknum)
result = status not in (
adafruit_esp32spi.SOCKET_LISTEN,
adafruit_esp32spi.SOCKET_CLOSED,
Expand All @@ -234,14 +164,17 @@ def connected(self):
self._socknum = NO_SOCKET_AVAIL
return result

@property
def socknum(self):
"""The socket number"""
return self._socknum

def close(self):
"""Close the socket, after reading whatever remains"""
_the_interface.socket_close(self._socknum)


class timeout(TimeoutError):
"""TimeoutError class. An instance of this error will be raised by recv_into() if
the timeout has elapsed and we haven't received any data yet."""

def __init__(self, msg):
super().__init__(msg)


# pylint: enable=unused-argument, redefined-builtin, invalid-name
Loading