USB Driver "Deferred" Control Transfers

[apmorton]

A common problem I have faced with the ChibiOS USB stack is with implementing setup/control transfers that need to calculate anything dynamic or complex on the fly. When designing your own USB protocol, these kinds of issues can easily be avoided; unfortunately sometimes the protocol already exists, and you simply need to work around it.

Another scenario is that you may not know what protocol you are going to be speaking over usb until you have received a request for the full device descriptor, and you need to do some complex initialization before responding to this request for the device descriptor. (this might sound insane to you, but I can promise it is something that can make sense in the right context).

In these cases with the current USB driver you are very limited in what can be done, because the setup request must be entirely handled in an interrupt context.

I have solved these problems in the past by hijacking "epc[0]" in the event_cb handler to entirely supplant the built in handling of setup packets in the chibios usb driver. The solution works, but the end result is not pretty, and very fragile with respect to changes in ChibiOS itself. (IE, updating ChibiOS is more likely to break stuff). It also results in a fair amount of previously built-in control transfers needing to be handled in application code.

I have implemented a solution that comprises of the following new API's

Code: Select all

/*
 * call in requests_hook_cb and then return true in order to "defer" the transfer.
 * "deferring" effectively sets a flag in the driver state and then returns from the interrupt.
 * this means IN/OUT requests on EP0 will NAK until the deferred transfer is completed, or the host gives up.
 */
#define usbDeferTransfer(usbp);

/*
 * this is effectively usbSetupTransfer, except it can be called from anywhere in your application
 * as long as you have previously used usbDeferTransfer, and in addition to what usbSetupTransfer would do
 * it actually starts the transfer.
 */
void usbSetupDeferredTransferI(USBDriver *usbp, uint8_t *buf, size_t n, usbcallback_t endcb);

/*
 * allows you to issue a STALL on EP0 if you had previously used usbDeferTransfer.
 * effectively the same as returning false from requests_hook_cb, but allows you to
 * defer the decision.  Can be called from anywhere in your application.
 */
void usbStallDeferredTransferI(USBDriver *usbp);

/*
 * allows you to complete a deferred transfer in a blocking manner.
 * returns MSG_RESET if the transfer fails for any reason (usb reset, timeout, etc).
 * currently returns MSG_OK after the status phase has completed.
 * potentially should return number of bytes transferred instead, similar to usbReceive
 */
msg_t usbCompleteDeferredTransfer(USBDriver *usbp, uint8_t *buf, size_t n);


Additionally, I have added "requests_hook2_cb", which is identical to "requests_hook_cb" except that it is only called after "default_handler" has returned false. This allows to easily defer all otherwise unhandled transfers.

Diff follows:
Note that this diff is based on 17.6.x - it may not apply cleanly to trunk.

Code: Select all

diff --git a/os/hal/include/hal_usb.h b/os/hal/include/hal_usb.h
index 9abbc6eb9..218a34ae6 100644
--- a/os/hal/include/hal_usb.h
+++ b/os/hal/include/hal_usb.h
@@ -294,7 +294,8 @@ typedef enum {
   USB_EP0_IN_SENDING_STS = USB_IN_STATE | 3U,   /**< Sending status.        */
   USB_EP0_OUT_WAITING_STS = USB_OUT_STATE | 4U, /**< Waiting status.        */
   USB_EP0_OUT_RX = USB_OUT_STATE | 5U,          /**< Receiving.             */
-  USB_EP0_ERROR = 6U                            /**< Error, EP0 stalled.    */
+  USB_EP0_DEFER_WAITING = 6U,                   /**< Waiting for Deferral.  */
+  USB_EP0_ERROR = 7U                            /**< Error, EP0 stalled.    */
 } usbep0state_t;
 
 /**
@@ -307,7 +308,8 @@ typedef enum {
   USB_EVENT_UNCONFIGURED = 3,           /**< Configuration removed.         */
   USB_EVENT_SUSPEND = 4,                /**< Entering suspend mode.         */
   USB_EVENT_WAKEUP = 5,                 /**< Leaving suspend mode.          */
-  USB_EVENT_STALLED = 6                 /**< Endpoint 0 error, stalled.     */
+  USB_EVENT_STALLED = 6,                /**< Endpoint 0 error, stalled.     */
+  USB_EVENT_DEFER_TIMEOUT = 7           /**< Endpoint 0 deferral timeout.   */
 } usbevent_t;
 
 /**
@@ -474,11 +476,28 @@ typedef const USBDescriptor * (*usbgetdescriptor_t)(USBDriver *usbp,
  * @special
  */
 #define usbSetupTransfer(usbp, buf, n, endcb) {                             \
+  (usbp)->ep0defer = FALSE;                                                 \
   (usbp)->ep0next  = (buf);                                                 \
   (usbp)->ep0n     = (n);                                                   \
   (usbp)->ep0endcb = (endcb);                                               \
 }
 
+/**
+ * @brief   Defer transfer setup.
+ * @details This macro is used by the request handling callbacks in order to
+ *          defer a transaction over the endpoint zero.
+ *
+ * @param[in] usbp      pointer to the @p USBDriver object
+ *
+ * @special
+ */
+#define usbDeferTransfer(usbp) {                                            \
+  (usbp)->ep0defer = TRUE;                                                  \
+  (usbp)->ep0next  = NULL;                                                  \
+  (usbp)->ep0n     = 0;                                                     \
+  (usbp)->ep0endcb = NULL;                                                  \
+}
+
 /**
  * @brief   Reads a setup packet from the dedicated packet buffer.
  * @details This function must be invoked in the context of the @p setup_cb
@@ -614,9 +633,13 @@ extern "C" {
                         uint8_t *buf, size_t n);
   void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
                          const uint8_t *buf, size_t n);
+  void usbSetupDeferredTransferI(USBDriver *usbp, uint8_t *buf, size_t n,
+                                 usbcallback_t endcb);
+  void usbStallDeferredTransferI(USBDriver *usbp);
 #if USB_USE_WAIT == TRUE
   msg_t usbReceive(USBDriver *usbp, usbep_t ep, uint8_t *buf, size_t n);
   msg_t usbTransmit(USBDriver *usbp, usbep_t ep, const uint8_t *buf, size_t n);
+  msg_t usbCompleteDeferredTransfer(USBDriver *usbp, uint8_t *buf, size_t n);
 #endif
   bool usbStallReceiveI(USBDriver *usbp, usbep_t ep);
   bool usbStallTransmitI(USBDriver *usbp, usbep_t ep);
@@ -625,6 +648,7 @@ extern "C" {
   void _usb_suspend(USBDriver *usbp);
   void _usb_wakeup(USBDriver *usbp);
   void _usb_ep0setup(USBDriver *usbp, usbep_t ep);
+  void _usb_ep0transferI(USBDriver *usbp);
   void _usb_ep0in(USBDriver *usbp, usbep_t ep);
   void _usb_ep0out(USBDriver *usbp, usbep_t ep);
 #ifdef __cplusplus
diff --git a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
index ba5e4fc2f..d2ff7c52d 100644
--- a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
@@ -411,6 +411,12 @@ typedef struct {
    *          handle non standard requests.
    */
   usbreqhandler_t               requests_hook_cb;
+  /**
+   * @brief   Fallback requests hook callback.
+   * @details This hook allows to be handle requests that are left unhandled
+   *          after the built in default request handler is run.
+   */
+  usbreqhandler_t               requests_hook2_cb;
   /**
    * @brief   Start Of Frame callback.
    */
@@ -460,6 +466,10 @@ struct USBDriver {
    * @brief   Endpoint 0 state.
    */
   usbep0state_t                 ep0state;
+  /**
+   * @brief   Endpoint 0 transfer deferral flag
+   */
+  bool                          ep0defer;
   /**
    * @brief   Next position in the buffer to be transferred through endpoint 0.
    */
@@ -472,6 +482,12 @@ struct USBDriver {
    * @brief   Endpoint 0 end transaction callback.
    */
   usbcallback_t                 ep0endcb;
+  #if (USB_USE_WAIT == TRUE) || defined(__DOXYGEN__)
+  /**
+   * @brief   Endpoint 0 deferral waiting thread.
+   */
+  thread_reference_t            ep0deferthread;
+#endif
   /**
    * @brief   Setup packet buffer.
    */
diff --git a/os/hal/src/hal_usb.c b/os/hal/src/hal_usb.c
index a9ab95ca0..a671a2225 100644
--- a/os/hal/src/hal_usb.c
+++ b/os/hal/src/hal_usb.c
@@ -351,6 +351,11 @@ void usbStop(USBDriver *usbp) {
 #endif
     usbp->epc[i] = NULL;
   }
+
+#if USB_USE_WAIT == TRUE
+  osalThreadResumeI(&usbp->ep0deferthread, MSG_RESET);
+#endif
+
   osalOsRescheduleS();
 
   osalSysUnlock();
@@ -429,6 +434,10 @@ void usbDisableEndpointsI(USBDriver *usbp) {
     usbp->epc[i] = NULL;
   }
 
+#if USB_USE_WAIT == TRUE
+  osalThreadResumeI(&usbp->ep0deferthread, MSG_RESET);
+#endif
+
   /* Low level endpoints deactivation.*/
   usb_lld_disable_endpoints(usbp);
 }
@@ -512,6 +521,29 @@ void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
   usb_lld_start_in(usbp, ep);
 }
 
+void usbSetupDeferredTransferI(USBDriver *usbp, uint8_t *buf, size_t n,
+                               usbcallback_t endcb) {
+
+  osalDbgCheckClassI();
+  osalDbgCheck(usbp != NULL);
+  osalDbgAssert(usbp->ep0state == USB_EP0_DEFER_WAITING, "invalid ep0 state");
+
+  usbSetupTransfer(usbp, buf, n, endcb);
+  _usb_ep0transferI(usbp);
+}
+
+void usbStallDeferredTransferI(USBDriver *usbp) {
+
+  osalDbgCheckClassI();
+  osalDbgCheck(usbp != NULL);
+  osalDbgAssert(usbp->ep0state == USB_EP0_DEFER_WAITING, "invalid ep0 state");
+
+  usb_lld_stall_in(usbp, 0);
+  usb_lld_stall_out(usbp, 0);
+  // _usb_isr_invoke_event_cb(usbp, USB_EVENT_STALLED);
+  usbp->ep0state = USB_EP0_ERROR;
+}
+
 #if (USB_USE_WAIT == TRUE) || defined(__DOXYGEN__)
 /**
  * @brief   Performs a receive transaction on an OUT endpoint.
@@ -579,6 +611,27 @@ msg_t usbTransmit(USBDriver *usbp, usbep_t ep, const uint8_t *buf, size_t n) {
 
   return msg;
 }
+
+msg_t usbCompleteDeferredTransfer(USBDriver *usbp, uint8_t *buf, size_t n) {
+  msg_t msg = MSG_RESET;
+
+  osalSysLock();
+
+  switch (usbGetDriverStateI(usbp)) {
+  case USB_READY:
+  case USB_SELECTED:
+  case USB_ACTIVE:
+    usbSetupDeferredTransferI(usbp, buf, n, NULL);
+    msg = osalThreadSuspendS(&usbp->ep0deferthread);
+   break;
+  default:
+   break;
+  }
+
+  osalSysUnlock();
+  return msg;
+}
+
 #endif /* USB_USE_WAIT == TRUE */
 
 /**
@@ -688,6 +741,12 @@ void _usb_reset(USBDriver *usbp) {
     usbp->epc[i] = NULL;
   }
 
+#if USB_USE_WAIT == TRUE
+  osalSysLockFromISR();
+  osalThreadResumeI(&usbp->ep0deferthread, MSG_RESET);
+  osalSysUnlockFromISR();
+#endif
+
   /* EP0 state machine initialization.*/
   usbp->ep0state = USB_EP0_STP_WAITING;
 
@@ -734,6 +793,10 @@ void _usb_suspend(USBDriver *usbp) {
         osalSysUnlockFromISR();
       }
     }
+
+    osalSysLockFromISR();
+    osalThreadResumeI(&usbp->ep0deferthread, MSG_RESET);
+    osalSysUnlockFromISR();
   }
 #endif
 }
@@ -767,12 +830,23 @@ void _usb_wakeup(USBDriver *usbp) {
  * @notapi
  */
 void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
-  size_t max;
-
   /* Is the EP0 state machine in the correct state for handling setup
      packets?*/
   if (usbp->ep0state != USB_EP0_STP_WAITING) {
-    /* This is unexpected could require handling with a warning event.*/
+    /* If the user deferred a transfer and the host is requesting another
+       the user has taken too long and the host has given up on the previous
+       transfer.  Notify the user so they can potentially cleanup. */
+    if (usbp->ep0state == USB_EP0_DEFER_WAITING) {
+      _usb_isr_invoke_event_cb(usbp, USB_EVENT_DEFER_TIMEOUT);
+    }
+
+#if USB_USE_WAIT == TRUE
+      osalSysLockFromISR();
+      osalThreadResumeI(&usbp->ep0deferthread, MSG_RESET);
+      osalSysUnlockFromISR();
+#endif
+
+    /* Other cases unexpected.  Could require handling with a warning event.*/
     /* TODO: handling here.*/
 
     /* Resetting the EP0 state machine and going ahead.*/
@@ -794,14 +868,17 @@ void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
     if (((usbp->setup[0] & USB_RTYPE_TYPE_MASK) != USB_RTYPE_TYPE_STD) ||
         !default_handler(usbp)) {
     /*lint -restore*/
-      /* Error response, the state machine goes into an error state, the low
-         level layer will have to reset it to USB_EP0_WAITING_SETUP after
-         receiving a SETUP packet.*/
-      usb_lld_stall_in(usbp, 0);
-      usb_lld_stall_out(usbp, 0);
-      _usb_isr_invoke_event_cb(usbp, USB_EVENT_STALLED);
-      usbp->ep0state = USB_EP0_ERROR;
-      return;
+      if ((usbp->config->requests_hook2_cb == NULL) ||
+          !(usbp->config->requests_hook2_cb(usbp))) {
+        /* Error response, the state machine goes into an error state, the low
+          level layer will have to reset it to USB_EP0_WAITING_SETUP after
+          receiving a SETUP packet.*/
+        usb_lld_stall_in(usbp, 0);
+        usb_lld_stall_out(usbp, 0);
+        _usb_isr_invoke_event_cb(usbp, USB_EVENT_STALLED);
+        usbp->ep0state = USB_EP0_ERROR;
+        return;
+      }
     }
   }
 #if (USB_SET_ADDRESS_ACK_HANDLING == USB_SET_ADDRESS_ACK_HW)
@@ -810,6 +887,22 @@ void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
     return;
   }
 #endif
+
+  if (usbp->ep0defer) {
+    usbp->ep0state = USB_EP0_DEFER_WAITING;
+    return;
+  }
+
+  osalSysLockFromISR();
+  _usb_ep0transferI(usbp);
+  osalSysUnlockFromISR();
+}
+
+void _usb_ep0transferI(USBDriver *usbp) {
+  size_t max;
+
+  osalDbgCheckClassI();
+
   /* Transfer preparation. The request handler must have populated
      correctly the fields ep0next, ep0n and ep0endcb using the macro
      usbSetupTransfer().*/
@@ -823,18 +916,14 @@ void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
     if (usbp->ep0n != 0U) {
       /* Starts the transmit phase.*/
       usbp->ep0state = USB_EP0_IN_TX;
-      osalSysLockFromISR();
       usbStartTransmitI(usbp, 0, usbp->ep0next, usbp->ep0n);
-      osalSysUnlockFromISR();
     }
     else {
       /* No transmission phase, directly receiving the zero sized status
          packet.*/
       usbp->ep0state = USB_EP0_OUT_WAITING_STS;
 #if (USB_EP0_STATUS_STAGE == USB_EP0_STATUS_STAGE_SW)
-      osalSysLockFromISR();
       usbStartReceiveI(usbp, 0, NULL, 0);
-      osalSysUnlockFromISR();
 #else
       usb_lld_end_setup(usbp, ep);
 #endif
@@ -845,18 +934,14 @@ void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
     if (usbp->ep0n != 0U) {
       /* Starts the receive phase.*/
       usbp->ep0state = USB_EP0_OUT_RX;
-      osalSysLockFromISR();
       usbStartReceiveI(usbp, 0, usbp->ep0next, usbp->ep0n);
-      osalSysUnlockFromISR();
     }
     else {
       /* No receive phase, directly sending the zero sized status
          packet.*/
       usbp->ep0state = USB_EP0_IN_SENDING_STS;
 #if (USB_EP0_STATUS_STAGE == USB_EP0_STATUS_STAGE_SW)
-      osalSysLockFromISR();
       usbStartTransmitI(usbp, 0, NULL, 0);
-      osalSysUnlockFromISR();
 #else
       usb_lld_end_setup(usbp, ep);
 #endif
@@ -909,6 +994,11 @@ void _usb_ep0in(USBDriver *usbp, usbep_t ep) {
     if (usbp->ep0endcb != NULL) {
       usbp->ep0endcb(usbp);
     }
+#if USB_USE_WAIT == TRUE
+    osalSysLockFromISR();
+    osalThreadResumeI(&usbp->ep0deferthread, MSG_OK);
+    osalSysUnlockFromISR();
+#endif
     usbp->ep0state = USB_EP0_STP_WAITING;
     return;
   case USB_EP0_STP_WAITING:
@@ -967,6 +1057,11 @@ void _usb_ep0out(USBDriver *usbp, usbep_t ep) {
     if (usbp->ep0endcb != NULL) {
       usbp->ep0endcb(usbp);
     }
+#if USB_USE_WAIT == TRUE
+    osalSysLockFromISR();
+    osalThreadResumeI(&usbp->ep0deferthread, MSG_OK);
+    osalSysUnlockFromISR();
+#endif
     usbp->ep0state = USB_EP0_STP_WAITING;
     return;
   case USB_EP0_STP_WAITING:


This mechanism works currently.
I am looking for feedback on the API.
Hopefully this work or something similar can be included into ChibiOS.

Thanks,
Austin Morton

[Giovanni]

Hi,

The driver is complex, frankly I am reluctant doing changes that could break things. What about adding an hook that allows to replace the standard EP0 handling? if it is entirely left to the application it is unlikely that changes in the driver would break it.

Giovanni

[apmorton]

Hi,

That is effectively what I was doing before.

Code: Select all

static void usbEventCallback(USBDriver *usbp, usbevent_t event) {
   switch (event) {
   case USB_EVENT_RESET:
      // check if we have not yet patched the endpoint 0 config
      if (usbp->epc[0] != &my_ep0config) {
         // copy the original ep0 config
         my_ep0config = *usbp->epc[0];

         // overwrite the original handlers with ours
         my_ep0config.setup_cb = myEp0SetupCallback;
         my_ep0config.in_cb = NULL;
         my_ep0config.out_cb = NULL;

         // update the endpoint config pointer
         usbp->epc[0] = &my_ep0config;
      }
      break;
   }
}


As mentioned - this works, but there are drawbacks.

• you now need to implement all the standard control transfers yourself
• it is all or nothing. you can't handle just a specific few transfers in a special way and leave the default/existing mechanism for everything else, so it is very time consuming to do something like this after the fact.
• it is _relatively_ stable, but I have had my code broken because of this when updating ChibiOS in the past. specifically there was a check for ep0state added in the LLD that was causing silent failures.

I agree - the driver is complex. However, I don't think the changes I have made make it any _more_ complex.

Basically no logic was changed in the existing _usb_ep0setup function. Only the second half of the function was factored out into _usb_ep0transferI so that it can be called from usbSetupDeferredTransferI.

The only potentially breaking change is that usb_lld_end_setup is now called while holding the system lock. However, in the only port I could find that uses it (AVR) this should not be a problem.

I think the feature is low-risk as is, but it could also be opt-in through something like "USB_USE_DEFER", similar to "USB_USE_WAIT".

Austin

[WowSuchName]

Hey you two,

I know this is an old thread, but I think having means to defer control transfers to threads would greatly improve the USB driver. Think of flash interactions (e.g., USB-DFU), which you really wouldn't want to do (and which you can't do using EFL driver) inside an ISR context.

@Austin: thanks a lot for sharing your changes. I was able to successfully port them to ver21.11.2 without any major issues and the alterations don't look like they'd break things.

@Giovanni: if you feel like touching the USB driver at some point in time, feel free to have a look at my "DFU-ready" branch on github. Beside Austin's deferred transfers ported to ver21.11.2, I implemented the LLD necessary stuff for the remaining architectures and also introduced zero-length packets as mentioned in this thread. It's been a while since I did the changes and tested them on an STM32F411 MCU, so I'm not so sure how through these tests were.


Best regards,
Tim

[WowSuchName]

Alright, seems like I didn't test it with system state debug

Austin's patch separates the actual transfer code from the ISR context in order to reuse it when performing the deferred transfer from thread level. The resulting I-class function _usb_ep0transferI did contain locks itself, though. Fix by https://github.com/trheinfels/ChibiOS/c ... 9b1719c484 Not sure, if this is the best approach, though, as the system is now locked longer than in the original implementation.


Best regards
Tim