Skip to content

Serial connections dropping characters, not flushing buffers #885

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

Closed
mischif opened this issue Jun 16, 2024 · 2 comments · Fixed by #886
Closed

Serial connections dropping characters, not flushing buffers #885

mischif opened this issue Jun 16, 2024 · 2 comments · Fixed by #886

Comments

@mischif
Copy link

mischif commented Jun 16, 2024

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
Copy link
Author

mischif commented Jun 18, 2024

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
Copy link
Contributor

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.

projectgus added a commit to projectgus/micropython-lib that referenced this issue Jun 18, 2024
If the CDC receive buffer was full and some code read less than 64
bytes (wMaxTransferSize), the CDC code would submit an OUT transfer
with N<64 bytes length to fill the buffer back up.

However if the host had more than N bytes to send then it would still send
the full 64 bytes (correctly) in the transfer. The remaining (64-N) bytes
would be lost.

Adds the restriction that CDCInterface rxbuf has to be at least 64 bytes.

Closes micropython#885.

Signed-off-by: Angus Gratton <[email protected]>
projectgus added a commit to projectgus/micropython-lib that referenced this issue Jun 18, 2024
If the CDC receive buffer was full and some code read less than 64
bytes (wMaxTransferSize), the CDC code would submit an OUT transfer
with N<64 bytes length to fill the buffer back up.

However if the host had more than N bytes to send then it would still send
the full 64 bytes (correctly) in the transfer. The remaining (64-N) bytes
would be lost.

Adds the restriction that CDCInterface rxbuf has to be at least 64 bytes.

Closes micropython#885.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
dpgeorge pushed a commit to projectgus/micropython-lib that referenced this issue Jul 3, 2024
If the CDC receive buffer was full and some code read less than 64 bytes
(wMaxTransferSize), the CDC code would submit an OUT transfer with N<64
bytes length to fill the buffer back up.

However if the host had more than N bytes to send then it would still send
the full 64 bytes (correctly) in the transfer. The remaining (64-N) bytes
would be lost.

Adds the restriction that CDCInterface rxbuf has to be at least 64 bytes.

Fixes issue micropython#885.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
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 a pull request may close this issue.

2 participants