improve usbd control transfer

tinyusb/class/cdc/cdc_device.c

@@ -44,8 +44,9 @@
 //--------------------------------------------------------------------+
 // INCLUDE
 //--------------------------------------------------------------------+
-#include <common/tusb_common.h>
+#include "common/tusb_common.h"
 #include "cdc_device.h"
+#include "device/usbd_pvt.h"
 
 //--------------------------------------------------------------------+
 // MACRO CONSTANT TYPEDEF
@@ -221,13 +222,13 @@ tusb_error_t cdcd_control_request_subtask(uint8_t port, tusb_control_request_t c
 
   if (CDC_REQUEST_GET_LINE_CODING == p_request->bRequest)
   {
-    SUBTASK_INVOKE( usbd_control_xfer_substak(port, (tusb_dir_t) p_request->bmRequestType_bit.direction,
-                                                    (uint8_t*) &cdcd_line_coding[port], min16_of(sizeof(cdc_line_coding_t), p_request->wLength)), err );
+    SUBTASK_INVOKE( usbd_control_xfer_stask(port, (tusb_dir_t) p_request->bmRequestType_bit.direction,
+                                            (uint8_t*) &cdcd_line_coding[port], min16_of(sizeof(cdc_line_coding_t), p_request->wLength)), err );
   }
   else if (CDC_REQUEST_SET_LINE_CODING == p_request->bRequest)
   {
-    SUBTASK_INVOKE( usbd_control_xfer_substak(port, (tusb_dir_t) p_request->bmRequestType_bit.direction,
-                                                    (uint8_t*) &cdcd_line_coding[port], min16_of(sizeof(cdc_line_coding_t), p_request->wLength)), err );
+    SUBTASK_INVOKE( usbd_control_xfer_stask(port, (tusb_dir_t) p_request->bmRequestType_bit.direction,
+                                            (uint8_t*) &cdcd_line_coding[port], min16_of(sizeof(cdc_line_coding_t), p_request->wLength)), err );
     // TODO notify application on xfer complete
   }
   else if (CDC_REQUEST_SET_CONTROL_LINE_STATE == p_request->bRequest )
@@ -253,10 +254,12 @@ tusb_error_t cdcd_control_request_subtask(uint8_t port, tusb_control_request_t c
       // De-active --> disconnected
       p_cdc->connected = false;
     }
+
+    usbd_control_status(port, p_request->bmRequestType_bit.direction);
   }
   else
   {
-    SUBTASK_RETURN(TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT);
+    usbd_control_stall(port); // stall unsupported request
   }
 
   OSAL_SUBTASK_END

tinyusb/class/msc/msc_device.c

@@ -44,8 +44,9 @@
 //--------------------------------------------------------------------+
 // INCLUDE
 //--------------------------------------------------------------------+
-#include <common/tusb_common.h>
+#include "common/tusb_common.h"
 #include "msc_device.h"
+#include "device/usbd_pvt.h"
 
 //--------------------------------------------------------------------+
 // MACRO CONSTANT TYPEDEF
@@ -133,16 +134,16 @@ tusb_error_t mscd_control_request_subtask(uint8_t port, tusb_control_request_t c
 
   if(MSC_REQUEST_RESET == p_request->bRequest)
   {
-    usbd_control_status(port, TUSB_DIR_IN);
+    usbd_control_status(port, p_request->bmRequestType_bit.direction);
   }
   else if (MSC_REQUEST_GET_MAX_LUN == p_request->bRequest)
   {
     // Note: lpc11/13u need xfer data's address to be aligned 64 -> make use of scsi_data instead of using max_lun directly
     p_msc->scsi_data[0] = p_msc->max_lun;
-    SUBTASK_INVOKE( usbd_control_xfer_substak(port, TUSB_DIR_IN, p_msc->scsi_data, 1), err);
+    SUBTASK_INVOKE( usbd_control_xfer_stask(port, p_request->bmRequestType_bit.direction, p_msc->scsi_data, 1), err);
   }else
   {
-    SUBTASK_RETURN(TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT);
+    usbd_control_stall(port); // stall unsupported request
   }
 
   OSAL_SUBTASK_END

tinyusb/device/usbd.c

@@ -47,6 +47,7 @@
 //--------------------------------------------------------------------+
 #include "tusb.h"
 #include "usbd.h"
+#include "device/usbd_pvt.h"
 
 //--------------------------------------------------------------------+
 // MACRO CONSTANT TYPEDEF
@@ -112,8 +113,8 @@ enum { USBD_CLASS_DRIVER_COUNT = sizeof(usbd_class_drivers) / sizeof(usbd_class_
 //--------------------------------------------------------------------+
 // INTERNAL OBJECT & FUNCTION DECLARATION
 //--------------------------------------------------------------------+
-static tusb_error_t usbd_set_configure_received(uint8_t port, uint8_t config_number);
-static tusb_error_t get_descriptor(uint8_t port, tusb_control_request_t const * const p_request, uint8_t const ** pp_buffer, uint16_t * p_length);
+static tusb_error_t proc_set_config_req(uint8_t port, uint8_t config_number);
+static uint16_t get_descriptor(uint8_t port, tusb_control_request_t const * const p_request, uint8_t const ** pp_buffer);
 
 //--------------------------------------------------------------------+
 // APPLICATION INTERFACE
@@ -171,8 +172,8 @@ static osal_queue_t usbd_queue_hdl;
 //--------------------------------------------------------------------+
 // IMPLEMENTATION
 //--------------------------------------------------------------------+
-tusb_error_t usbd_control_request_subtask(uint8_t port, tusb_control_request_t const * const p_request);
-static tusb_error_t usbd_body_subtask(void);
+static tusb_error_t proc_control_request_stask(uint8_t port, tusb_control_request_t const * const p_request);
+static tusb_error_t usbd_main_stask(void);
 
 tusb_error_t usbd_init (void)
 {
@@ -216,11 +217,11 @@ void usbd_task( void* param)
   (void) param;
 
   OSAL_TASK_BEGIN
-  usbd_body_subtask();
+  usbd_main_stask();
   OSAL_TASK_END
 }
 
-static tusb_error_t usbd_body_subtask(void)
+static tusb_error_t usbd_main_stask(void)
 {
   static usbd_task_event_t event;
 
@@ -254,7 +255,7 @@ static tusb_error_t usbd_body_subtask(void)
 
   if ( USBD_EVENTID_SETUP_RECEIVED == event.event_id )
   {
-    SUBTASK_INVOKE( usbd_control_request_subtask(event.port, &event.setup_received), error );
+    SUBTASK_INVOKE( proc_control_request_stask(event.port, &event.setup_received), error );
   }else if (USBD_EVENTID_XFER_DONE == event.event_id)
   {
     // Call class handling function, Class that endpoint not belong to should check and return
@@ -286,7 +287,7 @@ static tusb_error_t usbd_body_subtask(void)
 //--------------------------------------------------------------------+
 // CONTROL REQUEST
 //--------------------------------------------------------------------+
-tusb_error_t usbd_control_xfer_substak(uint8_t port, tusb_dir_t dir, uint8_t * buffer, uint16_t length)
+tusb_error_t usbd_control_xfer_stask(uint8_t port, tusb_dir_t dir, uint8_t * buffer, uint16_t length)
 {
   OSAL_SUBTASK_BEGIN
 
@@ -302,34 +303,35 @@ tusb_error_t usbd_control_xfer_substak(uint8_t port, tusb_dir_t dir, uint8_t * b
   }
 
   // Status opposite direction with Zero Length
-  usbd_control_status(port, 1-dir);
-
-  // no need to blocking wait for status to complete
+  // No need to wait for status to complete therefore
+  // status phase must not call tusb_dcd_control_complete/tusb_dcd_xfer_complete
+  usbd_control_status(port, dir);
 
   OSAL_SUBTASK_END
 }
 
-tusb_error_t usbd_control_request_subtask(uint8_t port, tusb_control_request_t const * const p_request)
+static tusb_error_t proc_control_request_stask(uint8_t port, tusb_control_request_t const * const p_request)
 {
   OSAL_SUBTASK_BEGIN
 
   tusb_error_t error;
   error = TUSB_ERROR_NONE;
 
-  //------------- Standard Control e.g in enumeration -------------//
+  //------------- Standard Request e.g in enumeration -------------//
   if( TUSB_REQ_RCPT_DEVICE    == p_request->bmRequestType_bit.recipient &&
       TUSB_REQ_TYPE_STANDARD  == p_request->bmRequestType_bit.type )
   {
     if ( TUSB_REQ_GET_DESCRIPTOR == p_request->bRequest )
     {
-      uint8_t const * p_buffer = NULL;
-      uint16_t length = 0;
-
-      error = get_descriptor(port, p_request, &p_buffer, &length);
+      uint8_t  const * buffer = NULL;
+      uint16_t const   len    = get_descriptor(port, p_request, &buffer);
 
-      if ( TUSB_ERROR_NONE == error )
+      if ( len )
       {
-        SUBTASK_INVOKE ( usbd_control_xfer_substak(port, (tusb_dir_t) p_request->bmRequestType_bit.direction, (uint8_t*) p_buffer, length ), error );
+        SUBTASK_INVOKE( usbd_control_xfer_stask(port, p_request->bmRequestType_bit.direction, (uint8_t*) buffer, len ), error );
+      }else
+      {
+        usbd_control_stall(port); // stall unsupported descriptor
       }
     }
     else if ( TUSB_REQ_SET_ADDRESS == p_request->bRequest )
@@ -337,19 +339,21 @@ tusb_error_t usbd_control_request_subtask(uint8_t port, tusb_control_request_t c
       tusb_dcd_set_address(port, (uint8_t) p_request->wValue);
       usbd_devices[port].state = TUSB_DEVICE_STATE_ADDRESSED;
 
-      #ifdef NRF52840_XXAA
-      // nrf52 auto handle set address, no need to return status
-      SUBTASK_RETURN(TUSB_ERROR_NONE);
+      #ifndef NRF52840_XXAA // nrf52 auto handle set address, we must not return status
+      usbd_control_status(port, p_request->bmRequestType_bit.direction);
       #endif
     }
     else if ( TUSB_REQ_SET_CONFIGURATION == p_request->bRequest )
     {
-      usbd_set_configure_received(port, (uint8_t) p_request->wValue);
-    }else
+      proc_set_config_req(port, (uint8_t) p_request->wValue);
+      usbd_control_status(port, p_request->bmRequestType_bit.direction);
+    }
+    else
     {
-      error = TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT;
+      usbd_control_stall(port); // Stall unsupported request
     }
   }
+
   //------------- Class/Interface Specific Request -------------//
   else if ( TUSB_REQ_RCPT_INTERFACE == p_request->bmRequestType_bit.recipient)
   {
@@ -364,28 +368,28 @@ tusb_error_t usbd_control_request_subtask(uint8_t port, tusb_control_request_t c
       SUBTASK_INVOKE( usbd_class_drivers[class_code].control_request_subtask(port, p_request), error );
     }else
     {
-      error = TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT;
+      usbd_control_stall(port); // Stall unsupported request
     }
   }
 
   //------------- Endpoint Request -------------//
   else if ( TUSB_REQ_RCPT_ENDPOINT == p_request->bmRequestType_bit.recipient &&
-            TUSB_REQ_TYPE_STANDARD == p_request->bmRequestType_bit.type &&
-            TUSB_REQ_CLEAR_FEATURE == p_request->bRequest )
-  {
-    tusb_dcd_edpt_clear_stall(port, u16_low_u8(p_request->wIndex) );
-  } else
+            TUSB_REQ_TYPE_STANDARD == p_request->bmRequestType_bit.type)
   {
-    error = TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT;
+    if (TUSB_REQ_CLEAR_FEATURE == p_request->bRequest )
+    {
+      tusb_dcd_edpt_clear_stall(port, u16_low_u8(p_request->wIndex) );
+      usbd_control_status(port, p_request->bmRequestType_bit.direction);
+    } else
+    {
+      usbd_control_stall(port); // Stall unsupported request
+    }
   }
 
-  if(TUSB_ERROR_NONE != error)
-  {
-    // Response with Protocol Stall if request is not supported
-    tusb_dcd_edpt_stall(port, 0);
-  }else if (p_request->wLength == 0)
+  //------------- Unsupported Request -------------//
+  else
   {
-    usbd_control_status(port, 1-p_request->bmRequestType_bit.direction);
+    usbd_control_stall(port); // Stall unsupported request
   }
 
   OSAL_SUBTASK_END
@@ -393,7 +397,7 @@ tusb_error_t usbd_control_request_subtask(uint8_t port, tusb_control_request_t c
 
 // TODO Host (windows) can get HID report descriptor before set configured
 // may need to open interface before set configured
-static tusb_error_t usbd_set_configure_received(uint8_t port, uint8_t config_number)
+static tusb_error_t proc_set_config_req(uint8_t port, uint8_t config_number)
 {
   tusb_dcd_set_config(port, config_number);
   usbd_devices[port].state = TUSB_DEVICE_STATE_CONFIGURED;
@@ -437,53 +441,56 @@ static tusb_error_t usbd_set_configure_received(uint8_t port, uint8_t config_num
   return TUSB_ERROR_NONE;
 }
 
-static tusb_error_t get_descriptor(uint8_t port, tusb_control_request_t const * const p_request, uint8_t const ** pp_buffer, uint16_t * p_length)
+static uint16_t get_descriptor(uint8_t port, tusb_control_request_t const * const p_request, uint8_t const ** pp_buffer)
 {
   tusb_desc_type_t const desc_type = (tusb_desc_type_t) u16_high_u8(p_request->wValue);
   uint8_t const desc_index = u16_low_u8( p_request->wValue );
 
-  uint8_t const * p_data = NULL ;
+  uint8_t const * desc_data = NULL ;
+  uint16_t len = 0;
 
   switch(desc_type)
   {
     case TUSB_DESC_DEVICE:
-      p_data      = tusbd_descriptor_pointers.p_device;
-      (*p_length) = sizeof(tusb_descriptor_device_t);
+      desc_data = tusbd_descriptor_pointers.p_device;
+      len       = sizeof(tusb_descriptor_device_t);
     break;
 
     case TUSB_DESC_CONFIGURATION:
-      p_data      = tusbd_descriptor_pointers.p_configuration;
-      (*p_length) = ((tusb_descriptor_configuration_t*)tusbd_descriptor_pointers.p_configuration)->wTotalLength;
+      desc_data = tusbd_descriptor_pointers.p_configuration;
+      len       = ((tusb_descriptor_configuration_t*)tusbd_descriptor_pointers.p_configuration)->wTotalLength;
     break;
 
     case TUSB_DESC_STRING:
-      if ( !(desc_index < 100) ) return TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT; // windows sometimes ask for string at index 238 !!!
+      // windows sometimes ask for string at index 238 !!!
+      if ( !(desc_index < 100) ) return 0;
 
-      p_data = tusbd_descriptor_pointers.p_string_arr[desc_index];
-      ASSERT( p_data != NULL, TUSB_ERROR_FAILED);
+      desc_data = tusbd_descriptor_pointers.p_string_arr[desc_index];
+      VERIFY( desc_data != NULL, 0 );
 
-      (*p_length)  = p_data[0];  // first byte of descriptor is its size
+      len  = desc_data[0];  // first byte of descriptor is its size
     break;
 
     case TUSB_DESC_DEVICE_QUALIFIER:
       // TODO If not highspeed capable stall this request otherwise
       // return the descriptor that could work in highspeed
-      return TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT;
+      return 0;
     break;
 
     // TODO Report Descriptor (HID Generic)
     // TODO HID Descriptor
 
-    default: return TUSB_ERROR_DCD_CONTROL_REQUEST_NOT_SUPPORT;
+    default: return 0;
   }
 
-  (*p_length) = min16_of(p_request->wLength, (*p_length) ); // cannot return more than hosts requires
-  ASSERT( (*p_length) <= TUSB_CFG_DEVICE_ENUM_BUFFER_SIZE, TUSB_ERROR_NOT_ENOUGH_MEMORY);
+  // up to Host's length
+  len = min16_of(p_request->wLength, len );
+  ASSERT( len <= TUSB_CFG_DEVICE_ENUM_BUFFER_SIZE, TUSB_ERROR_NOT_ENOUGH_MEMORY);
 
-  memcpy(usbd_enum_buffer, p_data, (*p_length));
+  memcpy(usbd_enum_buffer, desc_data, len);
   (*pp_buffer) = usbd_enum_buffer;
 
-  return TUSB_ERROR_NONE;
+  return len;
 }
 //--------------------------------------------------------------------+
 // USBD-CLASS API

tinyusb/device/usbd.h

@@ -123,25 +123,6 @@ void tud_umount_cb(uint8_t port);
 
 //void tud_device_suspended_cb(uint8_t port);
 
-//--------------------------------------------------------------------+
-// CLASS-USBD & INTERNAL API
-//--------------------------------------------------------------------+
-#ifdef _TINY_USB_SOURCE_FILE_
-
-extern osal_semaphore_t usbd_control_xfer_sem_hdl;
-
-tusb_error_t usbd_control_xfer_substak(uint8_t port, tusb_dir_t dir, uint8_t * buffer, uint16_t length);
-
-static inline bool usbd_control_status(uint8_t port, tusb_dir_t dir)
-{
-  return tusb_dcd_control_xfer(port, dir, NULL, 0);
-}
-
-tusb_error_t usbd_init(void);
-void usbd_task( void* param);
-
-#endif
-
 #ifdef __cplusplus
  }
 #endif

tinyusb/device/usbd_pvt.h

@@ -0,0 +1,73 @@
+/**************************************************************************/
+/*!
+    @file     usbd_pvt.h
+    @author   hathach
+
+    @section LICENSE
+
+    Software License Agreement (BSD License)
+
+    Copyright (c) 2018, hathach (tinyusb.org)
+    All rights reserved.
+
+    Redistribution and use in source and binary forms, with or without
+    modification, are permitted provided that the following conditions are met:
+    1. Redistributions of source code must retain the above copyright
+    notice, this list of conditions and the following disclaimer.
+    2. Redistributions in binary form must reproduce the above copyright
+    notice, this list of conditions and the following disclaimer in the
+    documentation and/or other materials provided with the distribution.
+    3. Neither the name of the copyright holders nor the
+    names of its contributors may be used to endorse or promote products
+    derived from this software without specific prior written permission.
+
+    THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ''AS IS'' AND ANY
+    EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+    WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+    DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER BE LIABLE FOR ANY
+    DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+    (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+    LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+    ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+    (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+    SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+*/
+/**************************************************************************/
+#ifndef USBD_PVT_H_
+#define USBD_PVT_H_
+
+
+#ifdef __cplusplus
+ extern "C" {
+#endif
+
+//--------------------------------------------------------------------+
+// INTERNAL API
+//--------------------------------------------------------------------+
+extern osal_semaphore_t usbd_control_xfer_sem_hdl;
+
+tusb_error_t usbd_init(void);
+void         usbd_task( void* param);
+
+// Carry out Data and Status stage of control transfer
+tusb_error_t usbd_control_xfer_stask(uint8_t port, tusb_dir_t dir, uint8_t * buffer, uint16_t length);
+
+// Return Status of control transfer
+// Note dir is value of direction bit in setup packet (aka DATA stage direction)
+static inline bool usbd_control_status(uint8_t port, tusb_dir_t dir)
+{
+  // status direction is reversed to one in the setup packet
+  return tusb_dcd_control_xfer(port, 1-dir, NULL, 0);
+}
+
+static inline void usbd_control_stall(uint8_t port)
+{
+  tusb_dcd_edpt_stall(port, 0);
+}
+
+
+#ifdef __cplusplus
+ }
+#endif
+
+#endif /* USBD_PVT_H_ */

tinyusb/osal/osal.h

@@ -72,10 +72,10 @@
 
   //------------- Sub Task -------------//
   #define OSAL_SUBTASK_BEGIN
-  #define OSAL_SUBTASK_END      return TUSB_ERROR_NONE;
+  #define OSAL_SUBTASK_END                    return TUSB_ERROR_NONE;
 
-  #define SUBTASK_RETURN(error)   return error;
-  #define SUBTASK_INVOKE(subtask, status) status = subtask
+  #define SUBTASK_RETURN(_error)              return _error;
+  #define SUBTASK_INVOKE(_subtask, _status)   (_status) = _subtask
 
   //------------- Sub Task Assert -------------//
   #define SUBTASK_ASSERT_STATUS(sts) VERIFY_STATUS(sts)

tinyusb/osal/osal_none.h

@@ -107,19 +107,19 @@ static inline osal_task_t osal_task_create(osal_func_t code, const char* name, u
 // SUBTASK (a sub function that uses OS blocking services & called by a task
 //--------------------------------------------------------------------+
 #define OSAL_SUBTASK_BEGIN  OSAL_TASK_BEGIN
-#define OSAL_SUBTASK_END \
-  default: TASK_RESTART; break; \
-  }}\
+
+#define OSAL_SUBTASK_END                                                        \
+  default: TASK_RESTART; break;                                                 \
+  }}                                                                            \
   return TUSB_ERROR_NONE;
 
-#define SUBTASK_INVOKE(_subtask, _status) \
-  do {\
-    _state = __LINE__; case __LINE__:\
-    {\
-      _status = _subtask; /* invoke sub task */\
-      if (TUSB_ERROR_OSAL_WAITING == _status) /* sub task not finished -> continue waiting */\
-        return TUSB_ERROR_OSAL_WAITING;\
-    }\
+#define SUBTASK_INVOKE(_subtask, _status)                                       \
+  do {                                                                          \
+    _state = __LINE__; case __LINE__:                                           \
+    {                                                                           \
+      (_status) = _subtask; /* invoke sub task */                               \
+      if (TUSB_ERROR_OSAL_WAITING == (_status)) return TUSB_ERROR_OSAL_WAITING; \
+    }                                                                           \
   }while(0)
 
 //------------- Sub Task Assert -------------//

tinyusb/tusb.c

@@ -39,6 +39,7 @@
 #define _TINY_USB_SOURCE_FILE_
 
 #include "tusb.h"
+#include "device/usbd_pvt.h"
 
 tusb_error_t tusb_init(void)
 {