Merge pull request #182 from espressif/usb/msc/speed_optimization

USB MSC: speed optimization

usb/esp_modem_usb_dte/idf_component.yml

@@ -1,9 +1,9 @@
 ## IDF Component Manager Manifest File
-version: "1.1.0"
+version: "1.1.0~1"
 description: USB DTE plugin for esp_modem component
 url: https://github.com/espressif/idf-extra-components/tree/master/usb/esp_modem_usb_dte
 
 dependencies:
   idf: ">=4.4"
   usb_host_cdc_acm: "2.*"
-  esp_modem: "^0.1.28"
+  esp_modem: ">=0.1.28,<2.0.0"

usb/test_app/main/usb_test_main.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -12,7 +12,7 @@
 static size_t before_free_8bit;
 static size_t before_free_32bit;
 
-#define TEST_MEMORY_LEAK_THRESHOLD (-10000) // @todo MSC test are leaking memory
+#define TEST_MEMORY_LEAK_THRESHOLD (-530)
 static void check_leak(size_t before_free, size_t after_free, const char *type)
 {
     ssize_t delta = after_free - before_free;

usb/test_app/pytest_usb_host.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: CC0-1.0
 
 from typing import Tuple
@@ -23,10 +23,7 @@ def test_usb_host(dut: Tuple[IdfDut, IdfDut]) -> None:
     device.expect_exact('USB initialization DONE')
 
     # 1.2 Run CDC test
-    host.expect_exact('Press ENTER to see the list of tests.')
-    host.write('[cdc_acm]')
-    host.expect_unity_test_output()
-    host.expect_exact("Enter next test, or 'enter' to see menu")
+    host.run_all_single_board_cases(group='cdc_acm')
 
     # 2.1 Prepare USB device for MSC test
     device.serial.hard_reset()
@@ -35,9 +32,7 @@ def test_usb_host(dut: Tuple[IdfDut, IdfDut]) -> None:
     device.expect_exact('USB initialization DONE')
 
     # 2.2 Run MSC test
-    host.write('[usb_msc]')
-    host.expect_unity_test_output()
-    host.expect_exact("Enter next test, or 'enter' to see menu")
+    host.run_all_single_board_cases(group='usb_msc')
 
     # 3.1 Prepare USB device with one Interface for HID tests
     device.serial.hard_reset()
@@ -46,9 +41,7 @@ def test_usb_host(dut: Tuple[IdfDut, IdfDut]) -> None:
     device.expect_exact('HID mock device with 1xInterface (Protocol=None) has been started')
 
     # 3.2 Run HID tests
-    host.write('[hid_host]')
-    host.expect_unity_test_output()
-    host.expect_exact("Enter next test, or 'enter' to see menu")
+    host.run_all_single_board_cases(group='hid_host')
 
     # 3.3 Prepare USB device with two Interfaces for HID tests
     device.serial.hard_reset()
@@ -57,6 +50,4 @@ def test_usb_host(dut: Tuple[IdfDut, IdfDut]) -> None:
     device.expect_exact('HID mock device with 2xInterfaces (Protocol=BootKeyboard, Protocol=BootMouse) has been started')
 
     # 3.4 Run HID tests
-    host.write('[hid_host]')
-    host.expect_unity_test_output()
-    host.expect_exact("Enter next test, or 'enter' to see menu")
+    host.run_all_single_board_cases(group='hid_host')

usb/usb_host_msc/CHANGELOG.md

@@ -0,0 +1,23 @@
+## 1.0.0
+
+- Initial version
+
+## 1.0.1
+
+- Fix compatibility with IDF v4.4
+
+## 1.0.2
+
+- Increase transfer timeout to 5 seconds to handle slower flash disks
+
+## 1.0.4
+
+- Claim support for USB composite devices
+
+## 1.1.0
+
+- Significantly increase performance with Virtual File System by allowing longer transfers
+- Optimize used heap memory by reusing the Virtual File System buffer
+- Optimize CPU usage by putting the background MSC task to 'Blocked' state indefinitely when there is nothing to do
+- Fix MSC commands for devices on interface numbers other than zero
+- Replace unsafe debug functions for direct access of MSC sectors with private SCSI commands

usb/usb_host_msc/CMakeLists.txt

@@ -4,6 +4,10 @@ set(sources src/msc_scsi_bot.c
             src/msc_host_vfs.c)
 
 idf_component_register( SRCS ${sources}
-                        INCLUDE_DIRS include
-                        PRIV_INCLUDE_DIRS private_include
+                        INCLUDE_DIRS include include/usb # 'include/usb' is here for backwards compatibility
+                        PRIV_INCLUDE_DIRS private_include include/esp_private
                         REQUIRES usb fatfs )
+
+# We access packeted USB string descriptor via pointers. The memory used for storing the descritptors
+# allows unaligned access, so this is not an issue
+target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-address-of-packed-member)

usb/usb_host_msc/idf_component.yml

@@ -1,9 +1,9 @@
-version: "1.0.4"
+version: "1.1.0"
 description: USB Host MSC driver
 url: https://github.com/espressif/idf-extra-components/tree/master/usb/usb_host_msc
 
 dependencies:
-  idf: ">=4.4"
+  idf: ">=4.4.1"
 
 targets:
   - esp32s2

usb/usb_host_msc/private_include/msc_scsi_bot.h → usb/usb_host_msc/include/esp_private/msc_scsi_bot.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -8,7 +8,7 @@
 
 #include <stdint.h>
 #include "esp_err.h"
-#include "msc_common.h"
+#include "usb/msc_host.h"
 
 #ifdef __cplusplus
 extern "C"
@@ -21,35 +21,31 @@ typedef struct {
     uint8_t code_q;
 } scsi_sense_data_t;
 
-esp_err_t scsi_cmd_read10(msc_device_t *device,
+esp_err_t scsi_cmd_read10(msc_host_device_handle_t device,
                           uint8_t *data,
                           uint32_t sector_address,
                           uint32_t num_sectors,
                           uint32_t sector_size);
 
-esp_err_t scsi_cmd_write10(msc_device_t *device,
+esp_err_t scsi_cmd_write10(msc_host_device_handle_t device,
                            const uint8_t *data,
                            uint32_t sector_address,
                            uint32_t num_sectors,
                            uint32_t sector_size);
 
-esp_err_t scsi_cmd_read_capacity(msc_device_t *device,
+esp_err_t scsi_cmd_read_capacity(msc_host_device_handle_t device,
                                  uint32_t *block_size,
                                  uint32_t *block_count);
 
-esp_err_t scsi_cmd_sense(msc_device_t *device, scsi_sense_data_t *sense);
+esp_err_t scsi_cmd_sense(msc_host_device_handle_t device, scsi_sense_data_t *sense);
 
-esp_err_t scsi_cmd_unit_ready(msc_device_t *device);
+esp_err_t scsi_cmd_unit_ready(msc_host_device_handle_t device);
 
-esp_err_t scsi_cmd_inquiry(msc_device_t *device);
+esp_err_t scsi_cmd_inquiry(msc_host_device_handle_t device);
 
-esp_err_t scsi_cmd_prevent_removal(msc_device_t *device, bool prevent);
+esp_err_t scsi_cmd_prevent_removal(msc_host_device_handle_t device, bool prevent);
 
-esp_err_t scsi_cmd_mode_sense(msc_device_t *device);
-
-esp_err_t msc_mass_reset(msc_device_t *device);
-
-esp_err_t msc_get_max_lun(msc_device_t *device, uint8_t *lun);
+esp_err_t scsi_cmd_mode_sense(msc_host_device_handle_t device);
 
 #ifdef __cplusplus
 }

usb/usb_host_msc/include/msc_host.h → usb/usb_host_msc/include/usb/msc_host.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -9,7 +9,7 @@
 #include <wchar.h>
 #include <stdint.h>
 #include "esp_err.h"
-#include <freertos/FreeRTOS.h>
+#include "freertos/FreeRTOS.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -19,6 +19,7 @@ extern "C" {
 #define ESP_ERR_MSC_MOUNT_FAILED    (ESP_ERR_MSC_HOST_BASE + 1)  /*!< Failed to mount storage */
 #define ESP_ERR_MSC_FORMAT_FAILED   (ESP_ERR_MSC_HOST_BASE + 2)  /*!< Failed to format storage */
 #define ESP_ERR_MSC_INTERNAL        (ESP_ERR_MSC_HOST_BASE + 3)  /*!< MSC host internal error */
+#define ESP_ERR_MSC_STALL           (ESP_ERR_MSC_HOST_BASE + 4)  /*!< USB transfer stalled */
 
 #define MSC_STR_DESC_SIZE 32
 
@@ -117,13 +118,14 @@ esp_err_t msc_host_uninstall_device(msc_host_device_handle_t device);
  * @param[in]  size   Number of bytes to be read
  * @return esp_err_t
  */
-esp_err_t msc_host_read_sector(msc_host_device_handle_t device, size_t sector, void *data, size_t size);
+esp_err_t msc_host_read_sector(msc_host_device_handle_t device, size_t sector, void *data, size_t size)
+__attribute__((deprecated("use API from esp_private/msc_scsi_bot.h")));
 
 /**
  * @brief Helper function for writing sector to mass storage device.
  *
  * @warning This call is not thread safe and should not be combined
- *          with accesses to storare through file system.
+ *          with accesses to storage through file system.
  *
  * @note  Provided sector and size cannot exceed
  *        sector_count and sector_size obtained from msc_host_device_info_t
@@ -134,7 +136,8 @@ esp_err_t msc_host_read_sector(msc_host_device_handle_t device, size_t sector, v
  * @param[in]  size   Number of bytes to be written
  * @return esp_err_t
  */
-esp_err_t msc_host_write_sector(msc_host_device_handle_t device, size_t sector, const void *data, size_t size);
+esp_err_t msc_host_write_sector(msc_host_device_handle_t device, size_t sector, const void *data, size_t size)
+__attribute__((deprecated("use API from esp_private/msc_scsi_bot.h")));
 
 /**
  * @brief Handle MSC HOST events.
@@ -147,9 +150,6 @@ esp_err_t msc_host_handle_events(uint32_t timeout_ms);
 /**
  * @brief Gets devices information.
  *
- * @warning This call is not thread safe and should not be combined
- *          with accesses to storare through file system.
- *
  * @param[in]  device  Handle to device
  * @param[out] info  Structure to be populated with device info
  * @return esp_err_t
@@ -164,6 +164,18 @@ esp_err_t msc_host_get_device_info(msc_host_device_handle_t device, msc_host_dev
  */
 esp_err_t msc_host_print_descriptors(msc_host_device_handle_t device);
 
+/**
+ * @brief MSC Bulk Only Transport Reset Recovery
+ *
+ * @see USB Mass Storage Class – Bulk Only Transport, Chapter 5.3.4
+ *
+ * @param[in] device Handle of MSC device
+ * @return
+ *       - ESP_OK:  The device was recovered from reset
+ *       - ESP_FAI: Recovery unsuccessful, might indicate broken device
+ */
+esp_err_t msc_host_reset_recovery(msc_host_device_handle_t device);
+
 #ifdef __cplusplus
 }
 #endif //__cplusplus

usb/usb_host_msc/include/msc_host_vfs.h → usb/usb_host_msc/include/usb/msc_host_vfs.h

@@ -1,13 +1,13 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #pragma once
 
 #include "esp_vfs_fat.h"
-#include "msc_host.h"
+#include "usb/msc_host.h"
 #include "esp_err.h"
 
 #ifdef __cplusplus

usb/usb_host_msc/private_include/msc_common.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -34,17 +34,43 @@ typedef struct {
 
 typedef struct msc_host_device {
     STAILQ_ENTRY(msc_host_device) tailq_entry;
-    usb_transfer_status_t transfer_status;
     SemaphoreHandle_t transfer_done;
     usb_device_handle_t handle;
     usb_transfer_t *xfer;
     msc_config_t config;
     usb_disk_t disk;
 } msc_device_t;
 
-esp_err_t msc_bulk_transfer(msc_device_t *device_handle, uint8_t *data, size_t size, msc_endpoint_t ep);
+/**
+ * @brief Trigger a BULK transfer to device: zero copy
+ *
+ * Data buffer ownership is transferred to the MSC driver and the application cannot access it before the transfer finishes.
+ * The buffer must be DMA capable, as it is going to be accessed by USB DMA.
+ *
+ * This function significantly improves performance with usage of Virtual File System, which creates a intermediate buffer for each opened file.
+ * The intermediate VFS buffer is then used for USB transfers too, which eliminates need of 2 large buffers and unnecessary copying of the data.
+ * The user can set size of the VFS buffer with setvbuf() function.
+ *
+ * @param[in]    device_handle MSC device handle
+ * @param[inout] data          Data buffer. Direction depends on 'ep'. Must be DMA capable.
+ * @param[in]    size          Size of buffer in bytes
+ * @param[in]    ep            Direction of the transfer
+ * @return esp_err_t
+ */
+esp_err_t msc_bulk_transfer_zcpy(msc_device_t *device_handle, uint8_t *data, size_t size, msc_endpoint_t ep);
+
+/**
+ * @brief Trigger a CTRL transfer to device
+ *
+ * The request and data must be filled by accessing private device_handle->xfer before calling this function
+ *
+ * @param[in] device_handle MSC device handle
+ * @param[in] len           Length of the transfer
+ * @return esp_err_t
+ */
+esp_err_t msc_control_transfer(msc_device_t *device_handle, size_t len);
 
-esp_err_t msc_control_transfer(msc_device_t *device_handle, usb_transfer_t *xfer, size_t len);
+esp_err_t clear_feature(msc_device_t *device, uint8_t endpoint);
 
 #define MSC_GOTO_ON_ERROR(exp) ESP_GOTO_ON_ERROR(exp, fail, TAG, "")
 

usb/usb_host_msc/src/diskio_usb.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -32,18 +32,14 @@ static DRESULT usb_disk_read (BYTE pdrv, BYTE *buff, DWORD sector, UINT count)
     assert(pdrv < FF_VOLUMES);
     assert(s_disks[pdrv]);
 
-    esp_err_t err;
     usb_disk_t *disk = s_disks[pdrv];
     size_t sector_size = disk->block_size;
     msc_device_t *dev = __containerof(disk, msc_device_t, disk);
 
-    for (int i = 0; i < count; i++) {
-        err = scsi_cmd_read10(dev, &buff[i * sector_size], sector + i, 1, sector_size);
-        if (err != ESP_OK) {
-            ESP_LOGE(TAG, "scsi_cmd_read10 failed (%d)", err);
-            return RES_ERROR;
-        }
-
+    esp_err_t err = scsi_cmd_read10(dev, buff, sector, count, sector_size);
+    if (err != ESP_OK) {
+        ESP_LOGE(TAG, "scsi_cmd_read10 failed (%d)", err);
+        return RES_ERROR;
     }
 
     return RES_OK;
@@ -54,18 +50,14 @@ static DRESULT usb_disk_write (BYTE pdrv, const BYTE *buff, DWORD sector, UINT c
     assert(pdrv < FF_VOLUMES);
     assert(s_disks[pdrv]);
 
-    esp_err_t err;
     usb_disk_t *disk = s_disks[pdrv];
     size_t sector_size = disk->block_size;
     msc_device_t *dev = __containerof(disk, msc_device_t, disk);
 
-    for (int i = 0; i < count; i++) {
-        err = scsi_cmd_write10(dev, &buff[i * sector_size], sector + i, 1, sector_size);
-        if (err != ESP_OK) {
-            ESP_LOGE(TAG, "scsi_cmd_write10 failed (%d)", err);
-            return RES_ERROR;
-        }
-
+    esp_err_t err = scsi_cmd_write10(dev, buff, sector, count, sector_size);
+    if (err != ESP_OK) {
+        ESP_LOGE(TAG, "scsi_cmd_write10 failed (%d)", err);
+        return RES_ERROR;
     }
     return RES_OK;
 }