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.

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

WebUSB serial support (compile-time option, currently defaulted to OFF) #4126

Merged
merged 23 commits into from
Feb 5, 2021

Conversation

FiriaCTO
Copy link

@FiriaCTO FiriaCTO commented Feb 4, 2021

I put a WEBUSB_README.md file in the top-level directory, but in brief - this PR incorporates the functionality from the tinyusb example program "WebUSB serial demo". When this option is enabled, websites like
https://adafruit.github.io/Adafruit_TinyUSB_Arduino/examples/webusb-serial/index.html can access a WebUSB serial console from the browser. This work is for #3950

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 4, 2021

Thanks for doing this work. Before reviewing it in detail, I'd like to ask you if you also considered using the "Serial API", sometimes known as "WebSerial". This does not need its own descriptor on the device side, unlike WebUSB. Discussed in this closed issue: #605. #3950 is basically a duplicate of that.

Spec: https://web.dev/serial/
Status in Chrome: https://www.chromestatus.com/feature/6577673212002304

The Chrome Serial API demo does not quite work with CircuitPython (I see repeated <DISCONNECTED> <CONNECTED>). Bbut I think we could get it to work

I am currently working on adding a second CDC serial connection, not connected to the REPL, to allow a host to exchange arbitrary bytes with CircuitPython. Adding WebUSB support means having to choose between these on many boards due to the number of endpoints.

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 4, 2021

Some consideration was given to "Web Serial", and in fact if we had not been able to add support for "WebUSB Serial", the newer "Web Serial" was our backup plan. The reason we had such a high level of interest in "WebUSB serial" is that we have existing products already shipping that use it.

The reason we went with WebUSB originally is that the project/product line predates the addition of "Web Serial" to Chrome.

On a related note, we are also going to try to make use of WEB_DFU on this product, but I have not been involved in that work yet.

To the "limited endpoints" issue, I'm fine with CIRCUITPY_USB_VENDOR continuing to default to "off", since we will be building our product-specific code on a private fork anyway.

I will also admit we only became aware of the endpoint limitation a few days ago.

Time permitting (the product is slated to launch in June) we also hope to investigate the possibility of being able to change the USB interfaces dynamically at run-time.

You mentioned the demo not working for you. On some computers I had to manually specify "WINUSB" as the driver to get Windows happy (only have to do this one time, even if device is later uninstalled and redetected.) On the computer/dev board combo I am specifically using today, I have had to only have one USB connection plugged in at a time - either the "programming/downloading" cable, or the "CircuitPython" cable. This has seemed to be independent of which optional interfaces are enabled, so I had been blaming my PC.

You mentioned a "second CDC interface" - we had been assuming that the existing serial interface could be "taken away" from CircuitPython, much like the LCD (at least on Kaluga_1) starts out as a "echo of the serial console", but then you can still use it in your own programs too. If that turns out to be the case, maybe users can control their board over the WebUSB console, and use the existing CDC UART programmatically.

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 4, 2021

Re-reading and actually clicking on links, I see the "Chrome Demo" you referenced was in regards to the "Web serial" API.

I thought you were saying you had trouble with the "WebUSB Serial" demo at https://adafruit.github.io/Adafruit_TinyUSB_Arduino/examples/webusb-serial/index.html

I tried the one you mentioned, and it won't let me "Add a port". I can access the same CDC UART via putty (with WebUSB running too).

I wonder if some sort of "experimental flag" needs to be enabled in Chrome...
(update) Experimental Flag "Use system serial port enumerator" seemed to get this working...

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 4, 2021

Re-reading and actually clicking on links, I see the "Chrome Demo" you referenced was in regards to the "Web serial" API.

Yes, sorry, I explicated the link.

I tried the one you mentioned, and it won't let me "Add a port". I can access the same CDC UART via putty (with WebUSB running too).

I wonder if some sort of "experimental flag" needs to be enabled in Chrome...

I think I must have enabled it ages ago, when it first appeared behind a flag in Chrome 80-ish. It is supposed to be publicly visible in Chrome 89 (stable release is currently Chrome 88).

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 4, 2021

I wonder if some sort of "experimental flag" needs to be enabled in Chrome...

Yep... I happen to be running Chrome 88.0.4324.150 so I had to enable "Experimental Web Platform features" and "Use system serial port enumerator", but I did get it to finally bring up the REPL in the Chrome demo.
(follow-up) I think the "Use system serial port enumerator" flag was the critical one. My Chrome should be new enough to already have "Web Serial" enabled.

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 4, 2021

I still think it is worth having the option to build with WebUSB support. Since that standard has been defined longer, it may make it into other web browsers sooner.

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 4, 2021

Chrome is somewhat of an outlier in trying to implement WebUSB and the Serial API; you can see in the links above that Firefox and other browsers have decided not to support it in the interest of safety.

I am fine with the basic idea of this PR: we'll just leave it turned off by default. Unfortunately at the moment my own PR has a bunch of additions and cleanups to gen_usb_descriptor.py, and I'm also changing how to specify which USB and USB HID devices to provide, so there's a lot of churn. I'm not sure whether to merge your stuff first, or be impolite and get mine merged and force you to redo yours. I think it may not be so hard to merge your changes, though, so let's see if I can get this reviewed quickly.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to include turned off by default. I think we will tend toward the Serial API to avoid needing new descriptors, but this is valuable.

Just two minor things.

supervisor/supervisor.mk Outdated Show resolved Hide resolved
tools/gen_usb_descriptor.py Outdated Show resolved Hide resolved
FiriaCTO and others added 4 commits February 4, 2021 16:18
descriptors should be plural

Co-authored-by: Dan Halbert <halbert@halwitz.org>
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change www.circuitpython.org to circuitpython.org in gen_usb_descriptor.py

tools/gen_usb_descriptor.py Outdated Show resolved Hide resolved
@dhalbert
Copy link
Collaborator

dhalbert commented Feb 4, 2021

Ah, we crossed!

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 4, 2021

Sorry for the "cross"... can you fix it from your side (if needed)?

@FiriaCTO
Copy link
Author

FiriaCTO commented Feb 5, 2021

The github webpage is saying "1 change requested", but it won't show me what that change is. What do I need to do here?
Do I need to click "re-request review"? Clicking "Show review" just takes me to the same PR page???

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 5, 2021

You don't need to do anything - that's just because my review was done before your fix.

@dhalbert dhalbert merged commit 0a55cfb into adafruit:main Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants