UDP send() does not clear esp32 write buffer

[faithanalog]

My Platform:

• metro m4 airlift lite
• adafruit nina-fw 1.7.3
• circuitpython 6.2.0
• April 20, 2021 library bundle

The problem:

udp.send() appends to the write buffer instead of replacing it. This results in the following behavior:

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

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

The expected behavior is that a call to write() will send the provided data as a datagram packet and nothing else.

Why this happens

In the nina-fw WiFiUdp.c implementation we can see the following:

• _sndSize tracks the write buffer size for the current packet
• beginPacket() sets _sndSize to 0. This is the only place where _sndSize is set to 0.
• endPacket() does not set _sndSize to 0.

The only nina-fw command that calls beginPacket() is the startTcpClient() command which, despite the outdated name, is also used to initialize UDP sockets.

sendUDPData() only calls endPacket(). It does not call beginPacket().

socket_write() has code roughly equivalent (with simplification and no handling of large buffers) to this:

if self.UDP_MODE:
    self._send_command_get_response(_INSERT_DATABUF_TCP_CMD, self._socknum_ll[0], memoryview(buffer))
    self._send_command_get_response(_SEND_UDP_DATA_CMD, self._socknum_ll)

_INSERT_DATABUF_TCP_CMD writes data to the UDP socket buffer, and then _SEND_UDP_DATA_CMD calls endPacket() which sends the current buffer as a datagram packet. beginPacket() is never called during a socket_write().

Workaround

Currently you can work around this problem by closing and re-opening the socket before every write. So every write() now becomes

sock.close()
sock.connect(socketaddr, conntype=esp.UDP_MODE)
sock.write()

The close() is necessary because connect() also opens a "server" port for receiving responses, and so connect() will fail if that server port is already open.

Potential fixes

It's not entirely clear to me whether this is a bug in esp32spi or in nina-fw. Changing esp32spi's socket_write to send a _START_SERVER_TCP_CMD message after sending _SEND_UDP_DATA_CMD would fix this issue. Changing nina-fw's sendUDPData() command to call beginPacket() again after endPacket() would also solve this issue. So would changing both to add a new command specifically to call beginPacket() into nina-fw and using it in esp32wifi.

So I'm happy to write a patch to fix this, but I don't know which project i should be writing the patch for.

Example code to cause this problem

import board
import time
from digitalio import DigitalInOut
import adafruit_esp32spi.adafruit_esp32spi_socket as socket
from adafruit_esp32spi import adafruit_esp32spi
from secrets import secrets

esp32_cs = DigitalInOut(board.ESP_CS)
esp32_ready = DigitalInOut(board.ESP_BUSY)
esp32_reset = DigitalInOut(board.ESP_RESET)
spi = board.SPI()
esp = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)


HOST = "192.168.1.100"
PORT = 3000


def connect():
    esp.reset()
    if esp.status == adafruit_esp32spi.WL_IDLE_STATUS:
        print("ESP32 found and in idle mode")
    print("Firmware vers.", esp.firmware_version)
    print("MAC addr:", [hex(i) for i in esp.MAC_address])
    print("Connecting to AP...")
    while not esp.is_connected:
        try:
            esp.connect_AP(secrets["ssid"], secrets["password"])
        except RuntimeError as e:
            print("could not connect to AP, retrying: ", e)
            continue
    print("Connected to", str(esp.ssid, "utf-8"), "\tRSSI:", esp.rssi)
    print("My IP address is", esp.pretty_ip(esp.ip_address))

    

    socket.set_interface(esp)
    socketaddr = socket.getaddrinfo(HOST, PORT)[0][4]
    
    global sock
    sock = socket.socket(type=socket.SOCK_DGRAM)

    sock.connect(socketaddr, conntype=esp.UDP_MODE)


connect()

i = 0


while True:
    if esp.is_connected:
        sock.send("{}\n".format(i))
        i = i + 1
    else:
        print("wifi is down :<")
        sock.close()
        connect()
    time.sleep(1.0)

[faithanalog]

Personal opinion part: I think that more generally the attempt to implement udp traffic under the api designed for tcp is maybe not a great long term solution.

The current implementation implements udp with patches on functions originally designed for tcp connections. So it runs startClientTcp() which in the case of udp just calls beginPacket() and returns. Then it runs startServerTcp() to start a udp server on the same port locally to receive responses.

This doesn't really align with how UDP actually works as a connectionless protocol.

Additionally, because a udp socket calls startServerTcp with the port of the target destination, the current API does not allow you to initiate UDP traffic to multiple destination hosts on the same port simultaneously. You must call close() on one before connect() for the next, because the local port bound for responses will be taken otherwise.

In the long term it may be good to implement an API specifically for UDP that aligns more with how UDP works to allow users to better take advantage of the strengths UDP. I would be happy to help implement that as well.

[dearmash]

You have hit the nail on the head. Correct on all points.

My goal with the original UDP PR #97 was to simply get it working enough for my use case, knowing very little of what the "general" UDP use case would be. Also there were pre-existing stubs in the code labeled as "not implemented", I assumed this was the desired api and just filled it in. I'm not the networking person whatsoever.

To get this working in general, it may require changes to both nina-fw as well as this library, but maybe not. WiFiNINA is another client of nina-fw that would need to be kept in sync. Those dependency chains are the reason I didn't want to mess with nina-fw.

My recommendation would be as you suggest, to break the interface between tcp and udp. Thus making a separate and more explicit beginPacket / write / endPacket interface.

WiFiUdpSendReceiveString would me my canonical example starting place, using the interface in WiFiUdp

If you do take this up, I'd be more than happy to lend a set of eyes for code review, though I technically have no power here.

[faithanalog]

Yeah I don't think I wrote it down how I had it in my head but the initial patch made a lot of sense as a low-impact way of initially getting some functionality in place.

To get this working in general, it may require changes to both nina-fw as well as this library, but maybe not. WiFiNINA is another client of nina-fw that would need to be kept in sync. Those dependency chains are the reason I didn't want to mess with nina-fw.

Oh hey I was actually just about to write something to this extent. yeah this is the big challenge is the question of where to implement the change. I think the cleanest / most efficient solution would involve changes to nina-fw.

But putting those things aside for practicality, I do think a proper UDP API can absolutely be implemented purely in esp32spi. I think all of the raw components of the underlying API that you'd need are already exposed:

• _START_CLIENT_TCP_CMD provides beginPacket()
• _SEND_UDP_DATA_CMD provides a way to write the packet
• _SEND_UDP_DATA_CMD provides endPacket()
• _START_SERVER_TCP_CMD provides begin() for receiving data
• _GET_DATABUF_TCP_CMD provides read() for reading received data

And all of these are already used on the existing UDP implementation, so we know they work. So it may be possible to implement most or all of an ideal UDP-specific API in terms of these without changing nina-fw, in a way that would also allow future changes to nina-fw to make it better. Basing it on the current WiFiNINA UDP API makes a lot of sense to me, thanks for pointing that out.

Something I want to note so I don't forget is _START_SERVER_TCP_CMD allocates a socket on the esp32 but only _STOP_CLIENT_TCP_CMD de-allocates it, which is just a thing to look out for so we don't accidentally design an implementation for receiving data that drains the esp32 of available socket IDs.

I'm happy to write a draft implementation in a PR when I get some time for it.

[dearmash]

when I get some time for it.

That's what I keep telling myself..

[f0x52]

Ran into this exact problem, unfortunately only found this issue after following all the threads myself.. Given nobody seems to have had the time to fix it in the meantime (totally understandable) maybe it'd be good to at least document this pitfall for now

[FoamyGuy]

@f0x52 you interested in submitting a PR with a warning added to the readme file or documentation?

[anecdata]

This issue also seems to affect UDP server. This UDP server code works:
https://gist.github.com/anecdata/61dfb2e5f4649a1a42885f8e9314800a
But if the server attempts to write a reply back to the client, the buffer gets appended rather than overwritten.

I've tried a number of variations, and it doesn't seem to work to close and re-open the socket before sending, or to create a new socket to send the reply.

[justmobilize]

Tested:

• Closing the socket
• Getting a new one
• Calling start_server

returns the same socket number and still sends the old data too.

[faithanalog]

Given it's 3 years on now and my life circumstances have changed, I ought to say I'm not likely to find time to write a fix for this. I may be able to help review someone else's PR if they write one.

[justmobilize]

Later today, I will try the beta firmware and see if it works or does something different.

[justmobilize]

Verified this still happens with the 1.8.0 beta. Might be best to get that tested/merged first and then figure out how to fix this.

cc @dhalbert