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
diff --git a/CMSIS_DAP/CMSIS/DAP/Firmware/Source/DAP.c b/CMSIS_DAP/CMSIS/DAP/Firmware/Source/DAP.c index 9e1f620..4c6293f 100644 --- a/CMSIS_DAP/CMSIS/DAP/Firmware/Source/DAP.c +++ b/CMSIS_DAP/CMSIS/DAP/Firmware/Source/DAP.c
@@ -132,7 +132,7 @@ 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:
diff --git a/include/DAP_config.h b/include/DAP_config.h index 88c11d9..1a7ac81 100755 --- a/include/DAP_config.h +++ b/include/DAP_config.h
@@ -96,7 +96,7 @@ /// 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>.
diff --git a/src/main.c b/src/main.c index 72fc5bb..06019f9 100644 --- a/src/main.c +++ b/src/main.c
@@ -94,8 +94,21 @@ } 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 @@ // 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 @@ 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;
diff --git a/src/tusb_edpt_handler.c b/src/tusb_edpt_handler.c index 6769868..59300d2 100644 --- a/src/tusb_edpt_handler.c +++ b/src/tusb_edpt_handler.c
@@ -6,11 +6,12 @@ #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; @@ -18,8 +19,7 @@ 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 @@ } 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 @@ // 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 @@ { 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 @@ 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 @@ } } // 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 =
diff --git a/src/tusb_edpt_handler.h b/src/tusb_edpt_handler.h index be1fdd0..dfc8b9e 100644 --- a/src/tusb_edpt_handler.h +++ b/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;