Merge pull request #23 from alrvid/master

Fixes connect() bug, crash bug, I/O error bug, and more.

Author: facchinm

.codespellrc

@@ -2,7 +2,7 @@
 # See: https://github.com/codespell-project/codespell#using-a-config-file
 [codespell]
 # In the event of a false positive, add the problematic word, in all lowercase, to a comma-separated list here:
-ignore-words-list = freeed,
+ignore-words-list = freeed,unstall,
 skip = ./.git,./.licenses,__pycache__,node_modules,./go.mod,./go.sum,./package-lock.json,./poetry.lock,./yarn.lock
 builtin = clear,informal,en-GB_to_en-US
 check-filenames =

src/USBHost/USBHost.cpp

@@ -1145,6 +1145,8 @@ USB_TYPE USBHost::generalTransfer(USBDeviceConnected * dev, USBEndpoint * ep, ui
         USB_DBG_TRANSFER("%s TRANSFER res: %s on ep: %p\r\n", type_str, ep->getStateString(), ep);
 
         if (res != USB_TYPE_IDLE) {
+            // Set state to USB_TYPE_IDLE to allow recovery from the error, but return the error
+            ep->setState(USB_TYPE_IDLE);
             return res;
         }
 

src/USBHostMSD/USBHostMSD.cpp

@@ -187,7 +187,14 @@ int USBHostMSD::inquiry(uint8_t lun, uint8_t page_code)
 
 int USBHostMSD::checkResult(uint8_t res, USBEndpoint * ep)
 {
-    // if ep stalled: send clear feature
+    // Guard against host not being initialized
+    if (nullptr == host)
+    {
+        return -1;
+    }
+
+    // Notice that USB_TYPE_STALL_ERROR is never actually set anywhere in the library for the ST target, because
+    // URB_STALL is never handled directly. Otherwise, this would be the code to unstall the EP.
     if (res == USB_TYPE_STALL_ERROR) {
         res = host->controlWrite(   dev,
                                     USB_RECIPIENT_ENDPOINT | USB_HOST_TO_DEVICE | USB_REQUEST_TYPE_STANDARD,
@@ -211,6 +218,12 @@ int USBHostMSD::SCSITransfer(uint8_t * cmd, uint8_t cmd_len, int flags, uint8_t
 
     int res = 0;
 
+    // Guard against host not being initialized
+    if (nullptr == host)
+    {
+        return -1;
+    }
+
     cbw.Signature = CBW_SIGNATURE;
     cbw.Tag = 0;
     cbw.DataLength = transfer_len;
@@ -273,7 +286,9 @@ int USBHostMSD::SCSITransfer(uint8_t * cmd, uint8_t cmd_len, int flags, uint8_t
                                     BO_MASS_STORAGE_RESET,
                                     0, msd_intf, NULL, 0);
 
-        // uninstall both endpoints
+        // Unstall [sic!] both endpoints.
+        // Notice that this is the unstall that is required by the specification as part of clearing a Phase Error,
+        // _not_ the unstall for an ordinarily stalled EP!
         res = host->controlWrite(   dev,
                                     USB_RECIPIENT_ENDPOINT | USB_HOST_TO_DEVICE | USB_REQUEST_TYPE_STANDARD,
                                     CLEAR_FEATURE,
@@ -310,6 +325,13 @@ int USBHostMSD::dataTransfer(uint8_t * buf, uint32_t block, uint8_t nbBlock, int
 int USBHostMSD::getMaxLun()
 {
     uint8_t buf[1], res;
+
+    // Guard against host not being initialized
+    if (nullptr == host)
+    {
+        return -1;
+    }
+
     res = host->controlRead(    dev, USB_RECIPIENT_INTERFACE | USB_DEVICE_TO_HOST | USB_REQUEST_TYPE_CLASS,
                                 0xfe, 0, msd_intf, buf, 1);
     USB_DBG("max lun: %d", buf[0]);

src/USBHostMSD/USBHostMSD.h

@@ -75,7 +75,7 @@ class USBHostMSD : public IUSBEnumerator, public mbed::BlockDevice
 
 
 private:
-    USBHost * host;
+    USBHost * host = nullptr;
     USBDeviceConnected * dev;
     bool dev_connected;
     USBEndpoint * bulk_in;

src/targets/TARGET_STM/USBEndpoint_STM.cpp

@@ -20,13 +20,20 @@
 
 #include "USBHost/dbg.h"
 #include "USBHost/USBEndpoint.h"
+#include "USBHost/USBDeviceConnected.h"
+
 extern uint32_t HAL_HCD_HC_GetMaxPacket(HCD_HandleTypeDef *hhcd, uint8_t chn_num);
 extern uint32_t HAL_HCD_HC_GetType(HCD_HandleTypeDef *hhcd, uint8_t chn_num);
 extern void HAL_HCD_DisableInt(HCD_HandleTypeDef *hhcd, uint8_t chn_num);
 extern void HAL_HCD_EnableInt(HCD_HandleTypeDef *hhcd, uint8_t chn_num);
 
-
-
+#if defined(TARGET_PORTENTA_H7)
+#define USBx_BASE   USB2_OTG_FS_PERIPH_BASE
+#elif defined(TARGET_GIGA)
+#define USBx_BASE   USB1_OTG_HS_PERIPH_BASE
+#else
+#define USBx_BASE   USB1_OTG_HS_PERIPH_BASE
+#endif
 
 void USBEndpoint::init(HCED *hced_, ENDPOINT_TYPE type_, ENDPOINT_DIRECTION dir_, uint32_t size, uint8_t ep_number, HCTD *td_list_[2])
 {
@@ -107,21 +114,45 @@ void USBEndpoint::setState(USB_TYPE st)
         if ((*addr) && (type != INTERRUPT_ENDPOINT)) {
             this->ep_queue.put((uint8_t *)1);
         }
-        HAL_HCD_HC_Halt((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num);
+        // Flush posted requests
+        USBx_HC(hced->ch_num)->HCCHAR |= USB_OTG_HCCHAR_CHDIS;
+        // Disable the channel
+        USBx_HC(hced->ch_num)->HCCHAR |= USB_OTG_HCCHAR_CHDIS | USB_OTG_HCCHAR_CHENA;
+        // Notice the lack of error handling here - after testing a lot of 
+        // combinations, it seems that the USB host controller doesn't work fully
+        // according to the reference manual in this aspect, which might also
+        // be the reason for lacking error handling in the ST USB LL - instead,
+        // we use a 50 us delay to wait for the channel to disable
+        wait_us(50);
+
         HAL_HCD_DisableInt((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num);
         *addr = 0;
 
     }
     if (st == USB_TYPE_ERROR) {
-        HAL_HCD_HC_Halt((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num);
-        HAL_HCD_DisableInt((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num);
+        // Flush posted requests
+        USBx_HC(hced->ch_num)->HCCHAR |= USB_OTG_HCCHAR_CHDIS;
+        // Disable the channel
+        USBx_HC(hced->ch_num)->HCCHAR |= USB_OTG_HCCHAR_CHDIS | USB_OTG_HCCHAR_CHENA;
+        // Notice the lack of error handling here - after testing a lot of 
+        // combinations, it seems that the USB host controller doesn't work fully
+        // according to the reference manual in this aspect, which might also
+        // be the reason for lacking error handling in the ST USB LL - instead,
+        // we use a 50 us delay to wait for the channel to disable
+        wait_us(50);
 
-    }
-    if (st == USB_TYPE_ERROR) {
+        HAL_HCD_DisableInt((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num);
         uint8_t hcd_speed = HCD_SPEED_FULL;
         /* small speed device with hub not supported
            if (this->speed) hcd_speed = HCD_SPEED_LOW;*/
-        HAL_HCD_HC_Init((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num, address, 0, hcd_speed,  type, size);
+
+        // Notice that dev->getAddress() must be used instead of device_address, because the latter will contain
+        // incorrect values in certain cases
+        HAL_HCD_HC_Init((HCD_HandleTypeDef *)hced->hhcd, hced->ch_num, address, dev->getAddress(), hcd_speed,  type, size);
+        // HAL_HCD_HC_Init() doesn't fully enable the channel after disable above, so we do it here -->
+        USBx_HC(hced->ch_num)->HCCHAR &= ~USB_OTG_HCCHAR_CHDIS;
+        USBx_HC(hced->ch_num)->HCCHAR |= USB_OTG_HCCHAR_CHENA;
+        // <--
     }
 }
 
@@ -150,7 +181,7 @@ USB_TYPE USBEndpoint::queueTransfer()
     state = USB_TYPE_PROCESSING;
     /*  one request */
     td_current->nextTD = (hcTd *)0;
-#if defined(MAX_NYET_RETRY)
+#if defined(MAX_NOTREADY_RETRY)
     td_current->retry = 0;
 #endif
     td_current->setup = setup;

src/targets/TARGET_STM/USBHALHost_STM.cpp

@@ -76,6 +76,21 @@ uint32_t HAL_HCD_HC_GetType(HCD_HandleTypeDef *hhcd, uint8_t chnum)
     return hhcd->hc[chnum].ep_type;
 }
 
+// The urb_state parameter can be one of the following according to the ST documentation, but the explanations
+// that follow are the result of an analysis of the ST HAL / LL source code, the Reference Manual for the
+// STM32H747 microcontroller, and the source code of this library:
+//
+//  - URB_ERROR    = various serious errors that may be hard or impossible to recover from without pulling
+//                   the thumb drive out and putting it back in again, but we will try
+//  - URB_IDLE     = set by a call to HAL_HCD_HC_SubmitRequest(), but never used by this library
+//  - URB_NYET     = never actually used by the ST HAL / LL at the time of writing, nor by this library
+//  - URB_STALL    = a stall response from the device - but it is never handled by the library and will end up
+//                   as a timeout at a higher layer, because of an ep_queue.get() timeout, which will activate
+//                   error recovery indirectly
+//  - URB_DONE     = the transfer completed normally without errors
+//  - URB_NOTREADY = a NAK, NYET, or not more than a couple of repeats of some of the errors that will
+//                   become URB_ERROR if they repeat several times in a row
+//
 void HAL_HCD_HC_NotifyURBChange_Callback(HCD_HandleTypeDef *hhcd, uint8_t chnum, HCD_URBStateTypeDef urb_state)
 {
     USBHALHost_Private_t *priv = (USBHALHost_Private_t *)(hhcd->pData);
@@ -93,7 +108,7 @@ void HAL_HCD_HC_NotifyURBChange_Callback(HCD_HandleTypeDef *hhcd, uint8_t chnum,
         if ((type == EP_TYPE_BULK) || (type == EP_TYPE_CTRL)) {
             switch (urb_state) {
                 case URB_DONE:
-#if defined(MAX_NYET_RETRY)
+#if defined(MAX_NOTREADY_RETRY)
                     td->retry = 0;
 #endif
                     if (td->size >  max_size) {
@@ -107,21 +122,18 @@ void HAL_HCD_HC_NotifyURBChange_Callback(HCD_HandleTypeDef *hhcd, uint8_t chnum,
                     }
                     break;
                 case  URB_NOTREADY:
-                    /*  try again  */
-                    /*  abritary limit , to avoid dead lock if other error than
-                     *  slow response is  */
-#if defined(MAX_NYET_RETRY)
-                    if (td->retry < MAX_NYET_RETRY) {
-                        /*  increment retry counter */
+#if defined(MAX_NOTREADY_RETRY)
+                    if (td->retry < MAX_NOTREADY_RETRY) {
                         td->retry++;
 #endif
+                        // Submit the same request again, because the device wasn't ready to accept the last one
                         length = td->size <= max_size ? td->size : max_size;
                         HAL_HCD_HC_SubmitRequest(hhcd, chnum, dir, type, !td->setup, (uint8_t *) td->currBufPtr, length, 0);
                         HAL_HCD_EnableInt(hhcd, chnum);
                         return;
-#if defined(MAX_NYET_RETRY)
+#if defined(MAX_NOTREADY_RETRY)
                     } else {
-                        //USB_ERR("urb_state != URB_NOTREADY");
+                        // MAX_NOTREADY_RETRY reached, so stop trying to resend and instead wait for a timeout at a higher layer
                     }
 #endif
                     break;
@@ -138,6 +150,9 @@ void HAL_HCD_HC_NotifyURBChange_Callback(HCD_HandleTypeDef *hhcd, uint8_t chnum,
                 td->state = USB_TYPE_IDLE;
             }
             else if (urb_state == URB_ERROR) {
+                // While USB_TYPE_ERROR in the endpoint state is used to activate error recovery, this value is actually never used.
+                // Going here will lead to a timeout at a higher layer, because of ep_queue.get() timeout, which will activate error
+                // recovery indirectly.
                 td->state = USB_TYPE_ERROR;
             } else {
                 td->state = USB_TYPE_PROCESSING;

src/targets/TARGET_STM/USBHALHost_STM.h

@@ -38,7 +38,8 @@
 
 #define TOTAL_SIZE (HCCA_SIZE + (MAX_ENDPOINT * ED_SIZE) + (MAX_TD * TD_SIZE))
 
-#define MAX_NYET_RETRY      5
+// This must be a large number to account for the relatively high write latency of USB thumb drives
+#define MAX_NOTREADY_RETRY      50000
 
 /* STM device FS have 11 channels (definition is for 60 channels) */
 static volatile  uint8_t usb_buf[TOTAL_SIZE];