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.

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

merged 6 commits into from
May 7, 2020

Conversation

dearmash
Copy link
Contributor

Writing to UDP requires separate APIs to use to append data and finalize.

TCP sends and confirms per-write and finalize is a no-op. UDP appends to a buffer and finalize sends all at once.

The simple socket_open method doesn't actually create a socket per https://github.com/arduino/nina-fw.

Since socket_connect is a convenience method anyway, and connect doesn't really make sense for UDP, add the start_server call for UDP to that method to have a socket to write as well as read on.

e.x. usage

esp.socket_connect(socket_num, dest, port, esp.UDP_MODE)
esp.socket_write(socket_num, buffer, esp.UDP_MODE)
avail = esp.socket_available(socket_num)
recv = esp.socket_read(socket_num, avail)
esp.socket_close(socket_num)

Writing to UDP requires separate APIs to use to append data and finalize.

TCP sends and confirms per-write and finalize is a no-op.  UDP appends to a buffer and finalize sends all at once.

The simple socket_open method doesn't actually create a socket per https://github.com/arduino/nina-fw.

Since socket_connect is a convenience method anyway, and connect doesn't really make sense for UDP, add the start_server call for UDP to that method to have a socket to write as well as read on.

e.x. usage

esp.socket_connect(socket_num, dest, port, esp.UDP_MODE)
esp.socket_write(socket_num, buffer, esp.UDP_MODE)
avail = esp.socket_available(socket_num)
recv = esp.socket_read(socket_num, avail)
esp.socket_close(socket_num)
@dearmash
Copy link
Contributor Author

Simple goal, was to get SSDP working on PyPortal so it can directly control my home automation devices. SSDP / multicast / all that goodness seems to work.

Getting the checks to pass now

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.

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
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?

@@ -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.

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

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
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).

adafruit_esp32spi/adafruit_esp32spi.py Outdated Show resolved Hide resolved
adafruit_esp32spi/adafruit_esp32spi.py Show resolved Hide resolved
@dearmash
Copy link
Contributor Author

Full disclosure this code sample is adapted from https://gist.github.com/dankrause/6000248

I didn't have any plans beyond just personal use of this. Looks like If there's any requirement for distribution, it's just attribution. Hoping this is enough.

It's heavily shortcutting via the pyportal library because I'm lazy. If you have a preferred method of getting an esp object, please do.

import time
from adafruit_pyportal import PyPortal

pyportal = PyPortal()
pyportal._connect_esp()
esp = pyportal._esp

socket_num = esp.get_socket()
dest = bytearray(b"\xEF\xFF\xFF\xFA") # 239.255.255.250
port = 1900
service = "ssdp:all"
mx = 3
conn_mode = esp.UDP_MODE

esp.socket_connect(socket_num, dest, port, conn_mode)

message = "\r\n".join(
    [
        "M-SEARCH * HTTP/1.1",
        "HOST: {dest}:{port}",
        'MAN: "ssdp:discover"',
        "ST: {st}",
        "MX: {mx}",
        "",
        "",
    ]
)

esp.socket_write(
    socket_num,
    message.format(dest=esp.pretty_ip(dest), port=port, st=service, mx=mx),
    conn_mode,
)

stamp = time.monotonic()
result = []
while time.monotonic() - stamp < 5:
    avail = min(1024, esp.socket_available(socket_num))
    if avail:
        stamp = time.monotonic()
        recv = esp.socket_read(socket_num, avail)
        result.append(recv)
esp.socket_close(socket_num)

print(result)

@dearmash dearmash requested a review from brentru May 7, 2020 06:11
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@dearmash this looks good to pull in, thanks for working on it!

@brentru brentru merged commit 94b0351 into adafruit:master May 7, 2020
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 8, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 3.4.0 from 3.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#97 from dearmash/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.6.3 from 3.6.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#45 from adafruit/dherrada-patch-2
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#42 from dherrada/i2c-fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_MSA301 to 1.2.2 from 1.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MSA301#5 from geekguy-wy/needs_renaming_to_match_lib_name
  > Merge pull request adafruit/Adafruit_CircuitPython_MSA301#4 from BiffoBear/rename-example-file

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyPortal to 3.2.4 from 3.2.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyPortal#81 from kattni/remove-from-pypi

Updating https://github.com/adafruit/Adafruit_CircuitPython_TSL2591 to 1.2.2 from 1.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_TSL2591#17 from adafruit/lux-note

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 2.2.1 from 2.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#32 from brentru/add-cellular

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 1.4.2 from 1.4.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#27 from brentru/add-cellular
@anecdata
Copy link
Member

anecdata commented May 11, 2020

@dearmash Are you on Discord? A user observed that successive UDP writes before close ends up appending to the buffer rather than just sending the new write data: https://discordapp.com/channels/327254708534116352/537365702651150357/709169810440847380 I don't see this behavior in ESP32 Arduino or with WiFiNINA. I've looked through ESP32SPI and NINA, and nothing jumps out at me... esp-idf issue maybe. Do you have any insights based on the work here?

@faithanalog
Copy link

@dearmash Are you on Discord? A user observed that successive UDP writes before close ends up appending to the buffer rather than just sending the new write data: https://discordapp.com/channels/327254708534116352/537365702651150357/709169810440847380 I don't see this behavior in ESP32 Arduino or with WiFiNINA. I've looked through ESP32SPI and NINA, and nothing jumps out at me... esp-idf issue maybe. Do you have any insights based on the work here?

@dearmash @anecdata i am also getting this behavior using the metro m4 airlift lite. i have just updated to latest stable versions of WiFiNINA and circuitpython and circuitpython library bundle.

Here's the weird thing: When my script first starts running everything works perfectly fine. However if i

  • lose wifi connection
  • close socket (or not, doesnt matter either way)
  • reconnect
  • recreate socket

i get this behavior where each call to send() will append to buffer instead of replacing the buffer as you described.

For example

sock.send("line 1+\n")
sock.send("line 2++\n")
sock.send("line 3+++\n")

would actually send

line 1+             <--- datagram 1
line 1+             <--- datagram 2
line 2++
line 1+             <--- datagram 3
line 2++
line 3+++

and this continues until the esp32's internal write buffer is full. at which point calls to send() will only send the buffer and not append anything new.

By the way this happens whether i call esp.reset() or not it literally does not matter which i find extra weird because I would think if its an esp32 state issue resetting it would fix it. maybe reset() isn't actually resetting it? idk.

@faithanalog
Copy link

Ive done some more digging and think I found the source. See #135

@anecdata
Copy link
Member

anecdata commented Apr 21, 2021

This PR broke the UDP client simpletest example esp32spi_udp_client.py

code.py output:
Server ping: 20 ms
Sending
Receiving
Traceback (most recent call last):
  File "code.py", line 52, in <module>
RuntimeError: buffer too small

Making the buffer size 49 instead of 48 fixes it, but I'm not sure why the extra byte is needed.

Edit:

No, still broken, it worked once at 49. Much larger buffer (128) gets a different error:

code.py output:
Server ping: 30 ms
Sending
Traceback (most recent call last):
  File "code.py", line 48, in <module>
  File "adafruit_esp32spi/adafruit_esp32spi_socket.py", line 86, in send
  File "/lib/adafruit_esp32spi/adafruit_esp32spi.py", line 719, in socket_write
RuntimeError: Failed to write 3 chunks (sent 2)

The example is a one-shot send example, not the (atypical) multiple sends on the same UDP socket addressed in this PR.

@dearmash
Copy link
Contributor Author

This PR broke the UDP client simpletest example esp32spi_udp_client.py

I'm not sure how this PR broke the simple test example, looking at the dates this PR predated the example.

In any case, I'm fully aware that this PR has shortcomings. Half due to only really implementing enough for my specific use case (pyportal controlled wemo smart devices), and half because the "real" solution would require fixes in nina-fw and Adafruit_CircuitPython_ESP32SPI in parallel.

I think @faithanalog has done a great job of discussing this in #135 and I'm looking forward to continuing the conversation there.

@anecdata
Copy link
Member

anecdata commented Apr 21, 2021

Sorry, my mistake, this popped up due to recent comments and I thought this was just Merged, and tested the changes in isolation from more recent code updates. The simpletest is fine with the current release.

@dearmash
Copy link
Contributor Author

No apology necessary, I'm genuinely looking forward to making it better. I know I half-assed it the first time around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants