micropython / micropython-lib

Core Python libraries ported to MicroPython
Other
2.3k stars 980 forks source link

Serial connections dropping characters, not flushing buffers #885

Open mischif opened 2 weeks ago

mischif commented 2 weeks ago

I tested this on a Pico running MP 1.23 and the latest version of usb-device-cdc on mip main.py:

from io import BytesIO
from select import poll, POLLIN, POLLHUP, POLLERR
from time import sleep_ms

import usb.device

from usb.device.cdc import CDCInterface

SERIAL_TWO = None

def ingest():
    f = BytesIO(512)
    ba = bytearray(256)
    mv = memoryview(ba)

    print("beginning ingest")
    # SERIAL_TWO.read(4)
    while True:
        bytes_read = SERIAL_TWO.readinto(mv)
        if bytes_read:
            f.write(mv[:bytes_read])
            print("{} written, total {}".format(bytes_read, f.tell()))
        else:
            break

    f.seek(0)
    print(f.read(-1))

def main():
    global SERIAL_TWO

    buf = bytearray(4)
    SERIAL_TWO = CDCInterface(timeout=0)
    usb.device.get().init(SERIAL_TWO, builtin_driver=True)
    while not SERIAL_TWO.is_open():
        sleep_ms(100)

    api_poll = poll()
    api_poll.register(SERIAL_TWO, POLLIN)

    while True:
        cmd_waiting = api_poll.poll(0)
        if cmd_waiting and cmd_waiting[0][1] is POLLIN:
            # SERIAL_TWO.read(4)
            ingest()

        sleep_ms(1000)

if __name__ == '__main__': main()

client.py:

from serial import Serial
from string import ascii_letters

def main():
    test_str = ascii_letters * 7
    conn = Serial("/dev/ttyACM1", 115200)
    conn.read_all()
    data = "\x03" + "\n" + "\x21" + "\n" + test_str
    conn.write(data.encode())

if __name__ == '__main__': main()

I need to read four bytes off the input to confirm I should call ingest(), but in doing so I consistently lose eight bytes after the first call to readinto() (the output skips V -> e after 256 bytes) and some of the content never appears to leave the buffer. Moving the read() call into ingest() doesn't help, nor does using readinto() instead.

mischif commented 2 weeks ago

This may or may not be related, but main() is meant to be a tight loop and since I can't say for sure how long ingest() will take I really need to call it via schedule(); when I do that I can only get one buffer's worth of data before reading from SERIAL_TWO fails like I hit EOF. I know there's more there but I can't get to it, could this also be some sort of schedule()-induced deadlock?

projectgus commented 2 weeks ago

There is a bug here, thanks for the report. When N bytes was read from the CDC interface, N < 64, and the host has more than N bytes still to send then (64-N) bytes were dropped. Fix incoming.

This may or may not be related, but main() is meant to be a tight loop and since I can't say for sure how long ingest() will take I really need to call it via schedule(); when I do that I can only get one buffer's worth of data before reading from SERIAL_TWO fails like I hit EOF. I know there's more there but I can't get to it, could this also be some sort of schedule()-induced deadlock?

This part has the same root cause as #873 - the callbacks for completed USB transfers run using the same mechanism as schedule() so you can't block waiting for USB data inside a scheduled callback, it will block indefinitely. Need to find a way to exit the callback to allow more USB data transfer.

I will also look at adding a cdc.any() function that returns the number of buffered bytes. That would make it possible to delay calling the callback again until you know all the bytes are ready. Can also look at putting together an asyncio example for CDC (I believe it shouldn't need any code changes) as this type of application is a lot easier to structure with asyncio.