[PATCH] AVR USB device driver

[rlippert]

Just a heads up to anyone who cares that I have implemented basic support to act as a USB device for hardware USB-capable AVR micros (sorry no V-USB!) in chibios HAL and sent a pull request via github at: https://github.com/ChibiOS/ChibiOS/pull/11

Comments/complaints welcome (preferably on the pull request itself) if anyone is able or willing to test this outside of my narrow testing on my PJRC TEENSY2++ board.

[utzig]

Hi,

I tried this driver yesterday on an ATmega32U4. I just had to make small changes to reflect the differences in the USB registers between both processors... Then I created a demo with serial usb enabled and it was enumerated by my computer as an ACM-CDC device (or something similar) but I was not able to communicate with it. How did you test the driver? Would you mind also uploading the demo to git repo?

Cheers,
Fabio Utzig

[utzig]

Ok, I got CDC working. Problem was that I was reusing the same endpoint for both bulk in/out and AVR cannot do it. Using different endpoints made it work.

Now I'll work on removing the requirement of adding a new API to all usb drivers (usb_lld_enable_address).

Cheers,
Fabio Utzig

[rlippert]

Thanks for testing! I have an updated version locally that re-targets to the "contrib" repository, removes the need to change the API and fixes a few bugs that popped up when transferring large amounts of data.

I'm still trying to track down an interrupt timing bug during setup transactions that occasionally puts EP0 into a bad transaction state when opening the CDC serial port but I haven't had much luck since adding debug prints changes the timing enough to make it work fine

Once I get this bug tracked down I'll remove all the debug stuff I added and send a pull request to the chibios-contrib repository.

[utzig]

Hi, still working on this?

[rlippert]

Sorry for the delay, finally got a chance to clean this up and sync against the recent HAL v4 changes that are going on in the base chibios which had a lot of USB API changes...

New pull request sent against the contrib repository here: https://github.com/ChibiOS/ChibiOS-Contrib/pull/73

I added the ability to use the named address spaces feature of gcc 4.7+ to store static stuff like the USB descriptors in flash so they don't take up a lot of RAM using the AVR_USB_USE_NAMED_ADDRESS_SPACES preprocessor boolean.
It's off by default since it depends on a few minor type changes in the main chibios repository which I'm not sure how to get committed, but including the changes below:

Code: Select all

From dddc3ffe569d01abf0032e29f2e093666552b8e1 Mon Sep 17 00:00:00 2001
From: Rob Lippert <[email protected]>
Date: Sat, 7 May 2016 19:36:26 -0700
Subject: [PATCH] hal/usb: allow ports to override type of usb buffer pointer

This allows AVR port to use named address spaces to store USB
descriptors in e.g. flash/eeprom and transmit them directly
without bouncing through a memory buffer.

Signed-off-by: Rob Lippert <[email protected]>
---
 os/hal/include/hal_usb.h                   | 6 +++---
 os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h | 5 +++++
 os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h | 5 +++++
 os/hal/src/hal_usb.c                       | 8 ++++----
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/os/hal/include/hal_usb.h b/os/hal/include/hal_usb.h
index 0e9ecb2..5b81b42 100644
--- a/os/hal/include/hal_usb.h
+++ b/os/hal/include/hal_usb.h
@@ -605,11 +605,11 @@ extern "C" {
   void usbInitEndpointI(USBDriver *usbp, usbep_t ep,
                         const USBEndpointConfig *epcp);
   void usbDisableEndpointsI(USBDriver *usbp);
-  void usbReadSetupI(USBDriver *usbp, usbep_t ep, uint8_t *buf);
+  void usbReadSetupI(USBDriver *usbp, usbep_t ep, usbbufptr_t buf);
   void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
-                        uint8_t *buf, size_t n);
+                        usbbufptr_t buf, size_t n);
   void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
-                         const uint8_t *buf, size_t n);
+                         const usbbufptr_t buf, size_t n);
 #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);
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 1132b8e..4cb7ae4 100644
--- a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
@@ -204,6 +204,11 @@
 /*===========================================================================*/
 
 /**
+ * @brief   Peripheral-specific arbitrary byte data pointer.
+ */
+typedef uint8_t *usbbufptr_t;
+
+/**
  * @brief   Peripheral-specific parameters block.
  */
 typedef struct {
diff --git a/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h b/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
index 2cd77dd..74579ed 100644
--- a/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
@@ -153,6 +153,11 @@
 /*===========================================================================*/
 
 /**
+ * @brief   Peripheral-specific arbitrary byte data pointer.
+ */
+typedef uint8_t *usbbufptr_t;
+
+/**
  * @brief   Type of an IN endpoint state structure.
  */
 typedef struct {
diff --git a/os/hal/src/hal_usb.c b/os/hal/src/hal_usb.c
index bc0cf08..825ade5 100644
--- a/os/hal/src/hal_usb.c
+++ b/os/hal/src/hal_usb.c
@@ -432,7 +432,7 @@ void usbDisableEndpointsI(USBDriver *usbp) {
  * @iclass
  */
 void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
-                      uint8_t *buf, size_t n) {
+                      usbbufptr_t buf, size_t n) {
   USBOutEndpointState *osp;
 
   osalDbgCheckClassI();
@@ -446,7 +446,7 @@ void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
   /*lint -save -e661 [18.1] pclint is confused by the check on ep.*/
   osp = usbp->epc[ep]->out_state;
   /*lint -restore*/
-  osp->rxbuf  = buf;
+  osp->rxbuf  = (__typeof__(osp->rxbuf)) buf;
   osp->rxsize = n;
   osp->rxcnt  = 0;
 #if USB_USE_WAIT == TRUE
@@ -471,7 +471,7 @@ void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
  * @iclass
  */
 void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
-                       const uint8_t *buf, size_t n) {
+                       const usbbufptr_t buf, size_t n) {
   USBInEndpointState *isp;
 
   osalDbgCheckClassI();
@@ -485,7 +485,7 @@ void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
   /*lint -save -e661 [18.1] pclint is confused by the check on ep.*/
   isp = usbp->epc[ep]->in_state;
   /*lint -restore*/
-  isp->txbuf  = buf;
+  isp->txbuf  = (__typeof__(isp->txbuf)) buf;
   isp->txsize = n;
   isp->txcnt  = 0;
 #if USB_USE_WAIT == TRUE
-- 
2.5.3

[Giovanni]

Hi,

There is a problem with __typeof__, it is a GCC extension so it cannot be accepted in high level files, we need to support several other compilers.

Giovanni

[rlippert]

Reworked version of chibios type patch that only use the typedef on TX operations (which is the only case that make sense for AVR named address spaces atleast) so does not need typeof() operator:

Code: Select all

From 0acccc56d5cf9073d38278d89ac8aa417656b3a2 Mon Sep 17 00:00:00 2001
From: Rob Lippert <[email protected]>
Date: Sat, 7 May 2016 19:36:26 -0700
Subject: [PATCH] hal/usb: allow ports to override type of usb buffer pointer

This allows AVR port to use named address spaces to store USB
descriptors in e.g. flash/eeprom and transmit them directly
without bouncing through a memory buffer.

Signed-off-by: Rob Lippert <[email protected]>
---
 os/hal/include/hal_usb.h                   |  4 ++--
 os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h |  5 +++++
 os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h |  5 +++++
 os/hal/src/hal_usb.c                       | 10 +++++-----
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/os/hal/include/hal_usb.h b/os/hal/include/hal_usb.h
index 0e9ecb2..81f170d 100644
--- a/os/hal/include/hal_usb.h
+++ b/os/hal/include/hal_usb.h
@@ -609,10 +609,10 @@ extern "C" {
   void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
                         uint8_t *buf, size_t n);
   void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
-                         const uint8_t *buf, size_t n);
+                         const usbbufptr_t buf, size_t n);
 #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 usbTransmit(USBDriver *usbp, usbep_t ep, const usbbufptr_t buf, size_t n);
 #endif
   bool usbStallReceiveI(USBDriver *usbp, usbep_t ep);
   bool usbStallTransmitI(USBDriver *usbp, usbep_t ep);
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 1132b8e..4cb7ae4 100644
--- a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
@@ -204,6 +204,11 @@
 /*===========================================================================*/
 
 /**
+ * @brief   Peripheral-specific arbitrary byte data pointer.
+ */
+typedef uint8_t *usbbufptr_t;
+
+/**
  * @brief   Peripheral-specific parameters block.
  */
 typedef struct {
diff --git a/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h b/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
index 2cd77dd..74579ed 100644
--- a/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/USBv1/hal_usb_lld.h
@@ -153,6 +153,11 @@
 /*===========================================================================*/
 
 /**
+ * @brief   Peripheral-specific arbitrary byte data pointer.
+ */
+typedef uint8_t *usbbufptr_t;
+
+/**
  * @brief   Type of an IN endpoint state structure.
  */
 typedef struct {
diff --git a/os/hal/src/hal_usb.c b/os/hal/src/hal_usb.c
index bc0cf08..7d02341 100644
--- a/os/hal/src/hal_usb.c
+++ b/os/hal/src/hal_usb.c
@@ -431,8 +431,7 @@ void usbDisableEndpointsI(USBDriver *usbp) {
  *
  * @iclass
  */
-void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
-                      uint8_t *buf, size_t n) {
+void usbStartReceiveI(USBDriver *usbp, usbep_t ep, uint8_t *buf, size_t n) {
   USBOutEndpointState *osp;
 
   osalDbgCheckClassI();
@@ -471,7 +470,7 @@ void usbStartReceiveI(USBDriver *usbp, usbep_t ep,
  * @iclass
  */
 void usbStartTransmitI(USBDriver *usbp, usbep_t ep,
-                       const uint8_t *buf, size_t n) {
+                       const usbbufptr_t buf, size_t n) {
   USBInEndpointState *isp;
 
   osalDbgCheckClassI();
@@ -547,7 +546,8 @@ msg_t usbReceive(USBDriver *usbp, usbep_t ep, uint8_t *buf, size_t n) {
  *
  * @api
  */
-msg_t usbTransmit(USBDriver *usbp, usbep_t ep, const uint8_t *buf, size_t n) {
+msg_t usbTransmit(USBDriver *usbp, usbep_t ep,
+                  const usbbufptr_t *buf, size_t n) {
   msg_t msg;
 
   osalSysLock();
@@ -801,7 +801,7 @@ void _usb_ep0setup(USBDriver *usbp, usbep_t ep) {
       /* Starts the receive phase.*/
       usbp->ep0state = USB_EP0_RX;
       osalSysLockFromISR();
-      usbStartReceiveI(usbp, 0, usbp->ep0next, usbp->ep0n);
+      usbStartReceiveI(usbp, 0, (uint8_t *) usbp->ep0next, usbp->ep0n);
       osalSysUnlockFromISR();
     }
     else {
-- 
2.5.3

(I haven't tested the ST USB drivers to see if they still compile since I don't have a ARM compiler handy, but I think they should)

[utzig]

I applied the patch today against SVN trunk. Also added small fixes for ATmega32U4 and added a demo for the Sparkfun Pro Micro board.

Now will wait for someone to contribute a patch with support for Arduino Leonardo which uses the same mega32U4 CPU, so quite easy to do!

Btw, Signed-off-by was correctly mapped to github username on the mirror (first time we tested this since adding it to the clone script)!

Cheers,
Fabio Utzig

[devlware]

Hi Fabio!

Can you check this patch with Leonardo support files?

From make:

Size after:
AVR Memory Usage
----------------
Device: atmega32u4

Program: 13568 bytes (41.4% Full)
(.text + .data + .bootloader)

Data: 1112 bytes (43.4% Full)
(.data + .bss + .noinit)

Diego