Technical debt -- and one SPI bus fix

hacks/rtt_debugging.md

@@ -78,14 +78,18 @@ rtt start
 <details><summary>As a single script...</summary><P/>
 
 ```
+reset halt
+rtt stop
 program ./build_rp2040/src/bus_pirate5_rev10.elf
 reset halt
 rp2040.core1 arp_reset assert 0
 rp2040.core0 arp_reset assert 0
 sleep 500
 rtt setup 0x20000000 0x100000 "SEGGER RTT"
 rtt start
+
 rtt server start 4321 0
+
 ```
 
 </details>
@@ -97,14 +101,18 @@ This requires a pre-release version of OpenOCD 0.12.0
 <details><summary>As a single script...</summary><P/>
 
 ```
+reset halt
+rtt stop
 program /home/henrygab/build_rp2350/src/bus_pirate6.elf
 reset halt
 rp2350.dap.core1 arp_reset assert 0
 rp2350.dap.core0 arp_reset assert 0
 sleep 500
 rtt setup 0x20000000 0x100000 "SEGGER RTT"
 rtt start
+
 rtt server start 4321 0
+
 ```
 
 </details>

src/CMakeLists.txt

@@ -370,20 +370,6 @@ set (bp5_rtt
 
 
 if(${PICO_PLATFORM} MATCHES "rp2350")
-        add_executable(bus_pirate5_xl
-                ${bp5_common}
-                ${bp5_nandflash}
-                ${bp5_rtt}
-                platform/bpi-rev10.h
-                platform/bpi-rev10.c 
-                pirate/shift.h
-                pirate/shift.c
-                display/robot5xx16.h
-                )
-        target_compile_definitions(bus_pirate5_xl PUBLIC BP_VER=XL5)
-        target_compile_definitions(bus_pirate5_xl PUBLIC BP_REV=10)
-        target_compile_definitions(bus_pirate5_xl PUBLIC BP_FIRMWARE_HASH="${BP_FIRMWARE_HASH}")
-
         add_executable(bus_pirate6
                 ${bp5_common}
                 ${bp5_nandflash}
@@ -401,23 +387,20 @@ if(${PICO_PLATFORM} MATCHES "rp2350")
                 bus_pirate6
         )        
 
-else()
-        add_executable(bus_pirate5_rev8
+        add_executable(bus_pirate5_xl
                 ${bp5_common}
+                ${bp5_nandflash}
                 ${bp5_rtt}
-                platform/bpi-rev8.h
-                platform/bpi-rev8.c
-                #tf card access control
-                fatfs/tf_card.c
-                fatfs/tf_card.h
+                platform/bpi-rev10.h
+                platform/bpi-rev10.c 
                 pirate/shift.h
                 pirate/shift.c
-                display/robot5x16.h
+                display/robot5xx16.h
                 )
-        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_VER=5) 
-        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_REV=8)
-        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_FIRMWARE_HASH="${BP_FIRMWARE_HASH}")
-
+        target_compile_definitions(bus_pirate5_xl PUBLIC BP_VER=XL5)
+        target_compile_definitions(bus_pirate5_xl PUBLIC BP_REV=10)
+        target_compile_definitions(bus_pirate5_xl PUBLIC BP_FIRMWARE_HASH="${BP_FIRMWARE_HASH}")
+else()
         add_executable(bus_pirate5_rev10
                 ${bp5_common}
                 ${bp5_nandflash}
@@ -436,6 +419,22 @@ else()
                 bus_pirate5_rev8  
                 bus_pirate5_rev10
         )
+
+        add_executable(bus_pirate5_rev8
+                ${bp5_common}
+                ${bp5_rtt}
+                platform/bpi-rev8.h
+                platform/bpi-rev8.c
+                #tf card access control
+                fatfs/tf_card.c
+                fatfs/tf_card.h
+                pirate/shift.h
+                pirate/shift.c
+                display/robot5x16.h
+                )
+        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_VER=5) 
+        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_REV=8)
+        target_compile_definitions(bus_pirate5_rev8 PUBLIC BP_FIRMWARE_HASH="${BP_FIRMWARE_HASH}")
 endif()
 
 

src/debug_rtt.c

@@ -1,6 +1,58 @@
 
 #include "debug_rtt.h"
 
+bool _SYSTEM_INITIALIZATION_COMPLETED = false;
+bool _SYSTEM_INITIALIZATION_COMPLETED_BY_CORE[2] = { false, false };
+
+void bp_mark_system_initialized(void) {
+    unsigned int core = get_core_num();
+    if (core == 0) {
+        _SYSTEM_INITIALIZATION_COMPLETED_BY_CORE[0] = true;
+        if (_SYSTEM_INITIALIZATION_COMPLETED_BY_CORE[1]) {
+            _SYSTEM_INITIALIZATION_COMPLETED = true;
+        }
+    } else if (core == 1) {
+        _SYSTEM_INITIALIZATION_COMPLETED_BY_CORE[1] = true;
+        if (_SYSTEM_INITIALIZATION_COMPLETED_BY_CORE[0]) {
+            _SYSTEM_INITIALIZATION_COMPLETED = true;
+        }
+    }
+}
+bool bp_is_system_initialized(void) {
+    return _SYSTEM_INITIALIZATION_COMPLETED;
+}
+
+// PICO SDK defines the following (weak) functions for handling assert() / hard_assert() failures:
+//
+//     void __assert_func(const char *file, int line, const char *func, const char *failedexpr);
+//     void hard_assertion_failure(void);
+//
+// Override these functions with our own versions so that information is also stored in RTT
+// and via terminal output.
+
+void __assert_func(const char *file, int line, const char *func, const char *failedexpr) {
+    BP_DEBUG_PRINT(BP_DEBUG_LEVEL_FATAL, BP_DEBUG_CAT_CATCHALL,
+        "assertion \"%s\" failed: file \"%s\", line %d%s%s\n",
+        failedexpr, file, line, func ? ", function: " : "",
+        func ? func : ""
+        );
+    printf(
+        "assertion \"%s\" failed: file \"%s\", line %d%s%s\n",
+        failedexpr, file, line, func ? ", function: " : "",
+        func ? func : ""
+        );
+    exit(1);
+}
+void hard_assertion_failure(void) {
+    BP_DEBUG_PRINT(BP_DEBUG_LEVEL_FATAL, BP_DEBUG_CAT_CATCHALL,
+        "hard_assert() failed\n"
+        );
+    panic("Hard assert");
+}
+
+
+
+
 
 // In C, there does not appear to be a way to use the
 // typesafe versions of the enum values.  :(
@@ -27,6 +79,6 @@ bp_debug_level_t _DEBUG_LEVELS[32] = {
     [E_DEBUG_CAT_ONBOARD_PIXELS ] = BP_DEBUG_LEVEL_VERBOSE,
     [E_DEBUG_CAT_ONBOARD_STORAGE] = BP_DEBUG_LEVEL_VERBOSE,
     // add others here, in order of enumeration value
-    [E_DEBUG_CAT_TEMP           ] = BP_DEBUG_LEVEL_NEVER, // Print EVERYTHING in TEMP category by default
+    [E_DEBUG_CAT_TEMP           ] = BP_DEBUG_LEVEL_DEBUG,
 }; // up to 32 categories, each with a debug level
 

src/debug_rtt.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "pirate.h"
+#include "pico.h" // needed for at least `get_core_num()`
 #include "lib/rtt/RTT/SEGGER_RTT.h"
 
 // To add a new "category" of debug messages which can be enabled/disabled
@@ -193,3 +194,45 @@ static inline bool bp_debug_should_print(bp_debug_level_t level, bp_debug_catego
 #define PRINT_DEBUG(...)   BP_DEBUG_PRINT(BP_DEBUG_LEVEL_DEBUG,   BP_DEBUG_DEFAULT_CATEGORY, __VA_ARGS__)
 
 #endif
+
+// Some assertions are only valid AFTER the system is fully initialized.
+// For example, anything that typically must run on core1 ... during
+// initialization, allow it to run on core0.  Once both cores are in
+// their infinite loops, then the system is fully initialized.
+void bp_mark_system_initialized(void);
+bool bp_is_system_initialized(void);
+
+#ifdef NDEBUG
+
+// These assertions only assert after the system is fully initialized.
+#define BP_ASSERT_CORE0() \
+    do {                                                                  \
+        if ((get_core_num() != 0) && bp_is_system_initialized()) {        \
+            PRINT_FATAL("ASSERTION FAILED: %s:%d\n", __FILE__, __LINE__); \
+            hard_assert(false);                                           \
+        }                                                                 \
+    } while (0)
+#define BP_ASSERT_CORE1() \
+    do {                                                                  \
+        if ((get_core_num() != 1) && bp_is_system_initialized()) {        \
+            PRINT_FATAL("ASSERTION FAILED: %s:%d\n", __FILE__, __LINE__); \
+            hard_assert(false);                                           \
+        }                                                                 \
+    } while (0)
+#define BP_ASSERT(_COND) \
+    do {                                                                  \
+        if (!(_COND)) {                                                   \
+            PRINT_FATAL("ASSERTION FAILED: %s:%d\n", __FILE__, __LINE__); \
+            hard_assert(false);                                           \
+        }                                                                 \
+    } while (0)
+
+#else
+
+// These assertions only assert after the system is fully initialized.
+#define BP_ASSERT_CORE0() assert((get_core_num() == 0) || !bp_is_system_initialized())
+#define BP_ASSERT_CORE1() assert((get_core_num() == 1) || !bp_is_system_initialized())
+#define BP_ASSERT(_COND)  assert(_COND)
+#endif
+
+

src/nand/spi_nand.c

@@ -180,8 +180,13 @@ typedef union {
 
 // private function prototypes
 static void csel_setup(void);
-static void csel_deselect(void);
-static void csel_select(void);
+//static void csel_deselect(void);
+// static void csel_select(void);
+#define csel_select()   csel_select_internal(__FILE__, __LINE__)
+#define csel_deselect() csel_deselect_internal(__FILE__, __LINE__)
+static void csel_select_internal(const char* file, int line);
+static void csel_deselect_internal(const char* file, int line);
+
 
 static int spi_nand_reset(void);
 static int read_id(nand_identity_t* identity_out);
@@ -216,7 +221,6 @@ int spi_nand_init(struct dhara_nand* dhara_parameters_out) {
     memset(dhara_parameters_out, 0, sizeof(struct dhara_nand));
 
     // initialize chip select
-    csel_deselect();
     csel_setup();
 
     // reset
@@ -497,15 +501,15 @@ static void csel_setup(void) {
     // gpio_set_dir(FLASH_STORAGE_CS, GPIO_OUT);
 }
 
-static void csel_deselect(void) {
+static void csel_deselect_internal(const char* file, int line) {
     // LL_GPIO_SetOutputPin(CSEL_PORT, CSEL_PIN);
     gpio_put(FLASH_STORAGE_CS, 1);
-    spi_busy_wait(false);
+    spi_busy_wait_internal(false, file, line);
 }
 
-static void csel_select(void) {
+static void csel_select_internal(const char* file, int line) {
     // LL_GPIO_ResetOutputPin(CSEL_PORT, CSEL_PIN);
-    spi_busy_wait(true);
+    spi_busy_wait_internal(true, file, line);
     gpio_put(FLASH_STORAGE_CS, 0);
 }
 

src/pirate.c

@@ -431,6 +431,11 @@ static void main_system_initialization(void) {
         "Init: binmode_setup()\n"
         );
     binmode_setup();
+
+    BP_DEBUG_PRINT(BP_DEBUG_LEVEL_VERBOSE, BP_DEBUG_CAT_EARLY_BOOT,
+        "Init: main_system_initialization() complete()\n"
+        );
+
 }
 
 static void core0_infinite_loop(void) {
@@ -639,6 +644,7 @@ void main(void) {
         "Init: entering core0_infinite_loop()\n"
         );
 
+    bp_mark_system_initialized();
     core0_infinite_loop(); // this never should exit, but....
     assert(false); // infinite loop above should never exit
 }
@@ -717,6 +723,8 @@ static void core1_infinite_loop(void) {
         if (system_config.binmode_usb_tx_queue_enable) {
             bin_tx_fifo_service();
         }
+        // also receive input from RTT, if available
+        rx_from_rtt_terminal();
 
         if (system_config.psu == 1 &&
             system_config.psu_irq_en == true &&
@@ -813,6 +821,7 @@ void core1_entry(void) {
     BP_DEBUG_PRINT(BP_DEBUG_LEVEL_VERBOSE, BP_DEBUG_CAT_EARLY_BOOT,
         "Init: starting core1_infinite_loop()\n"
         );
+    bp_mark_system_initialized();
     core1_infinite_loop();
     assert(false); // infinite loop above should never exit
 }
@@ -838,13 +847,16 @@ void lcd_irq_enable(int16_t repeat_interval) {
 }
 
 // gives protected access to spi (core safe)
-void spi_busy_wait(bool enable) {
+void spi_busy_wait_internal(bool enable, const char *file, int line) {
+
     if (!enable) {
-        // the check is to protect against the first csel_deselect call not matched by a csel_select
-        if (lock_is_owner_id_valid(spi_mutex.owner)) {
-            mutex_exit(&spi_mutex);
-        }
+
+        BP_ASSERT(lock_get_caller_owner_id() == spi_mutex.owner);
+        mutex_exit(&spi_mutex);
+
     } else {
+
         mutex_enter_blocking(&spi_mutex);
+        BP_ASSERT(lock_get_caller_owner_id() == spi_mutex.owner);
     }
 }

src/pirate.h

@@ -90,7 +90,9 @@
 
 void lcd_irq_enable(int16_t repeat_interval);
 void lcd_irq_disable(void);
-void spi_busy_wait(bool enable);
+
+#define spi_busy_wait(ENABLE) spi_busy_wait_internal(ENABLE, __FILE__, __LINE__)
+void spi_busy_wait_internal(bool enable, const char *file, int line);
 
 //#define BP_PIO_SHOW_ASSIGNMENT
 

src/ui/ui_flags.h

@@ -1,10 +1,26 @@
 
-#define UI_UPDATE_IMAGE (1u << 0)
-#define UI_UPDATE_INFOBAR (1u << 1)
-#define UI_UPDATE_NAMES (1u << 2)
-#define UI_UPDATE_LABELS (1u << 3)
-#define UI_UPDATE_VOLTAGES (1u << 4)
-#define UI_UPDATE_CURRENT (1u << 5)
-#define UI_UPDATE_FORCE (1u << 6) // force label update
-#define UI_UPDATE_ALL                                                                                                  \
-    (UI_UPDATE_IMAGE | UI_UPDATE_INFOBAR | UI_UPDATE_NAMES | UI_UPDATE_LABELS | UI_UPDATE_VOLTAGES | UI_UPDATE_CURRENT)
+typedef enum _ui_update_flag_t {
+    UI_UPDATE_NONE     = 0u,
+    UI_UPDATE_IMAGE    = (1u << 0),
+    UI_UPDATE_INFOBAR  = (1u << 1),
+    UI_UPDATE_NAMES    = (1u << 2),
+    UI_UPDATE_LABELS   = (1u << 3),
+    UI_UPDATE_VOLTAGES = (1u << 4),
+    UI_UPDATE_CURRENT  = (1u << 5),
+    UI_UPDATE_FORCE    = (1u << 6), // force label update
+    // space for one more UI update flag before this changes from uint8_t to uint16_t....
+
+    UI_UPDATE_ALL      = (
+        UI_UPDATE_IMAGE    |
+        UI_UPDATE_INFOBAR  |
+        UI_UPDATE_NAMES    |
+        UI_UPDATE_LABELS   |
+        UI_UPDATE_VOLTAGES |
+        UI_UPDATE_CURRENT  |
+        0u
+        ),
+} ui_update_flag_t;
+
+// if size grew, need to check all uses of this enum to ensure larger size is OK
+// Likely yes, but ... safer to check.
+_Static_assert(sizeof(ui_update_flag_t) == sizeof(uint8_t), "ui_update_flag_t larger than byte; review of code required");