Merge pull request #59 from rgrr/feature/systemview-ncm-debugging

Merge a lot of NCM debugging into systemview
rgrr · Jul 14, 2023 · 6914ff1 · 6914ff1
2 parents 8b2c69d + 6918f36
commit 6914ff1
Show file tree

Hide file tree

Showing 20 changed files with 1,839 additions and 252 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -31,7 +31,8 @@ option(OPT_PROBE_DEBUG_OUT     "Enable CDC for probe debug output"      ${DEFAUL
 option(OPT_SIGROK              "Enable sigrok"                          0)
 option(OPT_MSC                 "Enable Mass Storage Device"             1)
 option(OPT_MSC_RAM_UF2         "Enable file 'RAM.UF2' on Mass Storage"  1)
-set(OPT_NET                    "NCM" CACHE STRING "Enable lwIP on the Pico via ECM/NCM/RNDIS")
+set(OPT_MCU_OVERCLOCK_MHZ      "240" CACHE STRING "Set processor frequency. Should be a multiple of 24MHz, disable with empty string (=120MHz)")
+set(OPT_NET                    "NCM" CACHE STRING "Enable lwIP on the Pico via NCM/ECM/RNDIS, disable NET with empty string")
 set(OPT_NET_192_168            14 CACHE STRING "Set the subnet of 192.168.x")
 option(OPT_NET_ECHO_SERVER     "Enable echo server for testing"         1)
 option(OPT_NET_IPERF_SERVER    "Enable iperf server for tuning"         1)
@@ -95,6 +96,9 @@ add_definitions(
         -DTARGET_BOARD_${PICO_BOARD_UPPER}
         -DOPT_SPECIAL_CLK_FOR_PIO=${OPT_SPECIAL_CLK_FOR_PIO}
 )
+if(NOT OPT_MCU_OVERCLOCK_MHZ STREQUAL "")
+    add_compile_definitions(OPT_MCU_OVERCLOCK_MHZ=${OPT_MCU_OVERCLOCK_MHZ})
+endif()
 
 # set version string to "x.yy"
 # there are perhaps smarter ways...
@@ -257,7 +261,18 @@ if(NOT OPT_NET STREQUAL "")
         ${PICO_TINYUSB_PATH}/lib/networking/dhserver.c
     )
 
-    if(NOT OPT_NET STREQUAL "NCM")
+    if(OPT_NET STREQUAL "NCM")
+        # NCM: use own copy of ncm_device, original one is removed from target_sources() with a trick
+        message("--------- " ${PICO_TINYUSB_PATH})
+        target_sources(${PROJECT} PRIVATE
+            src/net/tinyusb/ncm_device_simple.c
+        )
+        set_source_files_properties(
+            ${PICO_TINYUSB_PATH}/src/class/net/ncm_device.c
+            PROPERTIES HEADER_FILE_ONLY ON
+        )
+    else()
+        # !NCM
         target_sources(${PROJECT} PRIVATE
             ${PICO_TINYUSB_PATH}/lib/networking/rndis_reports.c
         )

diff --git a/README.adoc b/README.adoc
@@ -99,7 +99,7 @@ containing the steps from installation until debugging in VSCode.
 |yes 
 |`openocd -f interface/cmsis-dap.cfg -f target/rp2040.cfg -c "adapter speed 25000"    -c "program {firmware.elf}  verify reset; shutdown;"`
 
-|pyOCD 0.34.x
+|pyOCD 0.34 & 0.35
 |yes
 |yes
 |`pyocd flash -f 400000 -t nrf52840 firmware.elf`
@@ -371,6 +371,7 @@ SUBSYSTEM=="usb", ATTR{idVendor}=="2e8a", ATTR{idProduct}=="000c", MODE:="0666"
 ACTION=="add", SUBSYSTEMS=="usb", KERNEL=="ttyACM[0-9]*", ATTRS{interface}=="YAPicoprobe CDC-UART",    MODE:="0666", SYMLINK+="ttyPicoTarget"
 ACTION=="add", SUBSYSTEMS=="usb", KERNEL=="ttyACM[0-9]*", ATTRS{interface}=="YAPicoprobe CDC-DEBUG",   MODE:="0666", SYMLINK+="ttyPicoProbe"
 ACTION=="add", SUBSYSTEMS=="usb", KERNEL=="ttyACM[0-9]*", ATTRS{interface}=="YAPicoprobe CDC-SIGROK",  MODE:="0666", SYMLINK+="ttyPicoSigRok
+ACTION=="add", SUBSYSTEMS=="usb", KERNEL=="ttyACM[0-9]*", ATTRS{interface}=="YAPicoprobe CDC-SysView", MODE:="0666", SYMLINK+="ttyPicoSysView"
 
 # mount Picoprobe to /media/picoprobe
 ACTION=="add", SUBSYSTEMS=="usb", SUBSYSTEM=="block", ENV{ID_FS_USAGE}=="filesystem", ATTRS{idVendor}=="2e8a", ATTRS{idProduct}=="000c", RUN+="/usr/bin/logger --tag picoprobe-mount Mounting what seems to be a Raspberry Pi Picoprobe", RUN+="/usr/bin/systemd-mount --no-block --collect --fsck=0 -o uid=hardy,gid=hardy,flush $devnode /media/picoprobe"
@@ -383,8 +384,11 @@ ACTION=="remove", SUBSYSTEMS=="usb", SUBSYSTEM=="block", ENV{ID_FS_USAGE}=="file
 
 ### Network Configuration
 
-SystemView connectivity over TCP/IP is on most systems not configuration free.  On Debian the configuration
-is as follows:
+SystemView connectivity over TCP/IP is on most systems not configuration free.  
+
+#### Debian
+
+On Debian the configuration is as follows:
 
 * after connecting the probe with the host, `ip a` shows its network interface, e.g. `enxfe<your-probe>`.
   If the network interface already shows an IP address everything is fine and you are ready
@@ -402,6 +406,27 @@ On my system this unfortunately leads to error messages (which are harmless) if
 The default address of the probe is 192.168.14.1.  If there is a collision with the current setup of the
 host, the probe firmware has to be rebuilt with `-DOPT_NET_192_168=<subnet>`.
 
+#### Win10
+
+A certain version of Win10 is required for USB-NCM connectivity.  Exact version is unknown.
+
+The driver needs to be installed manually.  Procedure is roughly as follows:
+
+* in "Device Manager" search for `YaPicoprobe NCM` with an exclamation mark
+* "Update Driver"
+* "Browse my computer for drivers"
+* "Let me pick..."
+* "Network adapters"
+* "Microsoft"
+* "UsbNcm Host Device" - if this is not available, then the version of Win10 is not ready for USB-NCM
+* confirm any dialog boxes
+* if `ipconfig` on a command line shows `192.168.14.2` for an Ethernet adapter, the procedure has most likely
+  succeeded and SystemView is ready for operation
+
+#### Win11
+
+It is said, that USB-NCM is working out of the box.  This is not tested.
+
 
 ### PlatformIO [[platformio]]
 https://platformio.org/[PlatformIO] configuration in `platformio.ini` is pretty straight forward:

diff --git a/doc/lwIP-notes.adoc b/doc/lwIP-notes.adoc
@@ -9,8 +9,9 @@
 ## Some lwIP Notes
 
 lwIP is used for creation of a network interface of YAPicoprobe to avoid
-an uncountable number of additional CDC com ports. +
-Currently just a Segger SysView server is listening on port 19111.
+an uncountable number of additional CDC COM ports.
+
+Currently a Segger SysView server is listening on port 19111.
 But I guess there is more to come.
 
 
@@ -64,19 +65,7 @@ messages even when the device has `allow-hotplug`.
   Unfortunately RNDIS seems to manipulate routing in a way that the
   default route on my Linux wants to go through the probe.  Not
   really what I want...
-  
-* *NCM*: is said to be the best choice.  I did not manage to get an
-  acceptable throughput.  Transfer seems to get stuck from time to time
-  and SystemView aborts.  So also not the favorite. +
-  `iperf` is running fast, it seems that `ncm_device.c` output is triggered
-  by incoming packets.
 
-* *ECM*: works good, packets are transferred continuously, throughput
-  also seems to be ok.  So this is the way to go. +
-  Unfortunately there is no driver integrated into Win10, so possible 
-  extra trouble appears.  Yes... extra trouble: cannot find a driver
-  for Win10.
-     
 [NOTE]
 ====
 RNDIS on Win10 works only, if RNDIS on the probe is the only USB class selected.
@@ -85,6 +74,20 @@ like SystemView, but you cannot have a probe which does SystemView and CMSIS-DAP
 This is not a fault of lwIP, it is a bug in the Win10 driver(?).
 ====
 
+* *ECM*: works good, packets are transferred continuously, throughput
+  also seems to be ok.  So this is the way to go. +
+  Unfortunately there is no driver integrated into Win10, so possible 
+  extra trouble appears.  Yes... extra trouble: cannot find a driver
+  for Win10.
+     
+* *NCM*: is said to be the best choice.  And in fact it is.
+  At least after creation of a `ncm_device_simple.c` driver which is a
+  stripped down version of `ncm_simple.c` which revealed as very buggy. +
+  Now thoughput under Linux and Windows is ok.  Operation with SystemView
+  works without glitches, `iperf` tests sometimes crashes the probe.
+  So consider this driver as beta and work in progress.
+
+
 
 ## Performance
 
@@ -93,9 +96,72 @@ must be set on built).  Good command line for measurement:
 
   iperf -c 192.168.14.1 -e -i 1 -l 1024
 
+## Testing
+
+Good test cases are the following command lines:
+
+  for MSS in 90 100 200 300 400 500 600 700 800 900 1000 1100 1200 1300 1400 1459 1460 1500; do iperf -c 192.168.14.1 -e -i 1 -l 1024 -M $MSS; sleep 10; done
+
+  for LEN in 40000 16000 1400 1024 800 200 80 22 8 2 1; do for MSS in 90 93 100 150 200 255 256 300 400 500 511 512 600 700 800 900 1000 1100 1200 1300 1400 1450 1459 1460 1500; do iperf -c 192.168.14.1 -e -i 1 -l $LEN -M $MSS; sleep 2; done; done
+
+Monitor performance/errors with Wireshark.
+
+
+## Some words about...
+
+### ... `net_glue.c`
+
+I'm really trying hard to switch context betwenn lwIP and TinyUSB correctly.  This leads
+to some kind of delayed call chains and also does not make the code neither nice nor
+very much maintainable.
+
+
+### ... `ncm_device_simple.c`
+
+`ncm_device_simple.c` is actually a mixture of `ecm_rndis_device.c` and `ncm_device.c`.
+From `ecm_rndis_device.c` the structure has been inherited and from `ncm_device.c` the 
+functionality. +
+The driver can be considered work in progress, because in conjunction with `iperf`
+crashes sometimes happen.  But for operation with SystemView quality seems to be good enough.
+
+
+### ... possible Bugs in `ncm_device.c`
+
+This is more or less obsoleted by `ncm_device_simple.c`.  But as a short summary: the original
+driver is very buggy.  Perhaps it is working in certain scenarios, but for sure not together with
+SystemView.
+
+* not sure, but perhaps it is best to call all functions within ncm_device in the FreeRTOS
+  context of TinyUSB
+* `wNtbOutMaxDatagrams` must be set to 1 [2023-06-27]
+** iperf runs then
+** Systemview still has problems
+** `wNtbOutMaxDatagrams == 0` generates a lot of retries with iperf
+* I guess that the *major problem* lies within handle_incoming_datagram() because it changes values
+  on an incoming packet although tud_network_recv_renew() is still handling the old one
+* is there multicore a problem!? (14.7.2023: no!)  I have seen retries with multicore even with 
+  `wNtbOutMaxDatagrams = 1`
+* I think it is assumed, that TinyUSB and lwIP are running in the same task (but in my scenario they don't)
+* if removing debug messages, then the receive path seems to work better, which
+  indicates a race condition somewhere
+
+There is an open issue in the TinyUSB repo for this issue: https://github.com/hathach/tinyusb/issues/2068
+
 
 ## Log
 
+### 2023-07-14
+
+* did some performance tuning with lwIP and TinyUSB
+* stripped sources
+* BUG: `ncm_device_simple` sometimes crashes with `iperf`
+
+### 2023-06-30
+
+* for debugging purposes reimplemented `ncm_device_simple.c` which can hold only
+  one ethernet frame per NTB (NCM Transfer Block).  This unfortunately requires
+  that the original `ncm_device.c` must be outcommented via `#if` on top.
+
 ### 2023-06-26
 
 * after some changes to `rtt_console.c`, `net_sysview.c` and `net_glue.c`

diff --git a/include/FreeRTOSConfig.h b/include/FreeRTOSConfig.h
@@ -78,7 +78,7 @@
 /* Memory allocation related definitions. */
 #define configSUPPORT_STATIC_ALLOCATION         1
 #define configSUPPORT_DYNAMIC_ALLOCATION        1
-#define configTOTAL_HEAP_SIZE                   (150*1024)
+#define configTOTAL_HEAP_SIZE                   (90*1024)
 #define configAPPLICATION_ALLOCATED_HEAP        0
 
 /* Hook function related definitions. */
@@ -97,6 +97,9 @@
     #warning "configGENERATE_RUN_TIME_STATS is set"
     #define portCONFIGURE_TIMER_FOR_RUN_TIME_STATS()        do {} while( 0 )
     #define portALT_GET_RUN_TIME_COUNTER_VALUE( dest )      ( dest = *((uint32_t *)(TF_TIMER_BASE + TF_TIMER_TIMERAWL_OFFSET)) )
+
+    #undef configUSE_TRACE_FACILITY
+    #define configUSE_TRACE_FACILITY            1
 #endif
 
 /* Co-routine related definitions. */

diff --git a/include/lwipopts.h b/include/lwipopts.h
@@ -65,41 +65,49 @@
 
 
 //--------------------------------------
-// performance tuning (do not change without extensive testing, optimized for ECM)
-#define TCP_MSS                         (1500 - 20 - 20)          // MTU minus header sizes (best value til now)
-#define TCP_SND_BUF                     (4 * TCP_MSS)             //   good tuning
+// performance tuning (do not change without extensive testing, optimized for ECM/NCM)
+#define TCP_MSS                                (1500 - 20 - 20)    // MTU minus header sizes (best value til now)
+#define TCP_SND_BUF                            (8 * TCP_MSS)       //   good tuning
 
-//#define TCP_WND                         TCP_SND_BUF
-//#define TCP_OVERSIZE                    (TCP_MSS / 4)             // til now no good value found
+//#define TCP_WND                                TCP_SND_BUF         // til now no good value found
+#define TCP_SND_QUEUELEN                       16
+#define TCP_SNDQUEUELOWAT                      (TCP_SND_QUEUELEN / 2)
+#define MEMP_NUM_TCP_SEG                       32
 
 //--------------------------------------
 // memory
-#define MEM_SIZE                        20000
-//#define MEM_LIBC_MALLOC                 1
-//#define MEMP_MEM_MALLOC                 1
+#define MEM_SIZE                               20000
+//#define MEMP_OVERFLOW_CHECK                    1
 //#define LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT 1
 
 
 //--------------------------------------
 // for freertos mode
-#define TCPIP_MBOX_SIZE                 64
-#define TCPIP_THREAD_STACKSIZE          8192
-#define TCPIP_THREAD_PRIO               11
+#define TCPIP_MBOX_SIZE                        64
+#define TCPIP_THREAD_STACKSIZE                 8192
+#define TCPIP_THREAD_PRIO                      27
 
 
 //--------------------------------------
 // trying...
 #define LWIP_PROVIDE_ERRNO              1
 #if LWIP_SOCKET
-    #define LWIP_TIMEVAL_PRIVATE        0      // required for LWIP_SOCKET
+    #define LWIP_TIMEVAL_PRIVATE        0                          // required for LWIP_SOCKET
 #endif
 
 
 //--------------------------------------
 // statistics
-//#define LWIP_STATS                      1
-//#define LWIP_STATS_DISPLAY              1
-//#define LINK_STATS                      1
+// use stats_display() for display
+#define LWIP_STATS                      0
+#define LWIP_STATS_DISPLAY              0
+#define ETHARP_STATS                    0                          // do not display the topics below
+#define ICMP_STATS                      0
+#define IPFRAG_STATS                    0
+#define LINK_STATS                      0
+#define MEMP_STATS                      0
+#define SYS_STATS                       0
+#define UDP_STATS                       0
 
 
 //--------------------------------------
@@ -108,7 +116,7 @@
 #define API_LIB_DEBUG                   LWIP_DBG_OFF
 #define API_MSG_DEBUG                   LWIP_DBG_OFF
 #define AUTOIP_DEBUG                    LWIP_DBG_OFF
-#define DHCP_DEBUG                      LWIP_DBG_ON
+#define DHCP_DEBUG                      LWIP_DBG_OFF
 #define DNS_DEBUG                       LWIP_DBG_OFF
 #define ETHARP_DEBUG                    LWIP_DBG_OFF
 #define ICMP_DEBUG                      LWIP_DBG_OFF
@@ -125,7 +133,7 @@
 #define TCP_INPUT_DEBUG                 LWIP_DBG_OFF
 #define TCP_FR_DEBUG                    LWIP_DBG_OFF
 #define TCP_RTO_DEBUG                   LWIP_DBG_OFF
-#define TCP_CWND_DEBUG                  LWIP_DBG_OFF
+#define TCP_CWND_DEBUG                  LWIP_DBG_ON
 #define TCP_WND_DEBUG                   LWIP_DBG_OFF
 #define TCP_RST_DEBUG                   LWIP_DBG_OFF
 #define TCP_OUTPUT_DEBUG                LWIP_DBG_OFF

diff --git a/include/picoprobe_config.h b/include/picoprobe_config.h
@@ -63,10 +63,10 @@
 
 // Base value of sys_clk in khz.  Must be <=125Mhz per RP2040 spec and a multiple of 24Mhz
 // to support integer divisors of the PIO clock and ADC clock (for sigrok)
-#if 0
-#define PROBE_CPU_CLOCK_KHZ      ((120 + 2*24) * 1000)             // overclocked, even 264MHz seems to be no problem
+#ifdef OPT_MCU_OVERCLOCK_MHZ
+    #define PROBE_CPU_CLOCK_KHZ      ((OPT_MCU_OVERCLOCK_MHZ) * 1000)     // overclocked, even 264MHz seems to be no problem
 #else
-#define PROBE_CPU_CLOCK_KHZ      ((120 + 0*24) * 1000)             // overclocked, even 264MHz seems to be no problem
+    #define PROBE_CPU_CLOCK_KHZ      ((120 + 0*24) * 1000)
 #endif
 
 

diff --git a/src/cdc/cdc_debug.c b/src/cdc/cdc_debug.c
@@ -79,7 +79,7 @@ void cdc_debug_thread(void *ptr)
         if (xStreamBufferIsEmpty(stream_printf)) {
             // -> end of transmission: flush and sleep for a long time (or until new data is available)
             tud_cdc_n_write_flush(CDC_DEBUG_N);
-            xEventGroupWaitBits(events, EV_TX_COMPLETE | EV_STREAM, pdTRUE, pdFALSE, pdMS_TO_TICKS(10000));
+            xEventGroupWaitBits(events, EV_TX_COMPLETE | EV_STREAM, pdTRUE, pdFALSE, pdMS_TO_TICKS(1000));
         }
         else {
             size_t cnt;
@@ -255,7 +255,7 @@ void cdc_debug_init(uint32_t task_prio)
         panic("cdc_debug_init: cannot create sema_printf\n");
     }
 
-    xTaskCreate(cdc_debug_thread, "CDC_DEBUG", configMINIMAL_STACK_SIZE, NULL, task_prio, &task_printf);
+    xTaskCreate(cdc_debug_thread, "CDC-ProbeUart", configMINIMAL_STACK_SIZE, NULL, task_prio, &task_printf);
     cdc_debug_line_state_cb(false, false);
 
     stdio_set_driver_enabled(&stdio_cdc, true);

diff --git a/src/cdc/cdc_sysview.c b/src/cdc/cdc_sysview.c
@@ -144,7 +144,7 @@ void cdc_sysview_line_state_cb(bool dtr, bool rts)
  * This seems to be necessary to survive e.g. a restart of the host (Linux)
  */
 {
-    printf("cdc_sysview_line_state_cb(%d,%d)\n", dtr, rts);
+    //printf("cdc_sysview_line_state_cb(%d,%d)\n", dtr, rts);
 
     tud_cdc_n_write_clear(CDC_SYSVIEW_N);
     tud_cdc_n_read_flush(CDC_SYSVIEW_N);
@@ -171,6 +171,13 @@ void cdc_sysview_rx_cb()
 
 
 
+bool net_sysview_is_connected(void)
+{
+    return m_connected;
+}   // net_sysview_is_connected
+
+
+
 uint32_t net_sysview_send(const uint8_t *buf, uint32_t cnt)
 /**
  * Send characters from SysView RTT channel into stream.
@@ -188,7 +195,7 @@ uint32_t net_sysview_send(const uint8_t *buf, uint32_t cnt)
         r = xStreamBufferSpacesAvailable(stream_sysview);
     }
     else {
-        if ( !m_connected) {
+        if ( !net_sysview_is_connected()) {
             xStreamBufferReset(stream_sysview);
         }
         else {
@@ -212,6 +219,6 @@ void cdc_sysview_init(uint32_t task_prio)
         picoprobe_error("cdc_sysview_init: cannot create stream_sysview\n");
     }
 
-    xTaskCreate(cdc_thread, "CDC_SysView", configMINIMAL_STACK_SIZE, NULL, task_prio, &task_sysview);
+    xTaskCreate(cdc_thread, "CDC-SysViewUart", configMINIMAL_STACK_SIZE, NULL, task_prio, &task_sysview);
     cdc_sysview_line_state_cb(false, false);
 }   // cdc_sysview_init