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

Second USB CDC (serial) channel #4215

Merged
merged 14 commits into from
Feb 20, 2021
Merged

Second USB CDC (serial) channel #4215

merged 14 commits into from
Feb 20, 2021

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Feb 18, 2021

Implements a second CDC channel on boards where it is possible.

  • New module usb_cdc. usb_cdc.serials[0] is the REPL channel; usb_cdc.serials[1] is the new secondary channel. Both are pre-created instances of usb_cdc.Serial.
  • usb_cdc.Serial is more strictly like pyserial than the current stream implementation. New pyserial-compatibility flags added to enable this, for use with usb_cdc:
    • read() defaults to reading one character.
    • readinto() does not take a second length argument. This was already implemented for busio.UART.
    • read() and readinto() never return None, just b'' and 0, respectively.
  • usb_cdc.Serial implements read(), readinto(), readline(), readlines(), write(), .in_waiting, .out_waiting, .reset_input_buffer, .reset_output_buffer, and .timeout. These parallel what is in pyserial. Also .connected is added, which is not in pyserial.

Other changes done during the implementation of usb_cdc:

  • Old USB_DEVICES and USB_HID_DEVICES compile time options are removed. Instead, there are now CIRCUITPY_USB_MSC, CIRCUITPY_USB_AUDIO, CIRCUITPY_USB_MIDI, etc. flags. Similarly for HID, there are now separate flags for each possible device: CIRCUITPY_USB_MOUSE, CIRCUITPY_USB_KEYBOARD, etc. The makefiles take care of passing the right info to the Python script that generates the descriptors.
  • All ports/boards now say how many USB endpoints they support.
  • Naming of supervisor.runtime common-hal routines is made consistent with the general common-hal naming scheme.
  • Various makefiles have been slightly cleaned up with alphabetization, more use of ?=, and better indentation.
  • tools/gen_usb_descriptor.py is black-formatted, and slightly cleaned up. It supports one extra CDC channel.
  • submodule usb_descriptor is updated to its latest version, which fixes the issue of skipping an endpoint.

Most chips with 8 endpoint pairs or more support usb_cdc. The exception is SAMD21, which has some bug that requires separate MSC endpoints for IN and OUT, instead of a pair. (EDIT: this turned out not to be true, and was fixed later.) Most stm32 chips do not have enough endpoints. ESP32-S2 and Spresense only have 5 endpoint pairs.

Tested on Metro M4 and on RPI Pico.

Fixes #231.
Fixes #4216.

@hathach
Copy link
Member

hathach commented Feb 18, 2021

from my experience, the ttyACM/COM numbering (0, 1) will swap around and could cause a bit of confusion 😃

@deshipu
Copy link

deshipu commented Feb 18, 2021

I have a silly question. If I disable the REPL CDC, on my build , will the index of the other CDC change in usb_cdc.serials? It would be nice to be able to test my code with repl enabled first, and then disable it without having to change the code.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Feb 18, 2021

from my experience, the ttyACM/COM numbering (0, 1) will swap around and could cause a bit of confusion

This has not happened to me (yet!): I tested on Linux, WIndows, and MacOS, and they were always assigned in sequential order.

I did have a problem in which I left pyserial running on ttyACM1, and then restarted the board. Then ttyACM0 and ttyACM2 were assigned. Then Linux got completely messed up and stopped assigning any new tty's, and I had to reboot. I attribute this to Linux's could-be-better USB support.

@dhalbert
Copy link
Collaborator Author

I have a silly question. If I disable the REPL CDC, on my build , will the index of the other CDC change in usb_cdc.serials? It would be nice to be able to test my code with repl enabled first, and then disable it without having to change the code.

Do you mean you would set CIRCUITPY_REPL_USB = 0? (That's the new way of disabling CDC.) I'm not sure what would happen -- I don't think I considered that case. The indices are fixed, and len(cdc_usb.serials) is always 2 right now. It might not even compile. So you want a serial connection but no REPL, is that right? I could probably make that work, maybe after this PR.

Is that because this will not otherwise work on SAMD21? You could disable MIDI and regain enough endpoints, and still have the REPL.

Eventually we will have dynamic USB descriptors, but I was able to avoid all that implementation work because there were just enough endpoints on many boards.

@hathach
Copy link
Member

hathach commented Feb 18, 2021

from my experience, the ttyACM/COM numbering (0, 1) will swap around and could cause a bit of confusion

This has not happened to me (yet!): I tested on Linux, WIndows, and MacOS, and they were always assigned in sequential order.

I did have a problem in which I left pyserial running on ttyACM1, and then restarted the board. Then ttyACM0 and ttyACM2 were assigned. Then Linux got completely messed up and stopped assigning any new tty's, and I had to reboot. I attribute this to Linux's could-be-better USB support.

yeah, It will be good on testing, and then decide to confuse us when we want to do something more stable. Just kidding, in case of issue e.g with ACM0 it will come back as ACM2 then we have ACM1 & ACM2. It can stay like that, and thing can be confusing on our support for which ACM for REPL. But it is really minor to those who want dual CDCs. Just make sure we have some note to user :)

@dhalbert
Copy link
Collaborator Author

yeah, It will be good on testing, and then decide to confuse us when we want to do something more stable. Just kidding, in case of issue e.g with ACM0 it will come back as ACM2 then we have ACM1 & ACM2. It can stay like that, and thing can be confusing on our support for which ACM for REPL. But it is really minor to those who want dual CDCs. Just make sure we have some note to user :)

It would be nice to be able to identify the CDC channels by the string ID's:

$ lsusb -D /dev/bus/usb/003/010 | grep iInterface
      iInterface              4 CircuitPython CDC control
      iInterface              5 CircuitPython CDC data
      iInterface              6 CircuitPython CDC2 control
      iInterface              7 CircuitPython CDC2 data
      iInterface              8 CircuitPython Mass Storage
      iInterface              9 CircuitPython HID
can't get device qualifier: Resource temporarily unavailable
can't get debug descriptor: Resource temporarily unavailable
      iInterface             13 CircuitPython Audio
      iInterface             12 CircuitPython MIDI

Do you know of a relatively platform-independent way to get those, or even an easy way on each platform? I look at the Qt stuff that Mu uses, but could not find a way to access these. That would assure that Mu is finding the REPL CDC, not the other one. RIght now it just looks for the first one. That was actually the testing I did; I wanted to make sure this wasn't going to completely confuse Mu right away.

@deshipu
Copy link

deshipu commented Feb 18, 2021

Do you mean you would set CIRCUITPY_REPL_USB = 0? (That's the new way of disabling CDC.) I'm not sure what would happen -- I don't think I considered that case. The indices are fixed, and len(cdc_usb.serials) is always 2 right now. It might not even compile. So you want a serial connection but no REPL, is that right? I could probably make that work, maybe after this PR.

Yes, that's what I meant.

Is that because this will not otherwise work on SAMD21? You could disable MIDI and regain enough endpoints, and still have the REPL.

No, it's not to free an endpoint, it's to avoid confusing end users. One of my specific cases is a stenotype, which is a serial device. Once I have all the code for it developed, I will disable all endpoints except for the CDC, so that when you connect it, it's obvious which device to choose in the Plover app. I expect a lot of people who try to make useful gadgets, as opposed to just playing with things, will want to disable the unnecessary endpoints (and, when dynamic endpoints are a thing, possibly enable them but only when a button is pressed at startup, similar to mounting the filesystem).

@tannewt
Copy link
Member

tannewt commented Feb 18, 2021

Here is a related issue for getting allowed by Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1169000 (Will review later.)

@Neradoc
Copy link

Neradoc commented Feb 18, 2021

pyserial exposes the port's string ID like this. That's on Mac but I think pyserial might provide that across platforms ?

import serial.tools.list_ports
for port in serial.tools.list_ports.comports():
    print(port.device,port.interface)
/dev/cu.Bluetooth-Incoming-Port None
/dev/cu.usbmodem7CDFA100E6201 CircuitPython CDC data

@tannewt tannewt self-requested a review February 18, 2021 20:04
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for this! Nothing major. Just a couple questions.

shared-module/usb_cdc/Serial.c Outdated Show resolved Hide resolved
shared-bindings/usb_cdc/Serial.c Outdated Show resolved Hide resolved
shared-module/usb_cdc/Serial.c Outdated Show resolved Hide resolved
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you!

@tannewt tannewt merged commit 7b6f89a into adafruit:main Feb 20, 2021
@dhalbert dhalbert deleted the secondary-cdc branch February 20, 2021 02:45
@deshipu
Copy link

deshipu commented Feb 27, 2021

It looks like this broke builds with CIRCUITPY_USB_MSC = 0:

ld: firmware.elf.lto.o: in function `usb_background':
<artificial>:(.text.usb_background+0xd2): undefined reference to `usb_msc_umount'
ld: <artificial>:(.text.usb_background+0x2ca): undefined reference to `usb_msc_mount'
ld: firmware.elf.lto.o: in function `storage_remount':
<artificial>:(.text.storage_remount+0x3c): undefined reference to `usb_msc_ejected'
ld: firmware.elf.lto.o:(.rodata._usbd_driver+0x18): undefined reference to `mscd_init'
ld: firmware.elf.lto.o:(.rodata._usbd_driver+0x1c): undefined reference to `mscd_reset'
ld: firmware.elf.lto.o:(.rodata._usbd_driver+0x20): undefined reference to `mscd_open'
ld: firmware.elf.lto.o:(.rodata._usbd_driver+0x24): undefined reference to `mscd_control_xfer_cb'
ld: firmware.elf.lto.o:(.rodata._usbd_driver+0x28): undefined reference to `mscd_xfer_cb'
collect2: error: ld returned 1 exit status
make: *** [Makefile:412: build-dorsch40k/firmware.elf] Error 1

@deshipu
Copy link

deshipu commented Feb 27, 2021

This can be worked around by always adding lib/tinyusb/src/class/msc/msc_device.c and supervisor/shared/usb/usb_msc_flash.c to the supervisor.mk.

@deshipu
Copy link

deshipu commented Feb 27, 2021

I looks like we should set CFG_TUD_MSC to 0 for tinyusb when CIRCUITPY_USB_MSC is 0.

@dhalbert
Copy link
Collaborator Author

If you figure this out in general way a PR would be welcome. Thanks! I did not test all the possibilities.

@deshipu
Copy link

deshipu commented Feb 27, 2021

#4283 should fix it

@louisfrederic
Copy link

Is the usb_cdc.data possible with the SAMD21?
what about the bug and how can somebody make two MSC Endpoints for IN an OUT?

"The exception is SAMD21, which has some bug that requires separate MSC endpoints for IN and OUT, instead of a pair"

@dhalbert
Copy link
Collaborator Author

@louisfrederic That turned out not to be true. usb_cdc is available on SAMD21 builds. I edited the post to make this clear.

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 this pull request may close these issues.

Serial Communication via USB only 'getch()'
6 participants