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

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

Open
3 tasks done
Flamabalistic opened this issue May 31, 2024 · 2 comments
Open
3 tasks done

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

Flamabalistic opened this issue May 31, 2024 · 2 comments
Labels
Type: Bug Bug in esp-usb

Comments

@Flamabalistic
Copy link

Answers checklist.

  • I have read the component documentation ESP-IDF Components and the issue is not addressed there.
  • I am using target and esp-idf version as defined in component's idf_component.yml
  • 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) )
@Flamabalistic Flamabalistic added the Type: Bug Bug in esp-usb label May 31, 2024
@github-actions github-actions bot changed the title Incorrect PID when acting as multiple CDC devices Incorrect PID when acting as multiple CDC devices (IEC-117) May 31, 2024
@tore-espressif
Copy link
Collaborator

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug in esp-usb
Projects
None yet
Development

No branches or pull requests

2 participants