diff --git a/components/usb/hcd.c b/components/usb/hcd.c index 589fe2da6b..f4d88f19c3 100644 --- a/components/usb/hcd.c +++ b/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 reserved24: 24; + uint32_t num_qtds: 8; //Number of transfer descriptors filled (excluding zero length packet) + 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 - usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, transfer->data_buffer + sizeof(usb_setup_packet_t), setup_pkt->wLength, + //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), 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) { - //We need to add an extra zero length packet, so two descriptors are used - usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, 0); - usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, NULL, 0, USBH_HAL_XFER_DESC_FLAG_HOC); - } else { - usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, USBH_HAL_XFER_DESC_FLAG_HOC); + } else { //OUT + if (zero_len_packet) { + //Adding a zero length packet, so two descriptors are used. + usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 0, transfer->data_buffer, transfer->num_bytes, 0); + usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, 1, NULL, 0, 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 - num_qtds = transfer->num_bytes / mps; + assert(mod_mps == 0); //IN transfers MUST be integer multiple of 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 - if (transfer->num_bytes % transfer->num_bytes > 0) { - num_qtds++; //For the last shot packet + num_qtds = transfer->num_bytes / mps; //Floor division to get the number of MPS sized packets + if (mod_mps > 0) { + num_qtds++; //Add a short packet for the remainder } } - assert(num_qtds <= XFER_LIST_LEN_INTR); - //Fill all but last descriptor + 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 + + 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 - usbh_hal_xfer_desc_fill(buffer->xfer_desc_list, num_qtds - 1, &transfer->data_buffer[bytes_filled], transfer->num_bytes - bytes_filled, - ((is_in) ? USBH_HAL_XFER_DESC_FLAG_IN : 0) | USBH_HAL_XFER_DESC_FLAG_HOC); + //Fill last QTD and zero length packet + if (zero_len_packet) { + //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; diff --git a/components/usb/include/usb/usb_types_stack.h b/components/usb/include/usb/usb_types_stack.h index 974d01def5..c35de488b6 100644 --- a/components/usb/include/usb/usb_types_stack.h +++ b/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 diff --git a/components/usb/test/hcd/test_hcd_port.c b/components/usb/test/hcd/test_hcd_port.c index d3e2678160..78f17b300b 100644 --- a/components/usb/test/hcd/test_hcd_port.c +++ b/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 - - Cleanup URBs and default pipe + - Check that all the pipe is still halted + - 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 - printf("Enqueuing URBs\n"); - 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); + //Test that suspending the port now fails as there is an active pipe + TEST_ASSERT_NOT_EQUAL(ESP_OK, hcd_port_command(port_hdl, HCD_PORT_CMD_SUSPEND)); + //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);