dap thread: multiple bugfixes

- Fix obvious bug with multi-packet pending response lengths
  The length needs to go along with the data buf, not a random global

- Move to async notifications instead of polling or hacky suspend/resume
  For a tick interval ~= the time taken to transmit one full bulk packet,
  you could get a NAK and flow-controlled if usb_thread went to sleep

- Remove redundant memcpys and unused variables

- Hack the reported DAP packet count to avoid bulk-OUT/NAK conditions
  as much as possible

Author: P33M

CMSIS_DAP/CMSIS/DAP/Firmware/Source/DAP.c

@@ -132,7 +132,7 @@ static uint8_t DAP_Info(uint8_t id, uint8_t *info) {
       length = 2U;
       break;
     case DAP_ID_PACKET_COUNT:
-      info[0] = DAP_PACKET_COUNT;
+      info[0] = DAP_PACKET_COUNT - 2; // HACK to avoid ring buffer fullness
       length = 1U;
       break;
     default:

include/DAP_config.h

@@ -96,7 +96,7 @@ This information includes:
 /// This configuration settings is used to optimize the communication performance with the
 /// debugger and depends on the USB peripheral. For devices with limited RAM or USB buffer the
 /// setting can be reduced (valid range is 1 .. 255).
-#define DAP_PACKET_COUNT        2U              ///< Specifies number of packets buffered.
+#define DAP_PACKET_COUNT        8U              ///< Specifies number of packets buffered.
 
 /// Indicate that UART Serial Wire Output (SWO) trace is available.
 /// This information is returned by the command \ref DAP_Info as part of <b>Capabilities</b>.

src/main.c

@@ -94,8 +94,21 @@ void dev_mon(void *ptr)
     } while (1);
 }
 
+void tud_event_hook_cb(uint8_t rhport, uint32_t eventid, bool in_isr)
+{
+  (void) rhport;
+  (void) eventid;
+  BaseType_t blah;
+  if (in_isr) {
+    xTaskNotifyFromISR(tud_taskhandle, 0, 0, &blah);
+  } else {
+    xTaskNotify(tud_taskhandle, 0, 0);
+  }
+}
+
 void usb_thread(void *ptr)
 {
+  uint32_t cmd;
 #ifdef PROBE_USB_CONNECTED_LED
     gpio_init(PROBE_USB_CONNECTED_LED);
     gpio_set_dir(PROBE_USB_CONNECTED_LED, GPIO_OUT);
@@ -113,9 +126,10 @@ void usb_thread(void *ptr)
         // If suspended or disconnected, delay for 1ms (20 ticks)
         if (tud_suspended() || !tud_connected())
             xTaskDelayUntil(&wake, 20);
-        // Go to sleep for up to a tick if nothing to do
+        // Go to sleep if nothing to do
         else if (!tud_task_event_ready())
-            xTaskDelayUntil(&wake, 1);
+          xTaskNotifyWait(0, 0xFFFFFFFFu, &cmd, 1);
+
     } while (1);
 }
 
@@ -282,7 +296,7 @@ void tud_mount_cb(void)
     xTaskCreate(autobaud_thread, "ABR", configMINIMAL_STACK_SIZE, NULL, AUTOBAUD_TASK_PRIO, &autobaud_taskhandle);
 #if(configNUMBER_OF_CORES > 1)
     vTaskCoreAffinitySet(autobaud_taskhandle, (1 << 1));
-    vTaskCoreAffinitySet(dap_taskhandle, (1 << 0));
+    vTaskCoreAffinitySet(dap_taskhandle, (1 << 1));
     vTaskCoreAffinitySet(uart_taskhandle, (1 << 0));
 #endif
     was_configured = 1;

src/tusb_edpt_handler.c

@@ -6,20 +6,20 @@
 
 #include "tusb_edpt_handler.h"
 #include "DAP.h"
+#include "semphr.h"
+
 
 static uint8_t itf_num;
 static uint8_t _rhport;
 
-volatile uint32_t _resp_len;
 
 static uint8_t _out_ep_addr;
 static uint8_t _in_ep_addr;
 
 static buffer_t USBRequestBuffer;
 static buffer_t USBResponseBuffer;
 
-static uint8_t DAPRequestBuffer[DAP_PACKET_SIZE];
-static uint8_t DAPResponseBuffer[DAP_PACKET_SIZE];
+static SemaphoreHandle_t edpt_spoon;
 
 #define WR_IDX(x) (x.wptr % DAP_PACKET_COUNT)
 #define RD_IDX(x) (x.rptr % DAP_PACKET_COUNT)
@@ -38,15 +38,15 @@ bool buffer_empty(buffer_t *buffer)
 }
 
 void dap_edpt_init(void) {
-
+	edpt_spoon = xSemaphoreCreateMutex();
+	xSemaphoreGive(edpt_spoon);
 }
 
 bool dap_edpt_deinit(void)
 {
-	memset(DAPRequestBuffer, 0, sizeof(DAPRequestBuffer));
-	memset(DAPResponseBuffer, 0, sizeof(DAPResponseBuffer));
-	USBRequestBuffer.wptr = USBRequestBuffer.rptr = 0;
-	USBResponseBuffer.wptr = USBResponseBuffer.rptr = 0;
+	memset(&USBRequestBuffer, 0, sizeof(USBRequestBuffer));
+	memset(&USBResponseBuffer, 0, sizeof(USBResponseBuffer));
+	vSemaphoreDelete(edpt_spoon);
 	return true;
 }
 
@@ -131,6 +131,8 @@ uint16_t dap_edpt_open(uint8_t __unused rhport, tusb_desc_interface_t const *itf
 	// The IN endpoint doesn't need a transfer to initialise it, as this will be done by the main loop of dap_thread
 	usbd_edpt_open(rhport, edpt_desc);
 
+	// Spawn DAP thread?
+
 	return drv_len;
 
 }
@@ -149,29 +151,29 @@ bool dap_edpt_xfer_cb(uint8_t __unused rhport, uint8_t ep_addr, xfer_result_t re
 	{
 		if(xferred_bytes >= 0u && xferred_bytes <= DAP_PACKET_SIZE)
 		{
+			xSemaphoreTake(edpt_spoon, portMAX_DELAY);
 			USBResponseBuffer.rptr++;
-
 			// This checks that the buffer was not empty in DAP thread, which means the next buffer was not queued up for the in endpoint callback
 			// So, queue up the buffer at the new read index, since we expect read to catch up to write at this point.
 			// It is possible for the read index to be multiple spaces behind the write index (if the USB callbacks are lagging behind dap thread),
 			// so we account for this by only setting wasEmpty to true if the next callback will empty the buffer
 			if(!USBResponseBuffer.wasEmpty)
 			{
-				usbd_edpt_xfer(rhport, ep_addr, RD_SLOT_PTR(USBResponseBuffer), (uint16_t) _resp_len);
+				usbd_edpt_xfer(rhport, ep_addr, RD_SLOT_PTR(USBResponseBuffer), USBResponseBuffer.data_len[RD_IDX(USBResponseBuffer)]);
 				USBResponseBuffer.wasEmpty = (USBResponseBuffer.rptr + 1) == USBResponseBuffer.wptr;
 			}
-
+			xSemaphoreGive(edpt_spoon);
 			//  Wake up DAP thread after processing the callback
-			vTaskResume(dap_taskhandle);
+			xTaskNotify(dap_taskhandle, 0, eSetValueWithOverwrite);
 			return true;
 		}
-
 		return false;
 
-	} else if(ep_dir == TUSB_DIR_OUT)    {
+	} else if(ep_dir == TUSB_DIR_OUT) {
 
 		if(xferred_bytes >= 0u && xferred_bytes <= DAP_PACKET_SIZE)
 		{
+			xSemaphoreTake(edpt_spoon, portMAX_DELAY);
 			// Only queue the next buffer in the out callback if the buffer is not full
 			// If full, we set the wasFull flag, which will be checked by dap thread
 			if(!buffer_full(&USBRequestBuffer))
@@ -183,22 +185,26 @@ bool dap_edpt_xfer_cb(uint8_t __unused rhport, uint8_t ep_addr, xfer_result_t re
 			else {
 				USBRequestBuffer.wasFull = true;
 			}
-
+			xSemaphoreGive(edpt_spoon);
 			//  Wake up DAP thread after processing the callback
-			vTaskResume(dap_taskhandle);
+			xTaskNotify(dap_taskhandle, 0, eSetValueWithOverwrite);
 			return true;
 		}
-
 		return false;
 	}
-	else return false;
+	return false;
 }
 
 void dap_thread(void *ptr)
 {
 	uint32_t n;
+	uint32_t cmd;
+	uint16_t resp_len;
 	do
 	{
+		// Wait for usb CB wake
+		xTaskNotifyWait(0, 0xFFFFFFFFu, &cmd, 1);
+
 		while(USBRequestBuffer.rptr != USBRequestBuffer.wptr)
 		{
 			/*
@@ -219,53 +225,45 @@ void dap_thread(void *ptr)
 				}
 			}
 			// Read a single packet from the USB buffer into the DAP Request buffer
-			memcpy(DAPRequestBuffer, RD_SLOT_PTR(USBRequestBuffer), DAP_PACKET_SIZE);
 			probe_info("%lu %lu DAP cmd %s len %02x\n",
-				       USBRequestBuffer.wptr, USBRequestBuffer.rptr,
-				       dap_cmd_string[DAPRequestBuffer[0]], DAPRequestBuffer[1]);
-			USBRequestBuffer.rptr++;
+					   USBRequestBuffer.wptr, USBRequestBuffer.rptr,
+					   dap_cmd_string[RD_SLOT_PTR(USBRequestBuffer)[0], RD_SLOT_PTR(USBRequestBuffer)[1]]);
 
 			// If the buffer was full in the out callback, we need to queue up another buffer for the endpoint to consume, now that we know there is space in the buffer.
+			xSemaphoreTake(edpt_spoon, portMAX_DELAY); // Suspend the scheduler to safely update the write index
 			if(USBRequestBuffer.wasFull)
 			{
-				vTaskSuspendAll(); // Suspend the scheduler to safely update the write index
 				USBRequestBuffer.wptr++;
 				usbd_edpt_xfer(_rhport, _out_ep_addr, WR_SLOT_PTR(USBRequestBuffer), DAP_PACKET_SIZE);
 				USBRequestBuffer.wasFull = false;
-				xTaskResumeAll();
 			}
+			xSemaphoreGive(edpt_spoon);
 
-			_resp_len = DAP_ExecuteCommand(DAPRequestBuffer, DAPResponseBuffer);
-			probe_info("%lu %lu DAP resp %s\n",
-					USBResponseBuffer.wptr, USBResponseBuffer.rptr,
-					dap_cmd_string[DAPResponseBuffer[0]]);
-
+			resp_len = DAP_ExecuteCommand(RD_SLOT_PTR(USBRequestBuffer), WR_SLOT_PTR(USBResponseBuffer)) & 0xffff;
+			USBRequestBuffer.rptr++;
+			probe_info("%lu %lu DAP resp %s len %u\n",
+					   USBResponseBuffer.wptr, USBResponseBuffer.rptr,
+					   dap_cmd_string[WR_SLOT_PTR(USBResponseBuffer)[0], resp_len);
 
+			USBResponseBuffer.data_len[WR_IDX(USBResponseBuffer)] = resp_len;
 			//  Suspend the scheduler to avoid stale values/race conditions between threads
-			vTaskSuspendAll();
+			xSemaphoreTake(edpt_spoon, portMAX_DELAY);
 
 			if(buffer_empty(&USBResponseBuffer))
 			{
-				memcpy(WR_SLOT_PTR(USBResponseBuffer), DAPResponseBuffer, (uint16_t) _resp_len);
 				USBResponseBuffer.wptr++;
 
-				usbd_edpt_xfer(_rhport, _in_ep_addr, RD_SLOT_PTR(USBResponseBuffer), (uint16_t) _resp_len);
+				usbd_edpt_xfer(_rhport, _in_ep_addr, RD_SLOT_PTR(USBResponseBuffer), USBResponseBuffer.data_len[RD_IDX(USBResponseBuffer)]);
 			} else {
 
-				memcpy(WR_SLOT_PTR(USBResponseBuffer), DAPResponseBuffer, (uint16_t) _resp_len);
 				USBResponseBuffer.wptr++;
 
 				// The In callback needs to check this flag to know when to queue up the next buffer.
 				USBResponseBuffer.wasEmpty = false;
 			}
-			xTaskResumeAll();
+			xSemaphoreGive(edpt_spoon);
 		}
-
-		// Suspend DAP thread until it is awoken by a USB thread callback
-		vTaskSuspend(dap_taskhandle);
-
 	} while (1);
-
 }
 
 usbd_class_driver_t const _dap_edpt_driver =

src/tusb_edpt_handler.h

@@ -17,6 +17,7 @@
 
 typedef struct {
 	uint8_t data[DAP_PACKET_COUNT][DAP_PACKET_SIZE];
+	uint16_t data_len[DAP_PACKET_COUNT];
 	volatile uint32_t wptr;
 	volatile uint32_t rptr;
 	volatile bool wasEmpty;