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.

Sign up for GitHub

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

Jump to bottom

Writing to a second serial connection in a thread has errors #874

Closed
mischif opened this issue Jun 11, 2024 · 4 comments · Fixed by #896
Closed

Writing to a second serial connection in a thread has errors #874

mischif opened this issue Jun 11, 2024 · 4 comments · Fixed by #896

Comments

@mischif
Copy link

mischif commented Jun 11, 2024

Running on a Pi Pico with MicroPython 1.23.0 and whatever the latest version of usb-device-cdc is in mip; code to reproduce:

from utime import sleep_ms, time
from _thread import start_new_thread

import usb.device
from usb.device.cdc import CDCInterface

SERIAL_TWO = None


def update():
	while True:
		print("Update: {}".format(time()), file=SERIAL_TWO, end="\r\n")
		sleep_ms(1000)

def main():
	global SERIAL_TWO
	SERIAL_TWO = CDCInterface()
	SERIAL_TWO.init(timeout=0)
	usb.device.get().init(SERIAL_TWO, builtin_driver=True)

	while not SERIAL_TWO.is_open():
		sleep_ms(100)

	start_new_thread(update, ())

	while True:
		sleep_ms(1000)


if __name__ == "__main__": main()

If you connect to both ports with mpremote a0 and mpremote a1, you'll see this error sporadically:

Traceback (most recent call last):
  File "usb/device/core.py", line 316, in _xfer_cb
  File "usb/device/cdc.py", line 327, in _wr_cb
  File "usb/device/core.py", line 833, in finish_read
AssertionError:

But I did see this error once:

Unhandled exception in thread started by <function update at 0x2000aef0>
Traceback (most recent call last):
  File "<stdin>", line 12, in update
  File "usb/device/cdc.py", line 356, in write
  File "usb/device/cdc.py", line 322, in _wr_xfer
  File "usb/device/core.py", line 570, in submit_xfer
  File "usb/device/core.py", line 302, in _submit_xfer
RuntimeError: xfer_pending
@projectgus
Copy link
Contributor

projectgus commented Jun 11, 2024 •
edited
Loading

Hi @mischif,

Thanks for the report, I can reproduce this here. The CDC class currently uses machine.disable_irq() and machine.enable_irq() to control concurrent access to the serial buffer, but this doesn't prevent a thread on CPU1 from accessing it at the same time a USB callback is running. EDIT: This is wrong, machine.disable_irq() locks a mutex.

Will look into an alternative locking mechanism for this.

@projectgus
Copy link
Contributor

Root cause is not quite that actually, it's that the current USB transfer callback code marks the endpoint as free (no pending transfer) before it calls the transfer callback, so the callback code can call submit_xfer() if needed to submit a new transfer immediately.

However when the thread on the other CPU is submitting transfers, there's a race where it may see the endpoint has no pending transfer and submit a new transfer before the callback is done executing. Need to redesign the callbacks to handle this sequence.

@projectgus projectgus mentioned this issue Jun 12, 2024
Merged
@mischif
Copy link
Author

mischif commented Jul 10, 2024

Is there an update for this bug? I see one PR related to this was merged, but it's apparently only a partial fix.

@projectgus
Copy link
Contributor

projectgus commented Jul 11, 2024 •
edited
Loading

There is, I was going to return to these fixes this week but I got sick instead. I should have something soon.

projectgus added a commit to projectgus/micropython-lib that referenced this issue Jul 11, 2024
@projectgus
usb: Fix race if transfers are submitted by a thread.
f9a85d8 
The USB pending transfer flag was cleared before calling the completion
callback, to allow the callback code to call submit_xfer() again.

Unfortunately this isn't safe in a multi-threaded environment, as another
thread may see the endpoint is available before the callback is done
executing and submit a new transfer.

Rather than adding extra locking, specifically treat the transfer as still
pending if checked from another thread while the callback is executing.

Closes micropython#874

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus projectgus mentioned this issue Jul 11, 2024
Merged
projectgus added a commit to projectgus/micropython-lib that referenced this issue Jul 11, 2024
@projectgus
usb: Fix race if transfers are submitted by a thread.
3e421b7 
The USB pending transfer flag was cleared before calling the completion
callback, to allow the callback code to call submit_xfer() again.

Unfortunately this isn't safe in a multi-threaded environment, as another
thread may see the endpoint is available before the callback is done
executing and submit a new transfer.

Rather than adding extra locking, specifically treat the transfer as still
pending if checked from another thread while the callback is executing.

Closes micropython#874

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

usb: Fix USB device library transfer thread safety projectgus/micropython-lib
2 participants
@projectgus @mischif