TMS320F28069M: controlSuite: USBLIB: CDC-ACM: USB OUT endpoint stops working if the circular buffer is full. Patch!

Part Number: TMS320F28069M
Other Parts Discussed in Thread: CONTROLSUITE

Hi,

This is a bug report and patch submission.  I am putting it here because it took me a while to track down and I'm sure others will appreciate it.  (Hopefully it gets mainlined into TI's USB stack)

The USB stack in the v3.4.6 release of controlSuite uses the SetDeferredOpFlag function in usbdcdc.c to track if it needs to come back and perform USB operations at a later date.  One example of this includes removing data from the hardware FIFO if the circular buffer is full.  This function uses a macro to set or clear a bit in a state variable to indicate if an deferred operation is required.

 SetDeferredOpFlag(volatile uint16_t *pui16DeferredOp, uint16_t ui16Bit,
                   bool bSet)
 {
    //
    // Set the flag bit to 1 or 0 using a bitband access.
    //
    HWREGBITHW(pui16DeferredOp, ui16Bit, bSet ? 1 : 0);

 }

Please note the ternary operator evaluates boolean bSet and is the third parameter in the HWREGBITHW macro.  This macro is defined in hw_types.h.

#define HWREGBITHW(address, mask, value)                                                                            \
         (*(volatile unsigned short *)(address)) =                                                                               \
                                    ((*(volatile unsigned short *)(address)) & ~((unsigned short)1 << mask)) \
                                   | ((unsigned short)value << mask)

We see that (unsigned short)value << mask) line expands out to ((unsigned short) bSet ? 1 : 0 << mask).  The ternary operator is very low on the C operator precedence.  Much lower precedence than bitwise shift.  This means that the bitwise or-ing operation is either with value 1 or with value 0 << mask.  We will never get the correct bitmask of (1 << mask).  And the operation that we need to come back and do is lost forever...  In our case, that deferred operation was to service the RX Hardware Fifo once the circular buffer had emptied a little.

Here is a patch diff that fixes the issue at two places: 1.) In the macro by putting () around all parameters to guarantee precedence and 2.) In the function by removing the unneeded ternary operator. 

diff --git a/device_support/f2806x/v151/MWare/inc/hw_types.h b/device_support/f2806x/v151/MWare/inc/hw_types.h
index 182252c..6f7e83d 100644
--- a/device_support/f2806x/v151/MWare/inc/hw_types.h
+++ b/device_support/f2806x/v151/MWare/inc/hw_types.h
@@ -55,8 +55,8 @@ typedef int16_t int8_t;
 //Emulated Bitbanded write
 #define HWREGBITHW(address, mask, value)                                       \
         (*(volatile unsigned short *)(address)) =                              \
-       ((*(volatile unsigned short *)(address)) & ~((unsigned short)1 << mask)) \
-       | ((unsigned short)value << mask)
+       ((*(volatile unsigned short *)(address)) & ~((unsigned short)1 << (mask))) \
+       | ((unsigned short)(value) << (mask))
 //Emulated Bitbanded read
 #define HWREGBITHR(address, mask)                                              \
         (((*(volatile unsigned short *)(address)) & ((unsigned short)1 << mask)) >> mask)
diff --git a/device_support/f2806x/v151/MWare/usblib/device/usbdcdc.c b/device_support/f2806x/v151/MWare/usblib/device/usbdcdc.c
index bea2096..7e230b7 100644
--- a/device_support/f2806x/v151/MWare/usblib/device/usbdcdc.c
+++ b/device_support/f2806x/v151/MWare/usblib/device/usbdcdc.c
@@ -558,10 +558,7 @@ static void
 SetDeferredOpFlag(volatile uint16_t *pui16DeferredOp, uint16_t ui16Bit,
                   bool bSet)
 {
-    //
-    // Set the flag bit to 1 or 0 using a bitband access.
-    //
-    HWREGBITHW(pui16DeferredOp, ui16Bit, bSet ? 1 : 0);
+    HWREGBITHW(pui16DeferredOp, ui16Bit, bSet);
 }

Hope that helps!

-Colin