Suggested feature: add a second USB serial port for remote control features

[kentindell]

I have a fork of the firmware for the CANPico boards and have added a second USB serial port for use by MIN (a reliable serial transport protocol) to send and receive remote control commands from a host (in my case, pushing CAN frames received to a host to see in Wireshark). It's a lot more robust than trying to multiplex the REPL serial port into raw mode.

I hardwired the second port by bumping CFG_TUD_CDC to 2 in tusb_config.h and the table usbd_desc_cfg in tusb_port.c.

[chris-est]

Hi @kentindell,
can you tell me what exactly you changed in the usbd_desc_cfg and how you use the extra serial port in python?
I would also like to have a separate serial port over USB to exchange data between the host and a RPi Pico.

[kentindell]

You can check out my patch here:

https://github.com/kentindell/canhack/blob/master/pico/micropython/v1.16.patch

You can create a second instance of a serial port, which you can then open to the host as a normal port - exactly as you want.

[chris-est]

Thank you for the patch, now i got two serial devices on the host: /dev/ttyACM0 and /dev/ttyACM1 👍
The first serial device is the REPL console, however i do not know how to read or write to the second serial device from within micropython. What do you mean by 'create a second instance of a serial port', do you have a small code example?

[kentindell]

Actually, I never wrote any Python code for USB serial: what I wrote is a new MicroPython class that wraps up the MIN protocol C library (https://github.com/min-protocol/min), adding C calls directly to the USB serial handler in the MicroPython firmware.

The existing Python serial API should work but I'm not sure how to address the port: sys.stdin is the REPL port. Probably it's a case of adding a new symbol into the sys class to name it, but I'm not that familiar with the MicroPython serial API. Any hints @dpgeorge?

[raveslave]

also been looking for a side-channel for piping data to/from the target
any updates in this topic?

[kentindell]

The current design doesn't have concurrency control between the streams so my patch only works if REPL isn't sending anything. For MIN that's OK because it's invoked from REPL so that's quiescent.

Obviously the proper fix is multiple buffers with concurrency control and ideally dynamic USB endpoint/buffer allocation. But I think that's a fairly big chunk of work.

[raveslave]

Noted that circuitpython has support to dynamically enable more CDC's making it easy to add a bi-directional "command-channel"

[raveslave]

btw, do you have any examples or projects where you setup two ports + min for cmds in the 2nd?

...wouldn't it be possible to pass the min encoded commands through the repl? a bit like filesystem commands work!?

[kentindell]

I used MIN with the CANPico to turn it into a CAN adapter, giving a host a Python API.

I used to use REPL in raw mode on the PyBoard for this but binary data kept generating spurious CTRL- commands, and it was really a mess to get a clean and robust solution that didn't also ruin REPL sessions.

[kentindell]

My modification to add a second CDC endpoint doesn't work since the refactoring to move TinyUSB configurations to the top level shared directory.

I've done a quick hack to the single file mp_usbd_descriptor.c to roll in what I did before for the Pico port. The only changes are to undef some stuff that's defined in tusb_config.h and to add in a second line into the descriptor array. The MSC class isn't defined so there's no attempt to take that into account.

When firmware boots there are no /dev/ttyACM ports enumerated. A detailed look with lusb shows no differences with an old version of the firmware that did work. I have no idea why it's not working, and I haven't got the time to learn in detail about USB. I think at this point I've reached the end of the road for maintaining a patch to keep this going.

#include "mpconfigport.h"
#include "tusb.h"
#include "mp_usbd.h"
#include "mp_usbd_internal.h"

#define USBD_CDC_CMD_MAX_SIZE (8)
#define USBD_CDC_IN_OUT_MAX_SIZE (64)

const tusb_desc_device_t mp_usbd_desc_device_static = {
    .bLength = sizeof(tusb_desc_device_t),
    .bDescriptorType = TUSB_DESC_DEVICE,
    .bcdUSB = 0x0200,
    .bDeviceClass = TUSB_CLASS_MISC,
    .bDeviceSubClass = MISC_SUBCLASS_COMMON,
    .bDeviceProtocol = MISC_PROTOCOL_IAD,
    .bMaxPacketSize0 = CFG_TUD_ENDPOINT0_SIZE,
    .idVendor = MICROPY_HW_USB_VID,
    .idProduct = MICROPY_HW_USB_PID,
    .bcdDevice = 0x0100,
    .iManufacturer = USBD_STR_MANUF,
    .iProduct = USBD_STR_PRODUCT,
    .iSerialNumber = USBD_STR_SERIAL,
    .bNumConfigurations = 1,
};

#define USBD_ITF_CDC2 (2) // needs 2 interfaces
#define USBD_CDC2_EP_CMD (0x83)
#define USBD_CDC2_EP_OUT (0x04)
#define USBD_CDC2_EP_IN (0x84)

#undef USBD_ITF_STATIC_MAX
#define USBD_ITF_STATIC_MAX (USBD_ITF_CDC2 + 2)

#undef USBD_STATIC_DESC_LEN
#define USBD_STATIC_DESC_LEN (TUD_CONFIG_DESC_LEN +                     \
    (CFG_TUD_CDC ? (TUD_CDC_DESC_LEN + TUD_CDC_DESC_LEN) : 0) +  \
    (CFG_TUD_MSC ? (TUD_MSC_DESC_LEN) : 0)    \
    )

const uint8_t mp_usbd_desc_cfg_static[USBD_STATIC_DESC_LEN] = {
    TUD_CONFIG_DESCRIPTOR(1, USBD_ITF_STATIC_MAX, USBD_STR_0, USBD_STATIC_DESC_LEN,
        0, USBD_MAX_POWER_MA),
    #if CFG_TUD_CDC
    TUD_CDC_DESCRIPTOR(USBD_ITF_CDC, USBD_STR_CDC, USBD_CDC_EP_CMD,
        USBD_CDC_CMD_MAX_SIZE, USBD_CDC_EP_OUT, USBD_CDC_EP_IN, USBD_CDC_IN_OUT_MAX_SIZE),
    TUD_CDC_DESCRIPTOR(USBD_ITF_CDC2, USBD_STR_CDC, USBD_CDC2_EP_CMD,
        USBD_CDC_CMD_MAX_SIZE, USBD_CDC2_EP_OUT, USBD_CDC2_EP_IN, USBD_CDC_IN_OUT_MAX_SIZE),
    #endif
    #if CFG_TUD_MSC
    TUD_MSC_DESCRIPTOR(USBD_ITF_MSC, 5, EPNUM_MSC_OUT, EPNUM_MSC_IN, 64),
    #endif
};

const uint16_t *tud_descriptor_string_cb(uint8_t index, uint16_t langid) {
    char serial_buf[USBD_DESC_STR_MAX + 1]; // Includes terminating NUL byte
    static uint16_t desc_wstr[USBD_DESC_STR_MAX + 1]; // Includes prefix uint16_t
    const char *desc_str;
    uint16_t desc_len;

    switch (index) {
        case 0:
            desc_wstr[1] = 0x0409; // supported language is English
            desc_len = 4;
            break;
        case USBD_STR_SERIAL:
            // TODO: make a port-specific serial number callback
            mp_usbd_port_get_serial_number(serial_buf);
            desc_str = serial_buf;
            break;
        case USBD_STR_MANUF:
            desc_str = MICROPY_HW_USB_MANUFACTURER_STRING;
            break;
        case USBD_STR_PRODUCT:
            desc_str = MICROPY_HW_USB_PRODUCT_FS_STRING;
            break;
        #if CFG_TUD_CDC
        case USBD_STR_CDC:
            desc_str = MICROPY_HW_USB_CDC_INTERFACE_STRING;
            break;
        #endif
        #if CFG_TUD_MSC
        case USBD_STR_MSC:
            desc_str = MICROPY_HW_USB_MSC_INTERFACE_STRING;
            break;
        #endif
        default:
            desc_str = NULL;
    }

    if (index != 0) {
        if (desc_str == NULL) {
            return NULL; // Will STALL the endpoint
        }

        // Convert from narrow string to wide string
        desc_len = 2;
        for (int i = 0; i < USBD_DESC_STR_MAX && desc_str[i] != 0; i++) {
            desc_wstr[1 + i] = desc_str[i];
            desc_len += 2;
        }
    }
    // first byte is length (including header), second byte is string type
    desc_wstr[0] = (TUSB_DESC_STRING << 8) | desc_len;

    return desc_wstr;
}


const uint8_t *tud_descriptor_device_cb(void) {
    return (const void *)&mp_usbd_desc_device_static;
}

const uint8_t *tud_descriptor_configuration_cb(uint8_t index) {
    (void)index;
    return mp_usbd_desc_cfg_static;
}

[projectgus]

@kentindell This code is 99% of the way there, in addition you need to set CFG_TUD_CDC to 2 so that TinyUSB knows that there are two CDC interfaces.

Here's a diff that registers /dev/ttyACM0 and /dev/ttyACM1 on Linux:
https://github.com/micropython/micropython/compare/master...projectgus:micropython:test/multi_usb_cdc?expand=1

On the general point of this feature request, once #9497 eventually lands then it should be possible to add extra CDC serial ports without writing any C code at all, unless you want to. So it should be easy to write an example for this use case.

USB debugging story for future reference

FWIW, I didn't see this immediately either, only noticed once I started digging at where the failure occurred. When I spun up your original patch there were two clues that sent me that way:

dmesg:

Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: new full-speed USB device number 25 using xhci_hcd
Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: New USB device found, idVendor=2e8a, idProduct=0005, bcdDevice= 1.00
Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: Product: Board in FS mode
Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: Manufacturer: MicroPython
Mar 01 09:21:26 obelisk kernel: usb 3-4.1.4: SerialNumber: e66118c4e37b4f21
Mar 01 09:21:27 obelisk kernel: usb 3-4.1.4: can't set config #1, error -32

Suggests the descriptors are parsed OK (this is why lsusb output looks correct), but the device is refusing to enable the requested (only) configuration.

The other one is usbmon module + Wireshark. Amazing for debugging USB without needing any external hardware.

This let me both look at the descriptors on-the-wire (looked alright), but then confirm the device was rejecting the SET CONFIGURATION request from the host:

I started looking in the tinyusbd CDC driver to see how it interacts with the configuration request, and that's when I noticed CFG_TUD_CDC was used to size most of the internal CDC state arrays.

[kentindell]

Thank you so much for this! I shall get straight on to it tomorrow. The USB debugging story is fascinating: I wouldn't have guessed at how to track down the bug. I would like to know more about USB at the lowest levels so that Wireshark tip is great.

[kentindell]

Oh, and I just saw what I wrote here two years ago: "I hardwired the second port by bumping CFG_TUD_CDC to 2 in tusb_config.h". D'oh!