adafruit / Adafruit_CircuitPython_NTP

Network Time Protocol (NTP) Helper for CircuitPython
MIT License
9 stars 18 forks source link

No longer works on Pico W #33

Closed dhalbert closed 5 months ago

dhalbert commented 5 months ago

3.1.0 doesn't work on Pico W. See below. Tried with 9.0.4 and 9.1.0-beta.1. I am guessing the problem is the .connect(). 3.1.0 does work on ESP32-S3 9.1.0-beta.1

Traceback (most recent call last):
  File "code.py", line 39, in <module>
  File "adafruit_ntp.py", line 92, in datetime
BrokenPipeError: 32

tagging @justmobilize

b-blake commented 5 months ago

@dhalbert, Thank you.

justmobilize commented 5 months ago

Confirmed on a Pico W

Works (old):

with self._pool.socket(self._pool.AF_INET, self._pool.SOCK_DGRAM) as sock:
    sock.settimeout(self._socket_timeout)
    sock.sendto(self._packet, self._socket_address)
    sock.recv_into(self._packet)

Fails (new):

with self._pool.socket(self._pool.AF_INET, self._pool.SOCK_DGRAM) as sock:
    sock.settimeout(self._socket_timeout)
    sock.connect(self._socket_address)
    sock.send(self._packet)
    sock.recv_into(self._packet)

The BrokenPipeError is on the sock.send(self._packet).

I'm guessing it's not keeping the UDP connection alive?

anecdata commented 5 months ago

there's no actual connection to keep alive? maybe some low-level issue in raspberrypi .send

justmobilize commented 5 months ago

Yeah, wrong choice of words. More like still knowing about it.

anecdata commented 5 months ago

MICROPY_PY_LWIP_SOCK_RAW is 1...

        case SOCKETPOOL_SOCK_DGRAM:
        #if MICROPY_PY_LWIP_SOCK_RAW
        case SOCKETPOOL_SOCK_RAW:
        #endif

does that make the DGRAM case a NOP? (edit: n/m, I guess not) https://github.com/adafruit/circuitpython/blob/0f0509035329e933e5348cf4731f6146f012ce5a/ports/raspberrypi/common-hal/socketpool/Socket.c#L1134

justmobilize commented 5 months ago

If you look at sendto, it uses the IP, send passes null

justmobilize commented 5 months ago

Easiest fix would be to check for sento and use it

cc @jepler

anecdata commented 5 months ago

I think NULL is correct for RAW (for port, anyway)

I'm guessing RAW doesn't work either

justmobilize commented 5 months ago

In sendto it passes the IP for raw also

justmobilize commented 5 months ago

@dhalbert and @jepler I see 3 options:

  1. Check for sendto
  2. Add a UDP only sendto to esp32spi (could be expanded in the future)
  3. Update send for the pico w to work

I can do 1 or 2, one of you would need to do 3.

Let me know, if it's on me will fix tomorrow.

dhalbert commented 5 months ago

tagging @jepler here re

  1. Update send for the pico w to work
dhalbert commented 5 months ago

The CPython examples I see all use .sendto() for UDP. The fastest fix is to use .sendto() if it exists, and not connect(), as it was before. Then .sendto() can be implemented on esp32spi and the pico w impl can be fixed later.

As to whether you connect or not, it's more complicated than one might think: https://blog.cloudflare.com/everything-you-ever-wanted-to-know-about-udp-sockets-but-were-afraid-to-ask-part-1/

justmobilize commented 5 months ago

@dhalbert so are you good with me:

  1. adding sendto to esp32spi
  2. after that is released, switching to sendto here?
dhalbert commented 5 months ago

If you already know how to do sendto right away on ESP32SPI, that sounds fine. Then you'd remove .connect(), I assume. If it's not clear about doing sendto, I'd say do the workaround in the library for now while you work on sendto. That will make the library backward compatible with multiple versions.

justmobilize commented 5 months ago

@dhalbert it would literally be adding:

    def sendto(self, data, address):
        self.connect(address)
        self.send(data)

I've already tested and works great. I imagine that there could be other cases it doesn't work, but can tackle those when it happens.

dhalbert commented 5 months ago

In that sendto(), do you want to keep the connection or disconnect after every send? There is some discussion of that in the link I posted.

justmobilize commented 5 months ago

@dhalbert what do we do in CP for the ESP32S3? Would like it to be consistent

jepler commented 5 months ago

Please file a bug against circuitpython if socket.connect isn't working for UDP sockets on pico w.

b-blake commented 5 months ago

My solution is not as elegant as your but it works for me. If you can test the first line of boot_out.txt for 'rp2040' that would be better and more universal.

import board BOARD_ID = board.board_id

        with self._pool.socket(self._pool.AF_INET, self._pool.SOCK_DGRAM) as sock:
            sock.settimeout(self._socket_timeout)
            if BOARD_ID ==  'raspberry_pi_pico_w':                    #####
                # Version 3.0.13
                sock.sendto(self._packet, (self._server, self._port)) #####
            else:                                                     #####
                # Version 3.1.0
                # sock.connect(self._socket_address)                    ##### Don't need
                sock.send(self._packet)                               #####
            # Get the time in the context to minimize the difference between it and receiving
            # the packet.
            sock.recv_into(self._packet)
            destination = time.monotonic_ns()
dhalbert commented 5 months ago

@dhalbert what do we do in CP for the ESP32S3? Would like it to be consistent

It calls lwip_sendto(), which ultimately calls a low-level send operation. It does not do a connect.

justmobilize commented 5 months ago

@dhalbert, no I don't want to call close - since we need to be able to recv_into after