main: handle RP2040's broken USB error handling

RP2040-E15 can also be triggered if a Debug Probe is connected to a
board with a floating ground. Typically this causes port ESD protection
to temporarily activate, meaning the Dp/Dm state gets corrupted. If this
happens in the middle of a handshake packet, the SIE can lock up.

The only way to detect this case is if SOF_RD stops advancing without a
corresponding suspend interrupt - so add a watchdog thread that forces a
disconnect if the hardware stops reporting frame counts.

This is disruptive, but immediate notification that the probe broke is
preferable to silently failing until the next character is sent by the host.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>

.gitmodules

@@ -3,4 +3,4 @@
 	url = https://github.com/ARM-software/CMSIS_5
 [submodule "freertos"]
 	path = freertos
-	url = https://github.com/FreeRTOS/FreeRTOS-Kernel
+	url = https://github.com/raspberrypi/FreeRTOS-Kernel

CMakeLists.txt

@@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.12)
 include(pico_sdk_import.cmake)
 
 set(FREERTOS_KERNEL_PATH ${CMAKE_CURRENT_LIST_DIR}/freertos)
-include(FreeRTOS_Kernel_import.cmake)
+include(freertos/portable/ThirdParty/GCC/RP2350_ARM_NTZ/FreeRTOS_Kernel_import.cmake)
 
 project(debugprobe)
 
@@ -15,6 +15,7 @@ endif ()
 
 add_executable(debugprobe
         src/probe_config.c
+        src/led.c
         src/main.c
         src/usb_descriptors.c
         src/probe.c
@@ -51,20 +52,12 @@ target_compile_definitions (debugprobe PRIVATE
 
 option (DEBUG_ON_PICO "Compile firmware for the Pico instead of Debug Probe" OFF)
 if (DEBUG_ON_PICO)
-    target_compile_definitions (debugprobe PRIVATE
+    target_compile_definitions (debugprobe PRIVATE 
 	DEBUG_ON_PICO=1
     )
-    if (PICO_BOARD STREQUAL "pico")
-        set_target_properties(debugprobe PROPERTIES
-            OUTPUT_NAME "debugprobe_on_pico"
-        )
-    elseif (PICO_BOARD STREQUAL "pico2")
-        set_target_properties(debugprobe PROPERTIES
-            OUTPUT_NAME "debugprobe_on_pico2"
-        )
-    else ()
-        error("Unsupported board ${PICO_BOARD}" PICO_BOARD)
-    endif ()
+    set_target_properties(debugprobe PROPERTIES 
+        OUTPUT_NAME "debugprobe_on_pico"
+    )
 endif ()
 
 

FreeRTOS_Kernel_import.cmake

@@ -1,92 +0,0 @@
-# SPDX-License-Identifier: BSD-3-clause
-# This is a copy of <FREERTOS_KERNEL_PATH>/portable/ThirdParty/GCC/RP2040/FREERTOS_KERNEL_import.cmake
-
-# This can be dropped into an external project to help locate the FreeRTOS kernel
-# It should be include()ed prior to project(). Alternatively this file may
-# or the CMakeLists.txt in this directory may be included or added via add_subdirectory
-# respectively.
-
-if (DEFINED ENV{FREERTOS_KERNEL_PATH} AND (NOT FREERTOS_KERNEL_PATH))
-    set(FREERTOS_KERNEL_PATH $ENV{FREERTOS_KERNEL_PATH})
-    message("Using FREERTOS_KERNEL_PATH from environment ('${FREERTOS_KERNEL_PATH}')")
-endif ()
-
-# first pass we look in old tree; second pass we look in new tree
-foreach(SEARCH_PASS RANGE 0 1)
-    if (SEARCH_PASS)
-        # ports may be moving to submodule in the future
-        set(FREERTOS_KERNEL_RP2040_RELATIVE_PATH "portable/ThirdParty/Community-Supported-Ports/GCC")
-        set(FREERTOS_KERNEL_RP2040_BACK_PATH "../../../../..")
-    else()
-        set(FREERTOS_KERNEL_RP2040_RELATIVE_PATH "portable/ThirdParty/GCC")
-        set(FREERTOS_KERNEL_RP2040_BACK_PATH "../../../..")
-    endif()
-
-    if(PICO_PLATFORM STREQUAL "rp2040")
-        set(FREERTOS_KERNEL_RP2040_RELATIVE_PATH "${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}/RP2040")
-    else()
-        if (PICO_PLATFORM STREQUAL "rp2350-riscv")
-            set(FREERTOS_KERNEL_RP2040_RELATIVE_PATH "${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}/RP2350_RISC-V")
-        else()
-            set(FREERTOS_KERNEL_RP2040_RELATIVE_PATH "${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}/RP2350_ARM_NTZ")
-        endif()
-    endif()
-
-    if (NOT FREERTOS_KERNEL_PATH)
-        # check if we are inside the FreeRTOS kernel tree (i.e. this file has been included directly)
-        get_filename_component(_ACTUAL_PATH ${CMAKE_CURRENT_LIST_DIR} REALPATH)
-        get_filename_component(_POSSIBLE_PATH ${CMAKE_CURRENT_LIST_DIR}/${FREERTOS_KERNEL_RP2040_BACK_PATH}/${FREERTOS_KERNEL_RP2040_RELATIVE_PATH} REALPATH)
-        if (_ACTUAL_PATH STREQUAL _POSSIBLE_PATH)
-            get_filename_component(FREERTOS_KERNEL_PATH ${CMAKE_CURRENT_LIST_DIR}/${FREERTOS_KERNEL_RP2040_BACK_PATH} REALPATH)
-        endif()
-        if (_ACTUAL_PATH STREQUAL _POSSIBLE_PATH)
-            get_filename_component(FREERTOS_KERNEL_PATH ${CMAKE_CURRENT_LIST_DIR}/${FREERTOS_KERNEL_RP2040_BACK_PATH} REALPATH)
-            message("Setting FREERTOS_KERNEL_PATH to ${FREERTOS_KERNEL_PATH} based on location of FreeRTOS-Kernel-import.cmake")
-            break()
-        elseif (PICO_SDK_PATH AND EXISTS "${PICO_SDK_PATH}/../FreeRTOS-Kernel")
-            set(FREERTOS_KERNEL_PATH ${PICO_SDK_PATH}/../FreeRTOS-Kernel)
-            message("Defaulting FREERTOS_KERNEL_PATH as sibling of PICO_SDK_PATH: ${FREERTOS_KERNEL_PATH}")
-            break()
-        endif()
-    endif ()
-
-    if (NOT FREERTOS_KERNEL_PATH)
-        foreach(POSSIBLE_SUFFIX Source FreeRTOS-Kernel FreeRTOS/Source)
-            # check if FreeRTOS-Kernel exists under directory that included us
-            set(SEARCH_ROOT ${CMAKE_CURRENT_SOURCE_DIR})
-            get_filename_component(_POSSIBLE_PATH ${SEARCH_ROOT}/${POSSIBLE_SUFFIX} REALPATH)
-            if (EXISTS ${_POSSIBLE_PATH}/${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}/CMakeLists.txt)
-                get_filename_component(FREERTOS_KERNEL_PATH ${_POSSIBLE_PATH} REALPATH)
-                message("Setting FREERTOS_KERNEL_PATH to '${FREERTOS_KERNEL_PATH}' found relative to enclosing project")
-                break()
-            endif()
-        endforeach()
-        if (FREERTOS_KERNEL_PATH)
-            break()
-        endif()
-    endif()
-
-    # user must have specified
-    if (FREERTOS_KERNEL_PATH)
-        if (EXISTS "${FREERTOS_KERNEL_PATH}/${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}")
-            break()
-        endif()
-    endif()
-endforeach ()
-
-if (NOT FREERTOS_KERNEL_PATH)
-    message(FATAL_ERROR "FreeRTOS location was not specified. Please set FREERTOS_KERNEL_PATH.")
-endif()
-
-set(FREERTOS_KERNEL_PATH "${FREERTOS_KERNEL_PATH}" CACHE PATH "Path to the FreeRTOS Kernel")
-
-get_filename_component(FREERTOS_KERNEL_PATH "${FREERTOS_KERNEL_PATH}" REALPATH BASE_DIR "${CMAKE_BINARY_DIR}")
-if (NOT EXISTS ${FREERTOS_KERNEL_PATH})
-    message(FATAL_ERROR "Directory '${FREERTOS_KERNEL_PATH}' not found")
-endif()
-if (NOT EXISTS ${FREERTOS_KERNEL_PATH}/${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}/CMakeLists.txt)
-    message(FATAL_ERROR "Directory '${FREERTOS_KERNEL_PATH}' does not contain a '${PICO_PLATFORM}' port here: ${FREERTOS_KERNEL_RP2040_RELATIVE_PATH}")
-endif()
-set(FREERTOS_KERNEL_PATH ${FREERTOS_KERNEL_PATH} CACHE PATH "Path to the FreeRTOS_KERNEL" FORCE)
-
-add_subdirectory(${FREERTOS_KERNEL_PATH}/${FREERTOS_KERNEL_RP2040_RELATIVE_PATH} FREERTOS_KERNEL)

LICENSE

@@ -1,23 +0,0 @@
-Unless otherwise indicated in individual files, this project is licenced as:
-
-MIT License
-
-Copyright (c) 2024 Raspberry Pi Ltd
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in all
-copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-SOFTWARE.

include/DAP_config.h

@@ -561,16 +561,7 @@ Status LEDs. In detail the operation of Hardware I/O and LED pins are enabled an
  - LED output pins are enabled and LEDs are turned off.
 */
 __STATIC_INLINE void DAP_SETUP (void) {
-// We synchronously setup probe IOs when the respective PIO program is loaded - not at start of day
-
-#ifdef PROBE_DAP_CONNECTED_LED
-    gpio_init(PROBE_DAP_CONNECTED_LED);
-    gpio_set_dir(PROBE_DAP_CONNECTED_LED, GPIO_OUT);
-#endif
-#ifdef PROBE_DAP_RUNNING_LED
-    gpio_init(PROBE_DAP_RUNNING_LED);
-    gpio_set_dir(PROBE_DAP_RUNNING_LED, GPIO_OUT);
-#endif
+  probe_gpio_init();
 }
 
 /** Reset Target Device with custom specific I/O pin or command sequence.

pico_sdk_import.cmake

@@ -1,4 +1,3 @@
-# SPDX-License-Identifier: BSD-3-Clause
 # This is a copy of <PICO_SDK_PATH>/external/pico_sdk_import.cmake
 
 # This can be dropped into an external project to help locate this SDK

src/cdc_uart.c

@@ -57,17 +57,6 @@ void cdc_uart_init(void) {
     gpio_set_pulls(PROBE_UART_RX, 1, 0);
     uart_init(PROBE_UART_INTERFACE, PROBE_UART_BAUDRATE);
 
-#ifdef PROBE_UART_TX_LED
-    tx_led_debounce = 0;
-    gpio_init(PROBE_UART_TX_LED);
-    gpio_set_dir(PROBE_UART_TX_LED, GPIO_OUT);
-#endif
-#ifdef PROBE_UART_RX_LED
-    rx_led_debounce = 0;
-    gpio_init(PROBE_UART_RX_LED);
-    gpio_set_dir(PROBE_UART_RX_LED, GPIO_OUT);
-#endif
-
 #ifdef PROBE_UART_HWFC
     /* HWFC implies that hardware flow control is implemented and the
      * UART operates in "full-duplex" mode (See USB CDC PSTN120 6.3.12).

src/led.c

@@ -0,0 +1,52 @@
+/*
+ * The MIT License (MIT)
+ *
+ * Copyright (c) 2021 a-pushkin on GitHub
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+
+#include <pico/stdlib.h>
+#include <stdint.h>
+
+#include "probe_config.h"
+
+void led_init(void) {
+#ifdef PROBE_USB_CONNECTED_LED
+    gpio_init(PROBE_USB_CONNECTED_LED);
+    gpio_set_dir(PROBE_USB_CONNECTED_LED, GPIO_OUT);
+#endif
+#ifdef PROBE_DAP_CONNECTED_LED
+    gpio_init(PROBE_DAP_CONNECTED_LED);
+    gpio_set_dir(PROBE_DAP_CONNECTED_LED, GPIO_OUT);
+#endif
+#ifdef PROBE_DAP_RUNNING_LED
+    gpio_init(PROBE_DAP_RUNNING_LED);
+    gpio_set_dir(PROBE_DAP_RUNNING_LED, GPIO_OUT);
+#endif
+#ifdef PROBE_UART_RX_LED
+    gpio_init(PROBE_UART_RX_LED);
+    gpio_set_dir(PROBE_UART_RX_LED, GPIO_OUT);
+#endif
+#ifdef PROBE_UART_TX_LED
+    gpio_init(PROBE_UART_TX_LED);
+    gpio_set_dir(PROBE_UART_TX_LED, GPIO_OUT);
+#endif
+}

src/led.h

@@ -0,0 +1,31 @@
+/*
+ * The MIT License (MIT)
+ *
+ * Copyright (c) 2021 a-pushkin on GitHub
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+
+#ifndef LED_H
+#define LED_H
+
+void led_init(void);
+
+#endif

src/main.c

@@ -42,6 +42,7 @@
 #include "probe.h"
 #include "cdc_uart.h"
 #include "get_serial.h"
+#include "led.h"
 #include "tusb_edpt_handler.h"
 #include "DAP.h"
 #include "hardware/structs/usb.h"
@@ -60,8 +61,6 @@ static uint8_t RxDataBuffer[CFG_TUD_HID_EP_BUFSIZE];
 
 TaskHandle_t dap_taskhandle, tud_taskhandle, mon_taskhandle;
 
-static int was_configured;
-
 void dev_mon(void *ptr)
 {
     uint32_t sof[3];
@@ -93,10 +92,6 @@ void dev_mon(void *ptr)
 
 void usb_thread(void *ptr)
 {
-#ifdef PROBE_USB_CONNECTED_LED
-    gpio_init(PROBE_USB_CONNECTED_LED);
-    gpio_set_dir(PROBE_USB_CONNECTED_LED, GPIO_OUT);
-#endif
     TickType_t wake;
     wake = xTaskGetTickCount();
     do {
@@ -133,6 +128,8 @@ int main(void) {
 
     DAP_Setup();
 
+    led_init();
+
     probe_info("Welcome to debugprobe!\n");
 
     if (THREADED) {
@@ -226,21 +223,17 @@ bool tud_vendor_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_control_requ
 void tud_suspend_cb(bool remote_wakeup_en)
 {
   probe_info("Suspended\n");
-  /* Were we actually configured? If not, threads don't exist */
-  if (was_configured) {
-	  vTaskSuspend(uart_taskhandle);
-	  vTaskSuspend(dap_taskhandle);
-  }
+  /* Join DAP and UART threads? Or just suspend them, for transparency */
+  vTaskSuspend(uart_taskhandle);
+  vTaskSuspend(dap_taskhandle);
   /* slow down clk_sys for power saving ? */
 }
 
 void tud_resume_cb(void)
 {
   probe_info("Resumed\n");
-  if (was_configured) {
-	  vTaskResume(uart_taskhandle);
-	  vTaskResume(dap_taskhandle);
-  }
+  vTaskResume(uart_taskhandle);
+  vTaskResume(dap_taskhandle);
 }
 
 void tud_unmount_cb(void)
@@ -250,19 +243,15 @@ void tud_unmount_cb(void)
   vTaskSuspend(dap_taskhandle);
   vTaskDelete(uart_taskhandle);
   vTaskDelete(dap_taskhandle);
-  was_configured = 0;
 }
 
 void tud_mount_cb(void)
 {
   probe_info("Connected, Configured\n");
-  if (!was_configured) {
-    /* UART needs to preempt USB as if we don't, characters get lost */
-    xTaskCreate(cdc_thread, "UART", configMINIMAL_STACK_SIZE, NULL, UART_TASK_PRIO, &uart_taskhandle);
-    /* Lowest priority thread is debug - need to shuffle buffers before we can toggle swd... */
-    xTaskCreate(dap_thread, "DAP", configMINIMAL_STACK_SIZE, NULL, DAP_TASK_PRIO, &dap_taskhandle);
-    was_configured = 1;
-  }
+  /* UART needs to preempt USB as if we don't, characters get lost */
+  xTaskCreate(cdc_thread, "UART", configMINIMAL_STACK_SIZE, NULL, UART_TASK_PRIO, &uart_taskhandle);
+  /* Lowest priority thread is debug - need to shuffle buffers before we can toggle swd... */
+  xTaskCreate(dap_thread, "DAP", configMINIMAL_STACK_SIZE, NULL, DAP_TASK_PRIO, &dap_taskhandle);
 }
 
 void vApplicationTickHook (void)

src/probe.c

@@ -30,6 +30,7 @@
 #include <hardware/clocks.h>
 #include <hardware/gpio.h>
 
+#include "led.h"
 #include "probe_config.h"
 #include "probe.h"
 #include "tusb.h"
@@ -146,7 +147,6 @@ void probe_write_mode(void) {
 
 void probe_init() {
     if (!probe.initted) {
-        probe_gpio_init();
         uint offset = pio_add_program(pio0, &probe_program);
         probe.offset = offset;
 
@@ -172,7 +172,7 @@ void probe_deinit(void)
     pio_remove_program(pio0, &probe_program, probe.offset);
 
     probe_assert_reset(1);	// de-assert nRESET
-    probe_gpio_deinit();
+
     probe.initted = 0;
   }
 }

src/probe.pio

@@ -86,19 +86,6 @@ static inline void probe_gpio_init()
     gpio_pull_up(PROBE_PIN_SWDIO);
 }
 
-// DAP interface says all pins have to be High-Z when disabled
-static inline void probe_gpio_deinit()
-{
-#if defined(PROBE_PIN_RESET)
-	gpio_deinit(PROBE_PIN_RESET);
-	gpio_disable_pulls(PROBE_PIN_RESET);
-#endif
-	gpio_deinit(PROBE_PIN_SWCLK);
-	gpio_disable_pulls(PROBE_PIN_SWCLK);
-	gpio_deinit(PROBE_PIN_SWDIO);
-	gpio_disable_pulls(PROBE_PIN_SWDIO);
-}
-
 static inline void probe_sm_init(pio_sm_config* sm_config) {
 
     // Set SWCLK as a sideset pin

src/probe_oen.pio

@@ -1,6 +1,3 @@
-; SPDX-License-Identifier: MIT
-; Copyright (c) 2023 Raspberry Pi Ltd
-
 ; Output-enable active-low variant of the SWD probe
 
 ; This program is very similar to the one in probe.pio. The only difference is
@@ -63,21 +60,6 @@ static inline void probe_gpio_init()
     gpio_pull_up(PROBE_PIN_SWDIOEN);
 }
 
-// DAP interface says all pins have to be High-Z when disabled
-static inline void probe_gpio_deinit()
-{
-#if defined(PROBE_PIN_RESET)
-	gpio_deinit(PROBE_PIN_RESET);
-	gpio_disable_pulls(PROBE_PIN_RESET);
-#endif
-	gpio_deinit(PROBE_PIN_SWCLK);
-	gpio_disable_pulls(PROBE_PIN_SWCLK);
-// Note for SWDIOEN - make sure the driver output enable is removed before removing drive - leave pull-up enabled
-	gpio_deinit(PROBE_PIN_SWDIOEN);
-	gpio_deinit(PROBE_PIN_SWDIO);
-	gpio_disable_pulls(PROBE_PIN_SWDIO);
-}
-
 static inline void probe_sm_init(pio_sm_config* sm_config) {
 
     // Set SWDIOEN and SWCLK as sideset pins

src/usb_descriptors.c

@@ -48,7 +48,7 @@ tusb_desc_device_t const desc_device =
 
     .idVendor           = 0x2E8A, // Pi
     .idProduct          = 0x000c, // CMSIS-DAP Debug Probe
-    .bcdDevice          = 0x0220, // Version 02.20
+    .bcdDevice          = 0x0201, // Version 02.01
     .iManufacturer      = 0x01,
     .iProduct           = 0x02,
     .iSerialNumber      = 0x03,
@@ -244,4 +244,4 @@ TU_VERIFY_STATIC(sizeof(desc_ms_os_20) == MS_OS_20_DESC_LEN, "Incorrect size");
 uint8_t const * tud_descriptor_bos_cb(void)
 {
   return desc_bos;
-}
+}