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

Add UDP support #97

Merged
merged 6 commits into from
May 7, 2020
Merged

Add UDP support #97

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
138bccb
Add UDP support
dearmash Apr 30, 2020
f437871
Formatting
dearmash Apr 30, 2020
aa96dd1
pylint
dearmash Apr 30, 2020
82edb0a
pylint
dearmash Apr 30, 2020
2c0466f
fix indentation for easier merge
dearmash Apr 30, 2020
974820a
Clarify log statemeny
dearmash Apr 30, 2020
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
30 changes: 27 additions & 3 deletions adafruit_esp32spi/adafruit_esp32spi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,15 @@
_GET_HOST_BY_NAME_CMD = const(0x35)
_START_SCAN_NETWORKS = const(0x36)
_GET_FW_VERSION_CMD = const(0x37)
_SEND_UDP_DATA_CMD = const(0x39)
_GET_TIME = const(0x3B)
_GET_IDX_BSSID_CMD = const(0x3C)
_GET_IDX_CHAN_CMD = const(0x3D)
_PING_CMD = const(0x3E)

_SEND_DATA_TCP_CMD = const(0x44)
_GET_DATABUF_TCP_CMD = const(0x45)
_INSERT_DATABUF_TCP_CMD = const(0x46)
_SET_ENT_IDENT_CMD = const(0x4A)
_SET_ENT_UNAME_CMD = const(0x4B)
_SET_ENT_PASSWD_CMD = const(0x4C)
Expand Down Expand Up @@ -689,15 +691,19 @@ def socket_connected(self, socket_num):
"""Test if a socket is connected to the destination, returns boolean true/false"""
return self.socket_status(socket_num) == SOCKET_ESTABLISHED

def socket_write(self, socket_num, buffer):
def socket_write(self, socket_num, buffer, conn_mode=TCP_MODE):
Copy link
Member

Choose a reason for hiding this comment

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

Please update ESP32SPI_Socket's send method to reflect changing the method signature: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_socket.py#L98

Each socket object contains it's connection type (defined during init here: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_socket.py#L73).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on the fence here, the esp_socket class seems like it was only intended for use as a tcp/tls wrapper for adafruit_requests.

on L73 it accepts a type that's not actually used. In fact the type=STREAM means it's not actually valid for UDP which is DGRAM (iirc of the top of my head). So in reality, if you tried to use that class for UDP, it'd break anyway, unless it were significantly updated as well. (this PR or a followup?)

on L98 when it calls write, it should default to TLS (really just "not UDP") which is handled identically under the hood in nina-fw.

https://github.com/arduino/nina-fw/blob/master/main/CommandHandler.cpp#L970 is where the tcp write method selects tcp/tls, and is unhandled for udp

"""Write the bytearray buffer to a socket"""
if self._debug:
print("Writing:", buffer)
self._socknum_ll[0][0] = socket_num
sent = 0
for chunk in range((len(buffer) // 64) + 1):
brentru marked this conversation as resolved.
Show resolved Hide resolved
total_chunks = (len(buffer) // 64) + 1
send_command = _SEND_DATA_TCP_CMD
if conn_mode == self.UDP_MODE: # UDP requires a different command to write
send_command = _INSERT_DATABUF_TCP_CMD
for chunk in range(total_chunks):
resp = self._send_command_get_response(
_SEND_DATA_TCP_CMD,
send_command,
(
self._socknum_ll[0],
memoryview(buffer)[(chunk * 64) : ((chunk + 1) * 64)],
Expand All @@ -706,6 +712,18 @@ def socket_write(self, socket_num, buffer):
)
sent += resp[0][0]

if conn_mode == self.UDP_MODE:
# UDP verifies chunks on write, not bytes
if sent != total_chunks:
raise RuntimeError(
"Failed to write %d chunks (sent %d)" % (total_chunks, sent)
)
# UDP needs to finalize with this command, does the actual sending
resp = self._send_command_get_response(_SEND_UDP_DATA_CMD, self._socknum_ll)
if resp[0][0] != 1:
raise RuntimeError("Failed to send data")
dearmash marked this conversation as resolved.
Show resolved Hide resolved
return

if sent != len(buffer):
raise RuntimeError(
"Failed to send %d bytes (sent %d)" % (len(buffer), sent)
Expand Down Expand Up @@ -749,6 +767,12 @@ def socket_connect(self, socket_num, dest, port, conn_mode=TCP_MODE):
print("*** Socket connect mode", conn_mode)

self.socket_open(socket_num, dest, port, conn_mode=conn_mode)
if conn_mode == self.UDP_MODE:
# UDP doesn't actually establish a connection
# but the socket for writing is created via start_server
self.start_server(port, socket_num, conn_mode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how much of an issue this is, start_server is defined underneath this fn. It works on my pyportal, but the rest of these files seem to respect the convention of only using functions defined above.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add an example of what you're running (your code.py) on your device?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a skeleton call flow to the description, I can clean up and put in a full code.py example. It's specific to SSDP though, I don't want to cause confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a UDP socket, you can't check if the socket was connected. Is there a way to check if the socket was allocated by the ESP32SPI to prevent attempting to open multiple UDP sockets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

socket_open is already verified implicitly the same way for tcp and udp

_send_command_get_response requires a response param to be present. If a socket fails to allocate, it doesn't return a result. The only way UDP socket_open fails is if dns lookup fails https://github.com/arduino/nina-fw/blob/master/arduino/libraries/WiFi/src/WiFiUdp.cpp#L100

https://github.com/arduino/nina-fw/blob/master/main/CommandHandler.cpp#L663 is the normal connection check, and UDP is unhandled, as you mention there isn't a connection. So there's nothing to verify there.

Also, the acutal _socket object is only created in begin() not beginPacket(), see https://github.com/arduino/nina-fw/blob/master/arduino/libraries/WiFi/src/WiFiUdp.cpp#L41

So socket_open really does nothing useful for UDP unless you begin(), as any writes / endPackets() will fail. In this way, by calling begin() it's verifying the rest works

I would categorize these as potentially bugs in nina-fw vs bugs in this library, they need to be fixed in parallel. I'm just way more comfortable not needing to flash the ESP32 to fix them (for now).

return True

times = time.monotonic()
while (time.monotonic() - times) < 3: # wait 3 seconds
if self.socket_connected(socket_num):
Expand Down