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

fix(host/cdc): Add case for CDC class in device descriptor (IEC-236) #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zwizwa
Copy link

@zwizwa zwizwa commented Nov 21, 2024

Description

Added extra case in cdc_parse_is_cdc_compliant() to support CDC ACM device that has USB_CLASS_COMM set in the device descriptor.

Related

Testing

Tested on ESP32-S3 board connected to STM32F103 with CDC ACM descriptor. The STM32F103 is recognized in Linux as a ttyACMx device.


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@zwizwa
Copy link
Author

zwizwa commented Nov 21, 2024

The CDC ACM device's descriptors as printed by cdc_acm_host_desc_print()

*** Device descriptor ***
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0x2
bDeviceSubClass 0x0
bDeviceProtocol 0x0
bMaxPacketSize0 64
idVendor 0x483
idProduct 0x5740
bcdDevice 2.00
iManufacturer 1
iProduct 2
iSerialNumber 3
bNumConfigurations 1
*** Configuration descriptor ***
bLength 9
bDescriptorType 2
wTotalLength 67
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
bMaxPower 100mA
	*** Interface descriptor ***
	bLength 9
	bDescriptorType 4
	bInterfaceNumber 0
	bAlternateSetting 0
	bNumEndpoints 1
	bInterfaceClass 0x2
	bInterfaceSubClass 0x2
	bInterfaceProtocol 0x1
	iInterface 0
	*** CDC Header Descriptor ***
	bcdCDC: 1.10
	*** CDC Call Descriptor ***
	bmCapabilities: 0x00
	bDataInterface: 1
	*** CDC ACM Descriptor ***
	bmCapabilities: 0x00
	*** CDC Union Descriptor ***
	bControlInterface: 0
	bSubordinateInterface[0]: 1
		*** Endpoint descriptor ***
		bLength 7
		bDescriptorType 5
		bEndpointAddress 0x83	EP 3 IN
		bmAttributes 0x3	INT
		wMaxPacketSize 16
		bInterval 255
	*** Interface descriptor ***
	bLength 9
	bDescriptorType 4
	bInterfaceNumber 1
	bAlternateSetting 0
	bNumEndpoints 2
	bInterfaceClass 0xa
	bInterfaceSubClass 0x0
	bInterfaceProtocol 0x0
	iInterface 0
		*** Endpoint descriptor ***
		bLength 7
		bDescriptorType 5
		bEndpointAddress 0x1	EP 1 OUT
		bmAttributes 0x2	BULK
		wMaxPacketSize 64
		bInterval 1
		*** Endpoint descriptor ***
		bLength 7
		bDescriptorType 5
		bEndpointAddress 0x82	EP 2 IN
		bmAttributes 0x2	BULK
		wMaxPacketSize 64
		bInterval 1

@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 21, 2024
@github-actions github-actions bot changed the title fix(host/cdc): Add case for CDC class in device descriptor fix(host/cdc): Add case for CDC class in device descriptor (IEC-236) Nov 21, 2024
@peter-marcisovsky
Copy link
Collaborator

Hi @zwizwa thanks for the PR, we will take a look

@tore-espressif
Copy link
Collaborator

@zwizwa the changes LGTM! Could you please share full configuration and device descriptor in hex format? I'd like to add it to our tests

@zwizwa
Copy link
Author

zwizwa commented Nov 25, 2024

@zwizwa the changes LGTM! Could you please share full configuration and device descriptor in hex format? I'd like to add it to our tests

Sure. Is there a simple way to let the ACM firmware print the hex dumps?

if (device_desc->bDeviceClass == USB_CLASS_PER_INTERFACE) {
if (device_desc->bDeviceClass == USB_CLASS_COMM) {
// 0. This is a Communication Device Class: Class defined in Device descriptor
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of our host tests for descriptors parsing are not passing, because:

Here, device_desc->bDeviceClass == USB_CLASS_COMM we made sure, that this whole device is CLASS_COMM, but we also have to check, whether it's underlying interfaces are USB_CLASS_COMM as well.

There is a CDC device failing our tests, with a following descriptors (SimCom 7070G):

*** Device descriptor ***
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0x2
bDeviceSubClass 0x0
bDeviceProtocol 0x0
bMaxPacketSize0 64
idVendor 0x1e0e
idProduct 0x9206
bcdDevice 0.00
iManufacturer 3
iProduct 2
iSerialNumber 4
bNumConfigurations 1
*** Configuration descriptor ***
bLength 9
bDescriptorType 2
wTotalLength 154
bNumInterfaces 6
bConfigurationValue 1
iConfiguration 1
bmAttributes 0xe0
bMaxPower 500mA
	*** Interface descriptor ***
	bLength 9
	bDescriptorType 4
	bInterfaceNumber 0
	bAlternateSetting 0
	bNumEndpoints 2
	bInterfaceClass 0xff
	bInterfaceSubClass 0xff
	bInterfaceProtocol 0xff
	iInterface 0
		*** Endpoint descriptor ***
		bLength 7
		bDescriptorType 5
		bEndpointAddress 0x81	EP 1 IN
		bmAttributes 0x2	BULK
		wMaxPacketSize 64
		bInterval 0
		*** Endpoint descriptor ***
		bLength 7
		bDescriptorType 5
		bEndpointAddress 0x1	EP 1 OUT
		bmAttributes 0x2	BULK
		wMaxPacketSize 64
		bInterval 0

According to your changes we marked this interface as CDC compliant (because we marked the whole device CDC compliant) But, in fact, this interface is not CDC compliant bInterfaceClass 0xff if I am not mistaken.

Suggested change
return true;
const usb_intf_desc_t *intf_desc = usb_parse_interface_descriptor(config_desc, intf_idx, 0, NULL);
if (intf_desc->bInterfaceClass == USB_CLASS_COMM) {
return true;
}

Or this suggestion... the idea is more clear here..

    if (device_desc->bDeviceClass == USB_CLASS_PER_INTERFACE || device_desc->bDeviceClass == USB_CLASS_COMM) {
        const usb_intf_desc_t *intf_desc = usb_parse_interface_descriptor(config_desc, intf_idx, 0, NULL);
        if (intf_desc->bInterfaceClass == USB_CLASS_COMM) {
            // 1. This is a Communication Device Class: Class defined in Interface, or Device descriptor
            return true;
        }

With this changes, the host tests are passing correctly. We will update the tests case for this scenario

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Opened Issue is new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants