eclipse-threadx / usbx

Eclipse ThreadX - USBX is a high-performance USB host, device, and on-the-go (OTG) embedded stack, that is fully integrated with Eclipse ThreadX RTOS
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/usbx/index.md
MIT License
148 stars 88 forks source link

UsbX device printer #101

Closed MarcoTrap closed 1 year ago

MarcoTrap commented 1 year ago

I found a bug when I use printer class with other classes like one or more CDCs: the printer class must be not register as first to get the issue. When the usb device is connected to the host at the request GET_DEVICE_ID of the printer the device does not answer because can not find the class.

I'm using the MIMXRT1170-EVK demoboard with MCUXpresso IDE v11.7.0 and SDK version 2.13.0 (667 2023-01-18). I added a workaround to fix the problem in the file "ux_device_stack_control_request_process.c"

                /* Is the request target to an interface?  */
                if ((request_type & UX_REQUEST_TARGET) == UX_REQUEST_TARGET_INTERFACE)
                {

                    /* Yes, so the request index contains the index of the interface 
                       the request is for. So if the current index does not match 
                       the request index, we should go to the next one.  */
                    /* For printer class (0x07) GET_DEVICE_ID (0x00) the high byte of 
                       wIndex is interface index (for recommended index sequence the interface
                       number is same as interface index inside configuration).  */
// WORKAROUND
                    // Check if it is a printer class (0x07) and if it is request GET_DEVICE_ID (0x00) then uses wIndex as big endian otherwise printer does not work with other devices
                    if ((class_ptr -> ux_slave_class_interface -> ux_slave_interface_descriptor.bInterfaceClass == 0x07) && request == 0x00)                   
                        {
                            request_index = _ux_utility_short_get_big_endian(transfer_request -> ux_slave_transfer_request_setup + UX_SETUP_INDEX);
                         }
// WORKAROUND
                        if (((request_index & 0xFF) != class_index) ||
                            ((class_ptr -> ux_slave_class_interface -> ux_slave_interface_descriptor.bInterfaceClass == 0x07) &&
                             (request == 0x00) &&
                             *(transfer_request -> ux_slave_transfer_request_setup + UX_SETUP_INDEX + 1) != class_index))
                           continue;
                }

With this solution the usb printer device is acknowledge by host.

xiaocq2001 commented 1 year ago

Thanks for the feedback. The printer case is actually checked by the code block after your workaround:

                        if (((request_index & 0xFF) != class_index) ||
                            ((class_ptr -> ux_slave_class_interface -> ux_slave_interface_descriptor.bInterfaceClass == 0x07) &&
                             (request == 0x00) &&
                             *(transfer_request -> ux_slave_transfer_request_setup + UX_SETUP_INDEX + 1) != class_index))
                           continue;

The mistake is ||, where should be && to do not skip printer case if request_index is not equal to index scanned.

MarcoTrap commented 1 year ago

Hi @xiaocq2001, I tried your solution and works for the printer class, but in a scenario with 2 CDC classes and 1 printer it does not fit.

When the host send the request "Set Control Line State" ( 21 22 03 00 02 00 00 00 ) for the CDC2 it sets DTR RTS to the CDC1. These are the values of the request:

  request_type = 0x21
  request = 0x22
  request_value = 0x3
  request_index = 0x2
  request_length = 0x0
  class_index = 0x00

with && the control is bypassed and so it is configured the CDC1.

This is the configuration of the classes:

      ux_slave_class_name = "CDC1",
      ux_slave_class_status = 1,
      ux_slave_class_entry_function = 0x3000F0D5,
      ux_slave_class_instance = 0x2001E9B0,
      ux_slave_class_client = 0x0,
      ux_slave_class_thread = (tx_thread_id = 0, tx_thread_run_count = 0, tx_th
      ux_slave_class_thread_stack = 0x0,
      ux_slave_class_interface_parameter = 0x2003FEE0,
      ux_slave_class_interface_number = 0,
      ux_slave_class_configuration_number = 1,
      ux_slave_class_interface = 0x2001E408 -> (
        ux_slave_interface_status = 1,
        ux_slave_interface_class = 0x2001DF50,
        ux_slave_interface_class_instance = 0x2001E9B0,
        ux_slave_interface_descriptor = (
          bLength = 9,
          bDescriptorType = 4,
          bInterfaceNumber = 1,
          bAlternateSetting = 0,
          bNumEndpoints = 2,
          bInterfaceClass = 10,
          bInterfaceSubClass = 0,
          bInterfaceProtocol = 0,
          iInterface = 0),
        ux_slave_interface_next_interface = 0x2001E440,
        ux_slave_interface_first_endpoint = 0x2001E594))
      ux_slave_class_name = "CDC1",
      ux_slave_class_status = 1,
      ux_slave_class_entry_function = 0x3000F0D5,
      ux_slave_class_instance = 0x2001E9B0,
      ux_slave_class_client = 0x0,
      ux_slave_class_thread = (tx_thread_id = 0, tx_thread_run_count = 0, tx_th
      ux_slave_class_thread_stack = 0x0,
      ux_slave_class_interface_parameter = 0x2003FEE0,
      ux_slave_class_interface_number = 0,
      ux_slave_class_configuration_number = 1,
      ux_slave_class_interface = 0x2001E408 -> (
        ux_slave_interface_status = 1,
        ux_slave_interface_class = 0x2001DF50,
        ux_slave_interface_class_instance = 0x2001E9B0,
        ux_slave_interface_descriptor = (
          bLength = 9,
          bDescriptorType = 4,
          bInterfaceNumber = 1,
          bAlternateSetting = 0,
          bNumEndpoints = 2,
          bInterfaceClass = 10,
          bInterfaceSubClass = 0,
          bInterfaceProtocol = 0,
          iInterface = 0),
        ux_slave_interface_next_interface = 0x2001E440,
        ux_slave_interface_first_endpoint = 0x2001E594))
      ux_slave_class_name = "CDC2",
      ux_slave_class_status = 1,
      ux_slave_class_entry_function = 0x3000F0D5,
      ux_slave_class_instance = 0x2001F410,
      ux_slave_class_client = 0x0,
      ux_slave_class_thread = (tx_thread_id = 0, tx_thread_run_count = 0, tx_th
      ux_slave_class_thread_stack = 0x0,
      ux_slave_class_interface_parameter = 0x2003FEE0,
      ux_slave_class_interface_number = 2,
      ux_slave_class_configuration_number = 1,
      ux_slave_class_interface = 0x2001E478 -> (
        ux_slave_interface_status = 1,
        ux_slave_interface_class = 0x2001E06C,
        ux_slave_interface_class_instance = 0x2001F410,
        ux_slave_interface_descriptor = (
          bLength = 9,
          bDescriptorType = 4,
          bInterfaceNumber = 3,
          bAlternateSetting = 0,
          bNumEndpoints = 2,
          bInterfaceClass = 10,
          bInterfaceSubClass = 0,
          bInterfaceProtocol = 0,
          iInterface = 0),
        ux_slave_interface_next_interface = 0x2001E4B0,
        ux_slave_interface_first_endpoint = 0x2001E750))
      ux_slave_class_name = "CDC2",
      ux_slave_class_status = 1,
      ux_slave_class_entry_function = 0x3000F0D5,
      ux_slave_class_instance = 0x2001F410,
      ux_slave_class_client = 0x0,
      ux_slave_class_thread = (tx_thread_id = 0, tx_thread_run_count = 0, tx_th
      ux_slave_class_thread_stack = 0x0,
      ux_slave_class_interface_parameter = 0x2003FEE0,
      ux_slave_class_interface_number = 2,
      ux_slave_class_configuration_number = 1,
      ux_slave_class_interface = 0x2001E478 -> (
        ux_slave_interface_status = 1,
        ux_slave_interface_class = 0x2001E06C,
        ux_slave_interface_class_instance = 0x2001F410,
        ux_slave_interface_descriptor = (
          bLength = 9,
          bDescriptorType = 4,
          bInterfaceNumber = 3,
          bAlternateSetting = 0,
          bNumEndpoints = 2,
          bInterfaceClass = 10,
          bInterfaceSubClass = 0,
          bInterfaceProtocol = 0,
          iInterface = 0),
        ux_slave_interface_next_interface = 0x2001E4B0,
        ux_slave_interface_first_endpoint = 0x2001E750))
      ux_slave_class_name = "PRINTER",
      ux_slave_class_status = 1,
      ux_slave_class_entry_function = 0x30010891,
      ux_slave_class_instance = 0x2001FE70,
      ux_slave_class_client = 0x0,
      ux_slave_class_thread = (tx_thread_id = 0, tx_thread_run_count = 0, tx_th
      ux_slave_class_thread_stack = 0x0,
      ux_slave_class_interface_parameter = 0x2003FECC,
      ux_slave_class_interface_number = 4,
      ux_slave_class_configuration_number = 1,
      ux_slave_class_interface = 0x2001E4B0 -> (
        ux_slave_interface_status = 1,
        ux_slave_interface_class = 0x2001E188,
        ux_slave_interface_class_instance = 0x2001FE70,
        ux_slave_interface_descriptor = (
          bLength = 9,
          bDescriptorType = 4,
          bInterfaceNumber = 4,
          bAlternateSetting = 0,
          bNumEndpoints = 2,
          bInterfaceClass = 7,
          bInterfaceSubClass = 1,
          bInterfaceProtocol = 2,
          iInterface = 9),
        ux_slave_interface_next_interface = 0x0,
        ux_slave_interface_first_endpoint = 0x2001E878))

I hope this helps to understand the problem.

Marco

xiaocq2001 commented 1 year ago

How about following check:

                    if (((request_index & 0xFF) != class_index) &&
                        !((class_ptr -> ux_slave_class_interface -> ux_slave_interface_descriptor.bInterfaceClass == 0x07) &&
                         (request == 0x00) &&
                         *(transfer_request -> ux_slave_transfer_request_setup + UX_SETUP_INDEX + 1) == class_index))
                        continue;

In case class index not match, the printer GET_DEVICE_ID case should be checked, if class index not match AND NOT printer GET_DEVICE_ID case, check next control.

MarcoTrap commented 1 year ago

Now it works! Will you add this fix in the next release?

Thanks!

xiaocq2001 commented 1 year ago

Sure, it will be fixed in next release.

xiaocq2001 commented 1 year ago

I updated the code a bit for a better view, as following:

                    if ((request_type == 0xA1) && (request == 0x00) &&
                        (class_ptr -> ux_slave_class_interface -> ux_slave_interface_descriptor.bInterfaceClass == 0x07))
                    {

                        /* Check wIndex high byte.  */
                        if(*(transfer_request -> ux_slave_transfer_request_setup + UX_SETUP_INDEX + 1) != class_index)
                            continue;
                    }
                    else
                    {

                        /* Check wIndex low.  */
                        if ((request_index & 0xFF) != class_index)
                            continue;
                    }

In case request type and request is printer GET_DEVICE_ID the wIndex high byte is checked, otherwise wIndex low byte is checked as interface target.

MarcoTrap commented 1 year ago

Yes, it's better and more understandable 👍