pbio/drv/usb: Convert to serial#482
Open
laurensvalk wants to merge 3 commits into
Open
Conversation
This reverts commit 0bd1ca8.
The previous WebUSB driver was a promising start, but it was not reliable. Messages could get stuck and the hub would not know if the app was disconnected. Bidirectional traffic was slow and prone to lockups. This commit replaces the USB drivers on all platforms with a standard serial USB device. We will be able to use this with Web Serial on most systems. Instead of manually subscribing and unsubscribing to keep track of the app connection state, we can use the DTR signal which is asserted on connect and deasserted on disconnect, even when the browser tab is abrubtly closed. Since serial is handled by the OS rather than our host application, in-flight messages don't get stuck if the host app is not reading them, which was part of the reason we had lockups before. This should also make it possible to use it with RFCOMM so we can add Bluetooth support for EV3 and NXT with relatively little changes. Finally, it may allow us to align the SPIKE Prime update procedure with the official firmware, for a more streamlined approach. Although the medium is a serial stream, we keep the same packetized event messages as before, much like we do for BLE. Frames are encoded with COBS with a 0x00 delimiter between messages, which makes it easy to get back in sync. The pull request for this change discusses further work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous WebUSB driver was a promising start, but it was not reliable. Messages could get stuck and the hub would not know if the app was disconnected. Bidirectional traffic was slow and prone to lockups.
This replaces the USB drivers on all platforms with a standard serial USB device. We will be able to use this with Web Serial on most systems. Instead of manually subscribing and unsubscribing to keep track of the app connection state, we can use the DTR signal which is asserted on connect and deasserted on disconnect, even when the browser tab is abrubtly closed.
Since serial is handled by the OS rather than our host application, in-flight messages don't get stuck if the host app is not reading them, which was part of the reason we had lockups before. This should also make it possible to use it with RFCOMM so we can add Bluetooth support for EV3 and NXT with relatively little changes.
Although the medium is a serial stream, we keep the same packetized event messages as before, much like we do for BLE. Frames are encoded with COBS with a 0x00 delimiter between messages, which makes it easy to get back in sync.
Summary
The most important is the change in
protocol.h. Subscribe is no longer needed and we add a read request and reply for the device information.typedef enum { /** Reply to a ::PBIO_PYBRICKS_OUT_EP_MSG_COMMAND. */ PBIO_PYBRICKS_IN_EP_MSG_RESPONSE = 1, /** Analog to BLE notification. Emitted while a host is connected. */ PBIO_PYBRICKS_IN_EP_MSG_EVENT = 2, + /** Reply to a ::PBIO_PYBRICKS_OUT_EP_MSG_READ. */ + PBIO_PYBRICKS_IN_EP_MSG_READ_REPLY = 3, } pbio_pybricks_usb_in_ep_msg_t; typedef enum { - PBIO_PYBRICKS_OUT_EP_MSG_SUBSCRIBE = 1, + /** A characteristic read request. */ + PBIO_PYBRICKS_OUT_EP_MSG_READ = 1, /** A Pybricks command */ PBIO_PYBRICKS_OUT_EP_MSG_COMMAND = 2, } pbio_pybricks_usb_out_ep_msg_t;Next steps
pbdrv_usb_process_threadthat effectively awaits onepbdrv_usb_tx_chunkper message that we send. This keeps this commit to the point, with the main focus on the driver update. But since the outgoing path is a serial stream, this common driver could just append it to an outgoing ring buffer, equivalent to the rx path. So we could probably do some refactoring with the various buffers we have today, and drop some assumptions about whole-packets in various places.stm32,ev3, ...) no longer call intopbsysfor hub version information. But it still lives in the common driver. This belongs insys/host. We could do the same clean up for the bluetooth drivers which hardwire this too, but that touches too much code to do it right now.Testing
This PR needs a complementary change to Pybricks Code, which is currently in progress. The following script can be used to start an existing slot, like the REPL:
The output should be:
This is also a good overview that this encoding is very minimal, and far simpler than trying to stay in sync because we don't need to understand the context of each message at the driver level.