Fix serial file transfer on macOS and 3rd party serial tool

firmware/application/usb_serial_cdc.c

@@ -58,18 +58,14 @@ CH_IRQ_HANDLER(USB0_IRQHandler) {
 
     usb0_isr();
 
-    if (status & USB0_USBSTS_D_UI) {
-#ifdef PRALINE
-        if (thread_usb_event) {
-            chSysLockFromIsr();
-            chEvtSignalI(thread_usb_event, EVT_MASK_USB);
-            chSysUnlockFromIsr();
-        }
-#else
+    /* Signal unconditionally: usb0_isr() reads and clears USBSTS itself, so a
+     * transfer completion arriving between our read above and its read would
+     * be processed but missing from `status`, and the event thread would not
+     * be woken to consume it. */
+    if (thread_usb_event) {
         chSysLockFromIsr();
         chEvtSignalI(thread_usb_event, EVT_MASK_USB);
         chSysUnlockFromIsr();
-#endif
     }
 
     if (status & USB0_USBSTS_D_SLI) {
@@ -102,8 +98,17 @@ void usb_configuration_changed(usb_device_t* const device) {
     (void)device;
 
     usb_endpoint_init(&usb_endpoint_int_in, false);
-    usb_endpoint_init(&usb_endpoint_bulk_in, false);
+    /* Enable ZLP on bulk IN: transfers that are an exact multiple of
+     * wMaxPacketSize (512) need a zero-length packet so hosts reading with
+     * buffers larger than one packet see the transfer terminate. */
+    usb_endpoint_init(&usb_endpoint_bulk_in, true);
     usb_endpoint_init(&usb_endpoint_bulk_out, false);
+
+    /* Consider the channel open as soon as the host configures the device.
+     * Waiting only for SET_CONTROL_LINE_STATE (DTR) leaves the bulk OUT
+     * endpoint unprimed for hosts/tools that never assert DTR, which then
+     * hang on their first command. */
+    on_channel_opened();
 }
 
 void setup_usb_serial_controller(void) {
@@ -154,12 +159,22 @@ usb_request_status_t usb_class_request(usb_endpoint_t* const endpoint, const usb
     return status;
 }
 
+/* CDC line coding (7 bytes): dwDTERate, bCharFormat, bParityType, bDataBits.
+ * The baud rate is meaningless for a native USB device but strict hosts
+ * (e.g. macOS) expect GET_LINE_CODING to return the full structure rather
+ * than a zero-length response. Defaults to 115200 8N1. */
+static uint8_t cdc_line_coding[7] = {0x00, 0xC2, 0x01, 0x00, 0x00, 0x00, 0x08};
+
 usb_request_status_t usb_get_line_coding_request(usb_endpoint_t* const endpoint, const usb_transfer_stage_t stage) {
     if (stage == USB_TRANSFER_STAGE_SETUP) {
+        uint16_t length = (endpoint->setup.length_h << 8) | endpoint->setup.length_l;
+        if (length > sizeof(cdc_line_coding))
+            length = sizeof(cdc_line_coding);
+
         usb_transfer_schedule_block(
             endpoint->in,
-            &endpoint->buffer,
-            0,
+            cdc_line_coding,
+            length,
             NULL,
             NULL);
     } else if (stage == USB_TRANSFER_STAGE_DATA) {
@@ -180,13 +195,20 @@ usb_request_status_t usb_set_control_line_state_request(usb_endpoint_t* const en
 
 usb_request_status_t usb_set_line_coding_request(usb_endpoint_t* const endpoint, const usb_transfer_stage_t stage) {
     if (stage == USB_TRANSFER_STAGE_SETUP) {
+        /* Setting the line coding means a host application opened the port;
+         * treat it like DTR so the channel recovers after a bus suspend even
+         * with tools that never assert DTR. */
+        on_channel_opened();
+
         usb_transfer_schedule_block(
             endpoint->out,
             &endpoint->buffer,
-            32,
+            sizeof(endpoint->buffer),
             NULL,
             NULL);
     } else if (stage == USB_TRANSFER_STAGE_DATA) {
+        /* Remember what the host set so GET_LINE_CODING can echo it back. */
+        memcpy(cdc_line_coding, (const void*)endpoint->buffer, sizeof(cdc_line_coding));
         usb_transfer_schedule_ack(endpoint->in);
     }
 

firmware/application/usb_serial_descriptor.c

@@ -30,7 +30,10 @@
 #define USB_WORD(x) (x & 0xFF), ((x >> 8) & 0xFF)
 #define USB_MAX_PACKET0 (64)
 #define USB_MAX_PACKET_BULK_FS (64)
-#define USB_MAX_PACKET_BULK_HS (64)
+/* USB 2.0 requires 512 for high-speed bulk endpoints. Hosts that enforce
+ * the spec (e.g. macOS) send packets larger than a non-compliant 64-byte
+ * wMaxPacketSize, which the device controller then drops. */
+#define USB_MAX_PACKET_BULK_HS (512)
 #define USB_STRING_LANGID (0x0409)
 
 uint8_t usb_descriptor_device[] = {
@@ -201,9 +204,9 @@ uint8_t usb_descriptor_configuration_high_speed[] = {
     7,                             // bLength
     USB_DESCRIPTOR_TYPE_ENDPOINT,  // bDescriptorType
     USB_INT_IN_EP_ADDR,            // bEndpointAddress
-    0x03,                          // bmAttributes: BULK
+    0x03,                          // bmAttributes: INTERRUPT
     USB_WORD(16),                  // wMaxPacketSize
-    0x20,                          // bInterval: no NAK
+    0x09,                          // bInterval: 2^(9-1) microframes = 32ms (HS range is 1..16)
 
     9,                              // bLength
     USB_DESCRIPTOR_TYPE_INTERFACE,  // bDescriptorType

firmware/application/usb_serial_device_to_host.c

@@ -40,12 +40,30 @@
 
 SerialUSBDriver SUSBD1;
 
+/* TX buffers handed to the USB DMA engine. They must stay valid until the
+ * transfer retires, so they cannot live on the caller's stack. Two buffers
+ * are alternated: with a transfer pool of 1, usb_transfer_schedule() only
+ * succeeds once the previous transfer has retired, so by the time a buffer
+ * is refilled the transfer-before-last that used it is guaranteed done. */
+static uint8_t tx_buffers[2][USBSERIAL_BUFFERS_SIZE];
+static uint8_t tx_buffer_index = 0;
+static bool tx_draining = FALSE;
+
 static void onotify(GenericQueue* qp) {
     SerialUSBDriver* sdp = chQGetLink(qp);
-    uint8_t buff[USBSERIAL_BUFFERS_SIZE];
-    int n = chOQGetFullI(&sdp->oqueue);
-    if (n > USBSERIAL_BUFFERS_SIZE) n = USBSERIAL_BUFFERS_SIZE;  // don't overflow
-    if (n > 0) {
+
+    /* Called with the system locked. Another thread may already be draining
+     * inside the unlocked window below; its loop will pick up our bytes. */
+    if (tx_draining)
+        return;
+    tx_draining = TRUE;
+
+    int n;
+    while ((n = chOQGetFullI(&sdp->oqueue)) > 0) {
+        if (n > USBSERIAL_BUFFERS_SIZE) n = USBSERIAL_BUFFERS_SIZE;  // don't overflow
+
+        uint8_t* buff = tx_buffers[tx_buffer_index];
+        tx_buffer_index ^= 1;
         for (int i = 0; i < n; i++) {
             buff[i] = chOQGetI(&sdp->oqueue);
         }
@@ -66,6 +84,8 @@ static void onotify(GenericQueue* qp) {
         } while (ret == -1);
         chSysLock();
     }
+
+    tx_draining = FALSE;
 }
 
 static size_t write(void* ip, const uint8_t* bp, size_t n) {

firmware/application/usb_serial_device_to_host.h

@@ -25,10 +25,12 @@
 #include "ch.h"
 #include "hal.h"
 
-#define USB_BULK_BUFFER_SIZE 64
+/* Must be >= the high-speed bulk wMaxPacketSize (512): a bulk OUT transfer
+ * descriptor smaller than one max-size packet cannot receive it. */
+#define USB_BULK_BUFFER_SIZE 512
 
 #ifndef USBSERIAL_BUFFERS_SIZE
-#define USBSERIAL_BUFFERS_SIZE 128
+#define USBSERIAL_BUFFERS_SIZE 512
 #endif
 
 struct SerialUSBDriverVMT {

firmware/application/usb_serial_endpoints.c

@@ -69,6 +69,8 @@ usb_endpoint_t usb_endpoint_bulk_in = {
     .out = 0,
     .setup_complete = 0,
     .transfer_complete = usb_queue_transfer_complete};
+/* Pool size must stay 1: the ping-pong TX buffers in
+ * usb_serial_device_to_host.c rely on only one IN transfer being in flight. */
 static USB_DEFINE_QUEUE(usb_endpoint_bulk_in, 1);
 
 usb_endpoint_t usb_endpoint_bulk_out = {
@@ -78,4 +80,4 @@ usb_endpoint_t usb_endpoint_bulk_out = {
     .out = &usb_endpoint_bulk_out,
     .setup_complete = 0,
     .transfer_complete = usb_queue_transfer_complete};
-static USB_DEFINE_QUEUE(usb_endpoint_bulk_out, 1);
+static USB_DEFINE_QUEUE(usb_endpoint_bulk_out, 2);

firmware/application/usb_serial_host_to_device.cpp

@@ -37,8 +37,9 @@ usb_serial_input_handler_t usb_serial_active_input_handler = nullptr;
 
 struct usb_bulk_buffer_t {
     uint8_t* data;
-    size_t length;
-    bool completed;
+    /* written from the USB ISR completion callback, read from thread context */
+    volatile size_t length;
+    volatile bool completed;
 };
 
 std::queue<usb_bulk_buffer_t*> usb_bulk_buffer_queue;

firmware/application/usb_serial_host_to_device.hpp

@@ -24,9 +24,7 @@
 
 #include "ch.h"
 #include "hal.h"
-#include "usb_serial_device_to_host.h"
-
-#define USB_BULK_BUFFER_SIZE 64
+#include "usb_serial_device_to_host.h" /* provides USB_BULK_BUFFER_SIZE */
 
 void init_host_to_device();
 void reset_transfer_queues();

firmware/application/usb_serial_shell_filesystem.cpp

@@ -371,7 +371,9 @@ void cmd_sd_write_binary(BaseSequentialStream* chp, int argc, char* argv[]) {
 
     chprintf(chp, "send %d bytes\r\n", size);
 
-    uint8_t buffer[USB_BULK_BUFFER_SIZE];
+    /* static: 512 bytes is too large for the shell thread stack, and only one
+     * file can be open through the shell at a time (shell_file is global). */
+    static uint8_t buffer[USB_BULK_BUFFER_SIZE];
 
     do {
         size_t bytes_to_read = size > USB_BULK_BUFFER_SIZE ? USB_BULK_BUFFER_SIZE : size;