usb: HCD multiple fixes

This commit fixes the following bugs in the HCD

- HCD control pipes fill incorrect transfer size

_buffer_fill_ctrl() would fill the transfer descriptors length with
wLength instead of transfer->num_bytes. Therefore, the remaining length
would be incorrect when a control transfer requests more bytes than are
available.

- Fix USB_TRANSFER_FLAG_ZERO_PACK behavior

The previous behavior of USB_TRANSFER_FLAG_ZERO_PACK was incorrect, and did not
support interrupt pipes. A zero length packet can now be added to Bulk/Interrupt
OUT transfers where the length is a multiple of the endpoint's MPS.

- Fixed HCD port suspend and resume test case

Halting a control pipe mid control transfer can lead some test devices to get stuck

components/usb/hcd.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -194,12 +194,13 @@ typedef struct {
             uint32_t reserved28: 28;
         } ctrl;                             //Control transfer related
         struct {
-            uint32_t zero_len_packet: 1;    //Bulk transfer should add a zero length packet at the end regardless
+            uint32_t zero_len_packet: 1;    //Added a zero length packet, so transfer consists of 2 QTDs
             uint32_t reserved31: 31;
         } bulk;                             //Bulk transfer related
         struct {
-            uint32_t num_qtds: 8;           //Number of transfer descriptors filled
+            uint32_t num_qtds: 8;           //Number of transfer descriptors filled (excluding zero length packet)
-            uint32_t reserved24: 24;
+            uint32_t zero_len_packet: 1;    //Added a zero length packet, so true number descriptors is num_qtds + 1
+            uint32_t reserved23: 23;
         } intr;                             //Interrupt transfer related
         struct {
             uint32_t num_qtds: 8;           //Number of transfer descriptors filled (including NULL descriptors)
@@ -2082,8 +2083,8 @@ static inline void _buffer_fill_ctrl(dma_buffer_block_t *buffer, usb_transfer_t
         //Not data stage. Fill with an empty descriptor
         usbh_hal_xfer_desc_clear(buffer->xfer_desc_list, 1);
     } else {
-        //Fill data stage
+        //Fill data stage. Note that we still fill with transfer->num_bytes instead of setup_pkt->wLength as it's possible to require more bytes than wLength
-        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, transfer->data_buffer + sizeof(usb_setup_packet_t), setup_pkt->wLength,
+        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, transfer->data_buffer + sizeof(usb_setup_packet_t), transfer->num_bytes - sizeof(usb_setup_packet_t),
                                 ((data_stg_in) ? USBH_HAL_XFER_DESC_FLAG_IN : 0) | USBH_HAL_XFER_DESC_FLAG_HOC);
     }
     //Fill status stage (i.e., a zero length packet). If data stage is skipped, the status stage is always IN.
@@ -2095,46 +2096,68 @@ static inline void _buffer_fill_ctrl(dma_buffer_block_t *buffer, usb_transfer_t
     buffer->flags.ctrl.cur_stg = 0;
 }
 
-static inline void _buffer_fill_bulk(dma_buffer_block_t *buffer, usb_transfer_t *transfer, bool is_in)
+static inline void _buffer_fill_bulk(dma_buffer_block_t *buffer, usb_transfer_t *transfer, bool is_in, int mps)
 {
+    //Only add a zero length packet if OUT, flag is set, and transfer length is multiple of EP's MPS
+    //Minor optimization: Do the mod operation last
+    bool zero_len_packet = !is_in && (transfer->flags & USB_TRANSFER_FLAG_ZERO_PACK) && (transfer->num_bytes % mps == 0);
     if (is_in) {
         usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes,
                                 USBH_HAL_XFER_DESC_FLAG_IN | USBH_HAL_XFER_DESC_FLAG_HOC);
-    } else if (transfer->flags & USB_TRANSFER_FLAG_ZERO_PACK) {
+    } else { //OUT
-        //We need to add an extra zero length packet, so two descriptors are used
+        if (zero_len_packet) {
-        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, 0);
+            //Adding a zero length packet, so two descriptors are used.
-        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, NULL, 0, USBH_HAL_XFER_DESC_FLAG_HOC);
+            usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, 0);
-    } else {
+            usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, NULL, 0, USBH_HAL_XFER_DESC_FLAG_HOC);
-        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, USBH_HAL_XFER_DESC_FLAG_HOC);
+        } else {
+            //Zero length packet not required. One descriptor is enough
+            usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, USBH_HAL_XFER_DESC_FLAG_HOC);
+        }
     }
     //Update buffer flags
-    buffer->flags.bulk.zero_len_packet = (is_in && (transfer->flags & USB_TRANSFER_FLAG_ZERO_PACK)) ? 1 : 0;
+    buffer->flags.bulk.zero_len_packet = zero_len_packet;
 }
 
 static inline void _buffer_fill_intr(dma_buffer_block_t *buffer, usb_transfer_t *transfer, bool is_in, int mps)
 {
     int num_qtds;
+    int mod_mps = transfer->num_bytes % mps;
+    //Only add a zero length packet if OUT, flag is set, and transfer length is multiple of EP's MPS
+    bool zero_len_packet = !is_in && (transfer->flags & USB_TRANSFER_FLAG_ZERO_PACK) && (mod_mps == 0);
     if (is_in) {
-        assert(transfer->num_bytes % mps == 0);  //IN transfers MUST be integer multiple of MPS
+        assert(mod_mps == 0);  //IN transfers MUST be integer multiple of MPS
-        num_qtds = transfer->num_bytes / mps;
+        num_qtds = transfer->num_bytes / mps;   //Can just floor divide as it's already multiple of MPS
     } else {
-        num_qtds = transfer->num_bytes / mps;    //Floor division for number of MPS packets
+        num_qtds = transfer->num_bytes / mps;   //Floor division to get the number of MPS sized packets
-        if (transfer->num_bytes % transfer->num_bytes > 0) {
+        if (mod_mps > 0) {
-            num_qtds++; //For the last shot packet
+            num_qtds++; //Add a short packet for the remainder
         }
     }
-    assert(num_qtds <= XFER_LIST_LEN_INTR);
+    assert((zero_len_packet) ? num_qtds + 1 : num_qtds <= XFER_LIST_LEN_INTR); //Check that the number of QTDs doesn't exceed the QTD list's length
-    //Fill all but last descriptor
+
+    uint32_t xfer_desc_flags = (is_in) ? USBH_HAL_XFER_DESC_FLAG_IN : 0;
     int bytes_filled = 0;
+    //Fill all but last QTD
     for (int i = 0; i < num_qtds - 1; i++) {
-        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, i, &transfer->data_buffer[bytes_filled], mps, (is_in) ? USBH_HAL_XFER_DESC_FLAG_IN : 0);
+        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, i, &transfer->data_buffer[bytes_filled], mps, xfer_desc_flags);
         bytes_filled += mps;
     }
-    //Fill in the last descriptor with HOC flag
+    //Fill last QTD and zero length packet
-    usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, num_qtds - 1, &transfer->data_buffer[bytes_filled], transfer->num_bytes - bytes_filled,
+    if (zero_len_packet) {
-                            ((is_in) ? USBH_HAL_XFER_DESC_FLAG_IN : 0) | USBH_HAL_XFER_DESC_FLAG_HOC);
+        //Fill in last data packet without HOC flag
+        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, num_qtds - 1, &transfer->data_buffer[bytes_filled], transfer->num_bytes - bytes_filled,
+                                xfer_desc_flags);
+        //HOC flag goes to zero length packet instead
+        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, num_qtds, NULL, 0, USBH_HAL_XFER_DESC_FLAG_HOC);
+    } else {
+        //Zero length packet not required. Fill in last QTD with HOC flag
+        usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, num_qtds - 1, &transfer->data_buffer[bytes_filled], transfer->num_bytes - bytes_filled,
+                                xfer_desc_flags | USBH_HAL_XFER_DESC_FLAG_HOC);
+    }
+
     //Update buffer members and flags
     buffer->flags.intr.num_qtds = num_qtds;
+    buffer->flags.intr.zero_len_packet = zero_len_packet;
 }
 
 static inline void _buffer_fill_isoc(dma_buffer_block_t *buffer, usb_transfer_t *transfer, bool is_in, int mps, int interval, int start_idx)
@@ -2220,7 +2243,7 @@ static void _buffer_fill(pipe_t *pipe)
             break;
         }
         case USB_PRIV_XFER_TYPE_BULK: {
-            _buffer_fill_bulk(buffer_to_fill, transfer, is_in);
+            _buffer_fill_bulk(buffer_to_fill, transfer, is_in, mps);
             break;
         }
         case USB_PRIV_XFER_TYPE_INTR: {
@@ -2269,7 +2292,7 @@ static void _buffer_exec(pipe_t *pipe)
         }
         case USB_PRIV_XFER_TYPE_INTR: {
             start_idx = 0;
-            desc_list_len = buffer_to_exec->flags.intr.num_qtds;
+            desc_list_len = (buffer_to_exec->flags.intr.zero_len_packet) ? buffer_to_exec->flags.intr.num_qtds + 1 : buffer_to_exec->flags.intr.num_qtds;
             break;
         }
         default: {
@@ -2389,7 +2412,7 @@ static inline void _buffer_parse_intr(dma_buffer_block_t *buffer, bool is_in, in
             transfer->actual_num_bytes = transfer->num_bytes - last_packet_rem_len;
         }
     } else {
-        //OUT INTR transfers can only complete successfully if all MPS packets have been transmitted. Double check
+        //OUT INTR transfers can only complete successfully if all packets have been transmitted. Double check
         for (int i = 0 ; i < buffer->flags.intr.num_qtds; i++) {
             int rem_len;
             int desc_status;

components/usb/include/usb/usb_types_stack.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -138,6 +138,29 @@ struct usb_transfer_s{
     usb_isoc_packet_desc_t isoc_packet_desc[0];     /**< Descriptors for each Isochronous packet */
 };
 
+/**
+ * @brief Terminate Bulk/Interrupt OUT transfer with a zero length packet
+ *
+ * OUT transfers normally terminate when the Host has transferred the exact amount of data it needs to the device.
+ * However, for bulk and interrupt OUT transfers, if the transfer size just happened to be a multiple of MPS, it will be
+ * impossible to know the boundary between two consecutive transfers to the same endpoint.
+ *
+ * Therefore, this flag will cause the transfer to automatically add a zero length packet (ZLP) at the end of the
+ * transfer if the following conditions are met:
+ * - The target endpoint is a Bulk/Interrupt OUT endpoint (Host to device)
+ * - The transfer's length (i.e., transfer.num_bytes) is a multiple of the endpoint's MPS
+ *
+ * Otherwise, this flag has no effect.
+ *
+ * Users should check whether their target device's class requires a ZLP, as not all Bulk/Interrupt OUT endpoints
+ * require them. For example:
+ * - For MSC Bulk Only Transport class, the Host MUST NEVER send a ZLP. Bulk transfer boundaries are determined by the CBW and CSW instead
+ * - For CDC Ethernet, the Host MUST ALWAYS send a ZLP if a segment (i.e., a transfer) is a multiple of MPS (See 3.3.1 Segment Delineation)
+ *
+ * @note See USB2.0 specification 5.7.3 and 5.8.3 for more details
+ * @note IN transfers normally terminate when the Host as receive the exact amount of data it needs (must be multiple of MPS)
+ *       or the endpoint sends a short packet to the Host
+ */
 #define USB_TRANSFER_FLAG_ZERO_PACK  0x01           /**< (For bulk OUT only). Indicates that a bulk OUT transfers should always terminate with a short packet, even if it means adding an extra zero length packet */
 
 #ifdef __cplusplus

components/usb/test/hcd/test_hcd_port.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -118,20 +118,19 @@ Test port suspend and resume with active pipes
 
 Purpose:
     - Test port suspend and resume procedure
-    - When suspending, the pipes should be halted before suspending the port. Any pending transfers should remain pending
+    - When suspending, the pipes should be halted before suspending the port
     - When resuming, the pipes should remain in the halted state
-    - Pipes on being cleared of the halt should resume transferring the pending transfers
 
 Procedure:
     - Setup the HCD and a port
     - Trigger a port connection
     - Create a default pipe
-    - Start transfers
+    - Test that port can't be suspended with an active pipe
     - Halt the default pipe after a short delay
     - Suspend the port
     - Resume the port
-    - Check that all the URBs have either completed successfully or been canceled by the pipe halt
+    - Check that all the pipe is still halted
-    - Cleanup URBs and default pipe
+    - Cleanup default pipe
     - Trigger disconnection and teardown
 */
 TEST_CASE("Test HCD port suspend and resume", "[hcd][ignore]")
@@ -142,26 +141,15 @@ TEST_CASE("Test HCD port suspend and resume", "[hcd][ignore]")
 
     //Allocate some URBs and initialize their data buffers with control transfers
     hcd_pipe_handle_t default_pipe = test_hcd_pipe_alloc(port_hdl, NULL, TEST_DEV_ADDR, port_speed); //Create a default pipe (using a NULL EP descriptor)
-    urb_t *urb_list[NUM_URBS];
-    for (int i = 0; i < NUM_URBS; i++) {
-        urb_list[i] = test_hcd_alloc_urb(0, URB_DATA_BUFF_SIZE);
-        //Initialize with a "Get Config Descriptor request"
-        urb_list[i]->transfer.num_bytes = sizeof(usb_setup_packet_t) + TRANSFER_MAX_BYTES;
-        USB_SETUP_PACKET_INIT_GET_CONFIG_DESC((usb_setup_packet_t *)urb_list[i]->transfer.data_buffer, 0, TRANSFER_MAX_BYTES);
-        urb_list[i]->transfer.context = (void *)0xDEADBEEF;
-    }
 
-    //Enqueue URBs but immediately suspend the port
+    //Test that suspending the port now fails as there is an active pipe
-    printf("Enqueuing URBs\n");
+    TEST_ASSERT_NOT_EQUAL(ESP_OK, hcd_port_command(port_hdl, HCD_PORT_CMD_SUSPEND));
-    for (int i = 0; i < NUM_URBS; i++) {
+
-        TEST_ASSERT_EQUAL(ESP_OK, hcd_urb_enqueue(default_pipe, urb_list[i]));
-    }
-    //Add a short delay to let the transfers run for a bit
-    esp_rom_delay_us(POST_ENQUEUE_DELAY_US);
     //Halt the default pipe before suspending
     TEST_ASSERT_EQUAL(HCD_PIPE_STATE_ACTIVE, hcd_pipe_get_state(default_pipe));
     TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_HALT));
     TEST_ASSERT_EQUAL(HCD_PIPE_STATE_HALTED, hcd_pipe_get_state(default_pipe));
+
     //Suspend the port
     TEST_ASSERT_EQUAL(ESP_OK, hcd_port_command(port_hdl, HCD_PORT_CMD_SUSPEND));
     TEST_ASSERT_EQUAL(HCD_PORT_STATE_SUSPENDED, hcd_port_get_state(port_hdl));
@@ -172,30 +160,12 @@ TEST_CASE("Test HCD port suspend and resume", "[hcd][ignore]")
     TEST_ASSERT_EQUAL(ESP_OK, hcd_port_command(port_hdl, HCD_PORT_CMD_RESUME));
     TEST_ASSERT_EQUAL(HCD_PORT_STATE_ENABLED, hcd_port_get_state(port_hdl));
     printf("Resumed\n");
+
     //Clear the default pipe's halt
     TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_CLEAR));
     TEST_ASSERT_EQUAL(HCD_PIPE_STATE_ACTIVE, hcd_pipe_get_state(default_pipe));
     vTaskDelay(pdMS_TO_TICKS(100)); //Give some time for resumed URBs to complete
-    //Dequeue URBs
-    for (int i = 0; i < NUM_URBS; i++) {
-        urb_t *urb = hcd_urb_dequeue(default_pipe);
-        TEST_ASSERT_EQUAL(urb_list[i], urb);
-        TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || urb->transfer.status == USB_TRANSFER_STATUS_CANCELED);
-        if (urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED) {
-            //We must have transmitted at least the setup packet, but device may return less than bytes requested
-            TEST_ASSERT_GREATER_OR_EQUAL(sizeof(usb_setup_packet_t), urb->transfer.actual_num_bytes);
-            TEST_ASSERT_LESS_OR_EQUAL(urb->transfer.num_bytes, urb->transfer.actual_num_bytes);
-        } else {
-            //A failed transfer should 0 actual number of bytes transmitted
-            TEST_ASSERT_EQUAL(0, urb->transfer.actual_num_bytes);
-        }
-        TEST_ASSERT_EQUAL(0xDEADBEEF, urb->transfer.context);
-    }
 
-    //Free URB list and pipe
-    for (int i = 0; i < NUM_URBS; i++) {
-        test_hcd_free_urb(urb_list[i]);
-    }
     test_hcd_pipe_free(default_pipe);
     //Cleanup
     test_hcd_wait_for_disconn(port_hdl, false);