-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
Compatible socket #167
Compatible socket #167
Changes from all commits
5b5c5c6
43d6243
cce919c
743afe6
c5b6203
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my prior comment I had assumed that the 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
(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. | ||
|
@@ -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( | ||
|
@@ -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, | ||
|
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.