Fix USB-serial TX flow control

The CDC-ACM bridge now appropriately NAKs incoming packets from the host when the UART TX buffer fills up instead of overflowing the buffer. The UART TX buffer now uses unmasked indices to simplify calculating the remaining space available.
devanlai · Apr 28, 2017 · 627b605 · 627b605
1 parent c9946fd
commit 627b605
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 35 deletions.
diff --git a/src/DAP/app.c b/src/DAP/app.c
@@ -35,9 +35,11 @@ static uint8_t outbox_head;
 
 static GenericCallback dfu_request_callback = NULL;
 
-static void on_receive_report(uint8_t* data, uint16_t len) {
+static bool on_receive_report(uint8_t* data, uint16_t len) {
     memcpy((void*)request_buffers[inbox_tail], (const void*)data, len);
     inbox_tail = (inbox_tail + 1) % DAP_PACKET_QUEUE_SIZE;
+
+    return ((inbox_tail + 1) % DAP_PACKET_QUEUE_SIZE) != outbox_head;
 }
 
 static void on_send_report(uint8_t* data, uint16_t* len) {

diff --git a/src/USB/cdc.c b/src/USB/cdc.c
@@ -67,8 +67,36 @@ static SetControlLineStateFunction cdc_set_control_line_state_callback = NULL;
 static SetLineCodingFunction cdc_set_line_coding_callback = NULL;
 static GetLineCodingFunction cdc_get_line_coding_callback = NULL;
 
+static usbd_device* cdc_usbd_dev;
+
+static void cdc_set_config(usbd_device *usbd_dev, uint16_t wValue);
+
+void cdc_setup(usbd_device* usbd_dev,
+               HostOutFunction cdc_rx_cb,
+               SetControlLineStateFunction set_control_line_state_cb,
+               SetLineCodingFunction set_line_coding_cb,
+               GetLineCodingFunction get_line_coding_cb) {
+    cdc_usbd_dev = usbd_dev;
+    cdc_rx_callback = cdc_rx_cb;
+    cdc_set_control_line_state_callback = set_control_line_state_cb,
+    cdc_set_line_coding_callback = set_line_coding_cb;
+    cdc_get_line_coding_callback = get_line_coding_cb;
+
+    cmp_usb_register_set_config_callback(cdc_set_config);
+}
+
 /* Generic CDC-ACM functionality */
 
+bool cdc_send_data(const uint8_t* data, size_t len) {
+    if (!cmp_usb_configured()) {
+        return false;
+    }
+    uint16_t sent = usbd_ep_write_packet(cdc_usbd_dev, ENDP_CDC_DATA_IN,
+                                         (const void*)data,
+                                         (uint16_t)len);
+    return (sent != 0);
+}
+
 static int cdc_control_class_request(usbd_device *usbd_dev,
                                      struct usb_setup_data *req,
                                      uint8_t **buf, uint16_t *len,
@@ -143,12 +171,41 @@ static int cdc_control_class_request(usbd_device *usbd_dev,
     return status;
 }
 
+/* CDC-ACM RX flow control */
+static bool cdc_rx_stalled = false;
+static void cdc_set_nak(void) {
+    if (!cdc_rx_stalled) {
+        usbd_ep_nak_set(cdc_usbd_dev, ENDP_CDC_DATA_OUT, true);
+        cdc_rx_stalled = true;
+    }
+}
+
+static void cdc_clear_nak(void) {
+    if (cdc_rx_stalled) {
+        usbd_ep_nak_set(cdc_usbd_dev, ENDP_CDC_DATA_OUT, false);
+        cdc_rx_stalled = false;
+    }
+}
+
 /* Receive data from the host */
 static void cdc_bulk_data_out(usbd_device *usbd_dev, uint8_t ep) {
+    // Force NAK to prevent the USB controller from accepting a second
+    // packet before we've processed this one to decide if have space
+    // to accept more packets.
+    cdc_set_nak();
+
     uint8_t buf[USB_CDC_MAX_PACKET_SIZE];
     uint16_t len = usbd_ep_read_packet(usbd_dev, ep, (void*)buf, sizeof(buf));
+    bool accept_more_packets = true;
     if (len > 0 && (cdc_rx_callback != NULL)) {
-        cdc_rx_callback(buf, len);
+        accept_more_packets = cdc_rx_callback(buf, len);
+    }
+
+    // Handle flow control
+    if (accept_more_packets) {
+        cdc_clear_nak();
+    } else {
+        cdc_set_nak();
     }
 }
 
@@ -164,32 +221,6 @@ static void cdc_set_config(usbd_device *usbd_dev, uint16_t wValue) {
     cmp_usb_register_control_class_callback(INTF_CDC_COMM, cdc_control_class_request);
 }
 
-static usbd_device* cdc_usbd_dev;
-
-void cdc_setup(usbd_device* usbd_dev,
-               HostOutFunction cdc_rx_cb,
-               SetControlLineStateFunction set_control_line_state_cb,
-               SetLineCodingFunction set_line_coding_cb,
-               GetLineCodingFunction get_line_coding_cb) {
-    cdc_usbd_dev = usbd_dev;
-    cdc_rx_callback = cdc_rx_cb;
-    cdc_set_control_line_state_callback = set_control_line_state_cb,
-    cdc_set_line_coding_callback = set_line_coding_cb;
-    cdc_get_line_coding_callback = get_line_coding_cb;
-
-    cmp_usb_register_set_config_callback(cdc_set_config);
-}
-
-bool cdc_send_data(const uint8_t* data, size_t len) {
-    if (!cmp_usb_configured()) {
-        return false;
-    }
-    uint16_t sent = usbd_ep_write_packet(cdc_usbd_dev, ENDP_CDC_DATA_IN,
-                                         (const void*)data,
-                                         (uint16_t)len);
-    return (sent != 0);
-}
-
 /* CDC-ACM USB UART bridge functionality */
 
 // User callbacks
@@ -260,11 +291,13 @@ static bool cdc_uart_get_line_coding(struct usb_cdc_line_coding* line_coding) {
     return true;
 }
 
-static void cdc_uart_on_host_tx(uint8_t* data, uint16_t len) {
+static bool cdc_uart_on_host_tx(uint8_t* data, uint16_t len) {
     console_send_buffered(data, (size_t)len);
     if (cdc_uart_rx_callback) {
         cdc_uart_rx_callback();
     }
+
+    return (console_send_buffer_space() >= USB_CDC_MAX_PACKET_SIZE);
 }
 
 static uint16_t packet_len = 0;
@@ -277,6 +310,7 @@ void cdc_uart_app_reset(void) {
     packet_len = 0;
     packet_timestamp = get_ticks();
     need_zlp = false;
+    cdc_clear_nak();
 }
 
 void cdc_uart_app_setup(usbd_device* usbd_dev,
@@ -299,6 +333,7 @@ void cdc_uart_app_set_timeout(uint32_t timeout_ms) {
 bool cdc_uart_app_update() {
     bool active = false;
 
+    // Handle sending received data to the host
     if (packet_len < USB_CDC_MAX_PACKET_SIZE) {
         uint16_t max_bytes = (USB_CDC_MAX_PACKET_SIZE- packet_len);
         packet_len += console_recv_buffered(&packet_buffer[packet_len], max_bytes);
@@ -326,6 +361,11 @@ bool cdc_uart_app_update() {
         }
     }
 
+    // Handle flow control for data received from the host
+    if (console_send_buffer_space() >= USB_CDC_MAX_PACKET_SIZE) {
+        cdc_clear_nak();
+    }
+
     return active;
 }
 

diff --git a/src/USB/hid.c b/src/USB/hid.c
@@ -126,6 +126,7 @@ static int hid_control_class_request(usbd_device *usbd_dev,
         case USB_HID_REQ_SET_REPORT: {
             if ((hid_report_out_callback != NULL) && (*len > 0))
             {
+                // TODO: flow control
                 hid_report_out_callback(*buf, *len);
                 status = USBD_REQ_HANDLED;
             }
@@ -156,6 +157,7 @@ static void hid_interrupt_in(usbd_device *usbd_dev, uint8_t ep) {
 static void hid_interrupt_out(usbd_device *usbd_dev, uint8_t ep) {
     uint8_t buf[USB_HID_MAX_PACKET_SIZE];
     uint16_t len = usbd_ep_read_packet(usbd_dev, ep, (void*)buf, sizeof(buf));
+    // TODO: flow control
     if (len > 0 && (hid_report_out_callback != NULL)) {
         hid_report_out_callback(buf, len);
     }

diff --git a/src/USB/usb_common.h b/src/USB/usb_common.h
@@ -25,7 +25,7 @@
 #include <libopencm3/usb/usbd.h>
 
 typedef void (*GenericCallback)(void);
-typedef void (*HostOutFunction)(uint8_t* data, uint16_t len);
+typedef bool (*HostOutFunction)(uint8_t* data, uint16_t len);
 typedef void (*HostInFunction)(uint8_t* data, uint16_t* len);
 
 #endif
diff --git a/src/console.c b/src/console.c
@@ -83,17 +83,17 @@ static bool console_tx_buffer_empty(void) {
 }
 
 static bool console_tx_buffer_full(void) {
-    return console_tx_head == ((console_tx_tail + 1) % CONSOLE_TX_BUFFER_SIZE);
+    return (uint16_t)(console_tx_tail - console_tx_head) == CONSOLE_TX_BUFFER_SIZE;
 }
 
 static void console_tx_buffer_put(uint8_t data) {
-    console_tx_buffer[console_tx_tail] = data;
-    console_tx_tail = (console_tx_tail + 1) % CONSOLE_TX_BUFFER_SIZE;
+    console_tx_buffer[console_tx_tail % CONSOLE_TX_BUFFER_SIZE] = data;
+    console_tx_tail++;
 }
 
 static uint8_t console_tx_buffer_get(void) {
-    uint8_t data = console_tx_buffer[console_tx_head];
-    console_tx_head = (console_tx_head + 1) % CONSOLE_TX_BUFFER_SIZE;
+    uint8_t data = console_tx_buffer[console_tx_head % CONSOLE_TX_BUFFER_SIZE];
+    console_tx_head++;
     return data;
 }
 
@@ -102,6 +102,10 @@ void console_tx_buffer_clear(void) {
     console_tx_tail = 0;
 }
 
+size_t console_send_buffer_space(void) {
+    return CONSOLE_TX_BUFFER_SIZE - (uint16_t)(console_tx_tail - console_tx_head);
+}
+
 static bool console_rx_buffer_empty(void) {
     return console_rx_head == console_rx_tail;
 }

diff --git a/src/console.h b/src/console.h
@@ -32,5 +32,6 @@ extern void console_send_blocking(uint8_t data);
 extern uint8_t console_recv_blocking(void);
 extern size_t console_send_buffered(const uint8_t* data, size_t num_bytes);
 extern size_t console_recv_buffered(uint8_t* data, size_t max_bytes);
+extern size_t console_send_buffer_space(void);
 
 #endif