Incorrect PID when acting as multiple CDC devices (IEC-117)

Flamabalistic commented 1 month ago

Answers checklist.

[X] I have read the component documentation ESP-IDF Components and the issue is not addressed there.
[X] I am using target and esp-idf version as defined in component's idf_component.yml
[X] I have searched the issue tracker for a similar issue and not found any related issue.

Which component are you using? If you choose Other, provide details in More Information.

device/esp_tinyusb

ESP-IDF version.

v5.3-dev-1353-gb3f7e2c8a4

Development Kit.

ESP32-S3-DevKitC-1

Used Component version.

1.4.4

More Information.

When acting as multiple CDC devices, the PID of the devices are misconfigured.

Instead of having PID 0x4001, as expected for a CDC device, they both instead have PID 0x4002, the PID of an MSC device.

I believe this is due to an error in usb_descriptors.c, as CFG_TUD_CDC is the number of CDC devices, not a flag as the code assumes. (The same goes with HID, and MIDI as seen in tusb_config.h

#define CFG_TUD_CDC                 CONFIG_TINYUSB_CDC_COUNT
#define CFG_TUD_MSC                 CONFIG_TINYUSB_MSC_ENABLED
#define CFG_TUD_HID                 CONFIG_TINYUSB_HID_COUNT
#define CFG_TUD_MIDI                CONFIG_TINYUSB_MIDI_COUNT
#define CFG_TUD_CUSTOM_CLASS        CONFIG_TINYUSB_CUSTOM_CLASS_ENABLED
#define CFG_TUD_ECM_RNDIS           CONFIG_TINYUSB_NET_MODE_ECM_RNDIS
#define CFG_TUD_NCM                 CONFIG_TINYUSB_NET_MODE_NCM
#define CFG_TUD_DFU                 CONFIG_TINYUSB_DFU_MODE_DFU
#define CFG_TUD_DFU_RUNTIME         CONFIG_TINYUSB_DFU_MODE_DFU_RUNTIME
#define CFG_TUD_BTH                 CONFIG_TINYUSB_BTH_ENABLED

To fix:

#define _PID_MAP(itf, n) ((CFG_TUD_##itf) << (n))
#define USB_TUSB_PID (0x4000 | _PID_MAP(CDC, 0) | _PID_MAP(MSC, 1) | _PID_MAP(HID, 2) | \
    _PID_MAP(MIDI, 3) ) //| _PID_MAP(AUDIO, 4) | _PID_MAP(VENDOR, 5) )

should be

#define _PID_MAP(itf, n) ((CFG_TUD_##itf > 0) << (n))
#define USB_TUSB_PID (0x4000 | _PID_MAP(CDC, 0) | _PID_MAP(MSC, 1) | _PID_MAP(HID, 2) | \
    _PID_MAP(MIDI, 3) ) //| _PID_MAP(AUDIO, 4) | _PID_MAP(VENDOR, 5) )

tore-espressif commented 3 weeks ago

hello @Flamabalistic thank you for the report.

You're right that the PID will mix MSC in this case. This code is taken from upstream tinyusb, for example here https://github.com/hathach/tinyusb/blob/master/examples/device/cdc_dual_ports/src/usb_descriptors.c#L35

Do you face any specific issues with this setup? If this is really a problem we can fix it in upstream TinyUSB and then fix it on our side too, just keep consistent behaviour

Flamabalistic commented 2 weeks ago

It could potentially cause issues with an incorrecy driver loading AFAIK - however as it's possible to overwrite the default PID, it's no big deal.

NOTE: For anyone who needs it, custom PIDs can be set in tinyusb_config_t.device_descriptor

espressif / esp-usb