From d0e4e724c704134a68ae390f3b0ac019b5025b94 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 23 Dec 2024 15:49:59 -0800 Subject: [PATCH 1/4] Warning if core assertions occur in early init --- src/debug_rtt.h | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/debug_rtt.h b/src/debug_rtt.h index 63470348..f3bb4551 100644 --- a/src/debug_rtt.h +++ b/src/debug_rtt.h @@ -202,23 +202,34 @@ static inline bool bp_debug_should_print(bp_debug_level_t level, bp_debug_catego void bp_mark_system_initialized(void); bool bp_is_system_initialized(void); -#ifdef NDEBUG - -// These assertions only assert after the system is fully initialized. +// Verify execution is on Core0 ... but only warn if system is not 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); \ - } \ + do { \ + if (get_core_num() != 0) { \ + if (bp_is_system_initialized()) { \ + PRINT_FATAL("Not on Core0: %s:%d\n", __FILE__, __LINE__); \ + hard_assert(false); \ + } else { \ + PRINT_FATAL("Warning: Not on Core0 during init: %s:%d\n", __FILE__, __LINE__); \ + } \ + } \ } while (0) + +// Verify execution is on Core1 ... but only warn if system is not fully initialized. #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); \ - } \ + do { \ + if (get_core_num() != 1) { \ + if (bp_is_system_initialized()) { \ + PRINT_FATAL("Not on Core1: %s:%d\n", __FILE__, __LINE__); \ + hard_assert(false); \ + } else { \ + PRINT_FATAL("Warning: Not on Core1 during init: %s:%d\n", __FILE__, __LINE__); \ + } \ + } \ } while (0) + +// Ensures an assertion is checked and file/line number are printed (even in NDEBUG compilation). +// If used hard_assert() directly, file/line number would not be shown. #define BP_ASSERT(_COND) \ do { \ if (!(_COND)) { \ @@ -226,13 +237,3 @@ bool bp_is_system_initialized(void); 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 - - From 6985dc0b037a3227e333d7c46a4d46c8e0e45a36 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 23 Dec 2024 15:50:18 -0800 Subject: [PATCH 2/4] COMMENTS ONLY: Mark potential problems --- src/commands/global/script.c | 2 +- src/mode/binloopback.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/commands/global/script.c b/src/commands/global/script.c index 80a84216..823d7f79 100644 --- a/src/commands/global/script.c +++ b/src/commands/global/script.c @@ -96,7 +96,7 @@ bool script_exec(char* location, bool pause_for_input, bool show_comments, bool if (file[i] == '\r' || file[i] == '\n' || file[i] == '\0') { break; } - rx_fifo_add(&file[i]); + rx_fifo_add(&file[i]); // BUGBUG -- breaks FIFO queue only being added to by Core1 // cmdln_try_add(&file[i]); // tx_fifo_put(&file[i]); } diff --git a/src/mode/binloopback.c b/src/mode/binloopback.c index c6c5f509..d56972fa 100644 --- a/src/mode/binloopback.c +++ b/src/mode/binloopback.c @@ -75,13 +75,13 @@ void binloopback_close(struct _bytecode* result, struct _bytecode* next) { void binloopback_write(struct _bytecode* result, struct _bytecode* next) { char c = result->out_data; - bin_rx_fifo_add(&c); + bin_rx_fifo_add(&c); // ?? BUGBUG -- should this be bin_tx_fifo_add() ?? } void binloopback_read(struct _bytecode* result, struct _bytecode* next) { uint32_t timeout = 0xfff; char c; - while (!bin_tx_fifo_try_get(&c)) { + while (!bin_tx_fifo_try_get(&c)) { // ?? BUGBUG -- should this be bin_rx_fifo_try_get() ?? timeout--; if (!timeout) { result->error = SERR_ERROR; From cda6cb7ceb997dc63513dd6f2250d0d08774de08 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 23 Dec 2024 16:13:31 -0800 Subject: [PATCH 3/4] Assert RX/TX core rules are followed --- src/usb_rx.c | 32 ++++++++++++++++++++++++++++---- src/usb_tx.c | 49 +++++++++++++++++++++++++++++++++++++------------ 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/usb_rx.c b/src/usb_rx.c index 7e607fba..d7a5c782 100644 --- a/src/usb_rx.c +++ b/src/usb_rx.c @@ -26,19 +26,24 @@ void rx_uart_irq_handler(void); queue_t rx_fifo; queue_t bin_rx_fifo; -#define RX_FIFO_LENGTH_IN_BITS 7 // 2^n buffer size. 2^3=8, 2^9=512 +#define RX_FIFO_LENGTH_IN_BITS 7 // tinyUSB requires 2^n buffer size, so declare in bits. 2^3=8, 2^7=128, 2^9=512, etc. #define RX_FIFO_LENGTH_IN_BYTES (0x0001 << RX_FIFO_LENGTH_IN_BITS) char rx_buf[RX_FIFO_LENGTH_IN_BYTES]; char bin_rx_buf[RX_FIFO_LENGTH_IN_BYTES]; // init buffer (and IRQ for UART debug mode) void rx_fifo_init(void) { + // OK to call from either core queue2_init(&rx_fifo, rx_buf, RX_FIFO_LENGTH_IN_BYTES); // buffer size must be 2^n for queue AND DMA rollover queue2_init(&bin_rx_fifo, bin_rx_buf, RX_FIFO_LENGTH_IN_BYTES); } // enables receive interrupt for ALREADY configured debug uarts (see init in debug.c) void rx_uart_init_irq(void) { + // RX FIFO (whether from UART, CDC, RTT, ...) should only be added to from core1 (deadlock risk) + // irq_set_exclusive_handler() sets the interrupt to be exclusive to the current core. + // therefore, this function must be called from Core1. + BP_ASSERT_CORE1(); // rx interrupt irq_set_exclusive_handler(debug_uart[system_config.terminal_uart_number].irq, rx_uart_irq_handler); // irq_set_priority(debug_uart[system_config.terminal_uart_number].irq, 0xFF); @@ -48,14 +53,16 @@ void rx_uart_init_irq(void) { // UART interrupt handler void rx_uart_irq_handler(void) { + BP_ASSERT_CORE1(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be added to from core1 (deadlock risk) // while bytes available shove them in the buffer while (uart_is_readable(debug_uart[system_config.terminal_uart_number].uart)) { uint8_t c = uart_getc(debug_uart[system_config.terminal_uart_number].uart); - queue2_add_blocking(&rx_fifo, &c); + queue2_add_blocking(&rx_fifo, &c); // BUGBUG -- blocking call from ISR! } } void rx_usb_init(void) { + BP_ASSERT_CORE1(); tusb_init(); } @@ -64,6 +71,8 @@ void rx_usb_init(void) { // If RX queue is full, store the last character in a local static variable. Always use that character // (if positive) first when entering the function. Thus, no RTT input characters are ever lost. void rx_from_rtt_terminal(void) { + BP_ASSERT_CORE1(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be added to from core1 (deadlock risk) + static int last_character = -1; // if prior callback left a prior character, use it first. @@ -88,6 +97,9 @@ void rx_from_rtt_terminal(void) { // USB (tinyUSB) interrupt handler // Invoked when CDC interface received data from host void tud_cdc_rx_cb(uint8_t itf) { + + BP_ASSERT_CORE1(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be added to from core1 (deadlock risk) + char buf[64]; if (itf == 0 && tud_cdc_n_available(0)) { @@ -95,7 +107,7 @@ void tud_cdc_rx_cb(uint8_t itf) { // while bytes available shove them in the buffer for (uint8_t i = 0; i < count; i++) { - queue2_add_blocking(&rx_fifo, &buf[i]); + queue2_add_blocking(&rx_fifo, &buf[i]); // BUGBUG -- blocking call from ISR! } } @@ -104,53 +116,65 @@ void tud_cdc_rx_cb(uint8_t itf) { // while bytes available shove them in the buffer for (uint8_t i = 0; i < count; i++) { - queue2_add_blocking(&bin_rx_fifo, &buf[i]); + queue2_add_blocking(&bin_rx_fifo, &buf[i]); // BUGBUG -- blocking call from ISR! } } } // Invoked when cdc when line state changed e.g connected/disconnected void tud_cdc_line_state_cb(uint8_t itf, bool dtr, bool rts) { + BP_DEBUG_PRINT(BP_DEBUG_LEVEL_VERBOSE, BP_DEBUG_CAT_TEMP, + "--> tud_cdc_line_state_cb(%d, %d, %d)\n", itf, dtr, rts + ); system_config.rts = rts; } // insert a byte into the queue void rx_fifo_add(char* c) { + BP_ASSERT_CORE1(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be added to from core1 (deadlock risk) queue2_add_blocking(&rx_fifo, c); } // functions to access the ring buffer from other code // block until a byte is available, remove from buffer void rx_fifo_get_blocking(char* c) { + BP_ASSERT_CORE0(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be drained from core0 (deadlock risk) queue2_remove_blocking(&rx_fifo, c); } // try to get a byte, remove from buffer if available, return false if no byte bool rx_fifo_try_get(char* c) { + // OK to call from either core return queue2_try_remove(&rx_fifo, c); } // block until a byte is available, return byte but leave in buffer void rx_fifo_peek_blocking(char* c) { + BP_ASSERT_CORE0(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be drained from core0 (deadlock risk) queue2_peek_blocking(&rx_fifo, c); } // try to peek at next byte, return byte but leave in buffer, return false if no byte bool rx_fifo_try_peek(char* c) { + // OK to call from either core return queue2_try_peek(&rx_fifo, c); } // BINMODE queue void bin_rx_fifo_add(char* c) { + BP_ASSERT_CORE1(); queue2_add_blocking(&bin_rx_fifo, c); } void bin_rx_fifo_get_blocking(char* c) { + BP_ASSERT_CORE0(); // RX FIFO (whether from UART, CDC, RTT, ...) should only be drained from core0 (deadlock risk) queue2_remove_blocking(&bin_rx_fifo, c); } void bin_rx_fifo_available_bytes(uint16_t* cnt) { + // OK to call from either core queue_available_bytes(&bin_rx_fifo, cnt); } bool bin_rx_fifo_try_get(char* c) { + // OK to call from either core bool result = queue2_try_remove(&bin_rx_fifo, c); // if(result) printf("%.2x ", (*c)); return result; diff --git a/src/usb_tx.c b/src/usb_tx.c index 94eaae91..216ac5a7 100644 --- a/src/usb_tx.c +++ b/src/usb_tx.c @@ -30,18 +30,46 @@ queue_t bin_tx_fifo; char tx_buf[TX_FIFO_LENGTH_IN_BYTES] __attribute__((aligned(2048))); char bin_tx_buf[TX_FIFO_LENGTH_IN_BYTES] __attribute__((aligned(2048))); -char tx_sb_buf[1024]; +#define MAXIMUM_STATUS_BAR_BUFFER_BYTES 1024 +char tx_sb_buf[MAXIMUM_STATUS_BAR_BUFFER_BYTES]; uint16_t tx_sb_buf_cnt = 0; uint16_t tx_sb_buf_index = 0; bool tx_sb_buf_ready = false; void tx_fifo_init(void) { + // OK to call from either core queue2_init(&tx_fifo, tx_buf, TX_FIFO_LENGTH_IN_BYTES); // buffer size must be 2^n for queue AND DMA rollover queue2_init(&bin_tx_fifo, bin_tx_buf, TX_FIFO_LENGTH_IN_BYTES); // buffer size must be 2^n for queue AND DMA // rollover } void tx_sb_start(uint32_t valid_characters_in_status_bar) { + + // BUGBUG -- This is currently called from both cores. + // This means the buffer is being updated from both cores ... + // and there is no lock / protection on the buffer. + + // // Something updated the status bar buffer. + // // Which core is this on? + // static uint prior_core = 0xFFFFu; + // uint core = get_core_num(); + // if (prior_core == 0xFFFFu) { + // prior_core = core; + // } else if (prior_core > 2) { + // --prior_core; + // } else if (prior_core == 2) { + // prior_core = core; + // } + + // if ((prior_core < 2) && (prior_core != core)) { + // BP_DEBUG_PRINT(BP_DEBUG_LEVEL_FATAL, BP_DEBUG_CAT_CATCHALL, + // "tx_sb_start() is called from multiple cores\n" + // ); + // // prior_core = 10; // limit how often we see this message + // prior_core = core; // show this every time it changes + // } + + BP_ASSERT(valid_characters_in_status_bar <= MAXIMUM_STATUS_BAR_BUFFER_BYTES); tx_sb_buf_cnt = valid_characters_in_status_bar; tx_sb_buf_ready = true; } @@ -145,27 +173,22 @@ void tx_fifo_service(void) { } void tx_fifo_put(char* c) { - uint core = get_core_num(); - assert(core == 0); // tx fifo should not be shared between cores - assert(core != 1); // tx fifo can deadlock if called from core1 + BP_ASSERT_CORE0(); // tx fifo shoudl only be added to from core 0 (deadlock risk) queue2_add_blocking(&tx_fifo, c); } void bin_tx_fifo_put(const char c) { - uint core = get_core_num(); - assert(core == 0); // binmode fifo should not be shared between cores - assert(core != 1); // binmode fifo can deadlock if called from core1 + BP_ASSERT_CORE0(); // tx fifo shoudl only be added to from core 0 (deadlock risk) queue2_add_blocking(&bin_tx_fifo, &c); } bool bin_tx_fifo_try_get(char* c) { + BP_ASSERT_CORE1(); // tx fifo is drained from core1 only return queue2_try_remove(&bin_tx_fifo, c); } -// #if(0) void bin_tx_fifo_service(void) { - uint core = get_core_num(); - assert(core == 1); // binmode fifo is drained from core1 only + BP_ASSERT_CORE1(); // tx fifo is drained from core1 only uint16_t bytes_available; char data[64]; @@ -190,10 +213,12 @@ void bin_tx_fifo_service(void) { tud_cdc_n_write(1, &data, i); tud_cdc_n_write_flush(1); } -// #endif bool bin_tx_not_empty(void) { + // OK to check empty from either core uint16_t cnt; + // gets the number of additional bytes that can be added to the queue queue_available_bytes(&bin_tx_fifo, &cnt); - return TX_FIFO_LENGTH_IN_BYTES - cnt; + // If that differs from the total size of the queue, then it's not empty + return cnt != TX_FIFO_LENGTH_IN_BYTES; } From 7acf0f7a8a56eba337b123ab25e908e2a3240b36 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 23 Dec 2024 16:59:11 -0800 Subject: [PATCH 4/4] Fix statusbar corruption --- src/commands/global/cls.c | 3 ++- src/pirate.c | 4 ++-- src/ui/ui_config.c | 4 +++- src/ui/ui_statusbar.c | 8 +++++++- src/ui/ui_statusbar.h | 6 ++++-- src/usb_tx.c | 25 +------------------------ 6 files changed, 19 insertions(+), 31 deletions(-) diff --git a/src/commands/global/cls.c b/src/commands/global/cls.c index b1f7b112..82a094cd 100644 --- a/src/commands/global/cls.c +++ b/src/commands/global/cls.c @@ -17,6 +17,7 @@ static const char* const usage[] = { static const struct ui_help_options options[] = {}; void ui_display_clear(struct command_result* res) { + BP_ASSERT_CORE0(); if (ui_help_show(res->help_flag, usage, count_of(usage), &options[0], count_of(options))) { return; } @@ -30,6 +31,6 @@ void ui_display_clear(struct command_result* res) { ui_term_init(); // Initialize VT100 if ANSI terminal if (system_config.terminal_ansi_color && system_config.terminal_ansi_statusbar) { ui_statusbar_init(); - ui_statusbar_update(UI_UPDATE_ALL); + ui_statusbar_update_blocking(); } } diff --git a/src/pirate.c b/src/pirate.c index 9208f956..14659276 100644 --- a/src/pirate.c +++ b/src/pirate.c @@ -522,7 +522,7 @@ static void core0_infinite_loop(void) { // and does the initial painting of the full statusbar if (system_config.terminal_ansi_statusbar) { ui_statusbar_init(); - ui_statusbar_update(UI_UPDATE_ALL); + ui_statusbar_update_blocking(); } break; default: @@ -768,7 +768,7 @@ static void core1_infinite_loop(void) { system_config.terminal_ansi_statusbar && system_config.terminal_ansi_statusbar_update && !system_config.terminal_ansi_statusbar_pause) { - ui_statusbar_update(update_flags); + ui_statusbar_update_from_core1(update_flags); } #if (BP_VER == 5 && BP_REV <= 9) diff --git a/src/ui/ui_config.c b/src/ui/ui_config.c index 05600412..c37eb913 100644 --- a/src/ui/ui_config.c +++ b/src/ui/ui_config.c @@ -153,6 +153,8 @@ uint32_t ui_config_action_ansi_color(uint32_t a, uint32_t b) { } uint32_t ui_config_action_ansi_toolbar(uint32_t a, uint32_t b) { + BP_ASSERT_CORE0(); + // NOTE: `b` is treated as a boolean value b = !!b; @@ -164,7 +166,7 @@ uint32_t ui_config_action_ansi_toolbar(uint32_t a, uint32_t b) { } ui_term_detect(); // Do we detect a VT100 ANSI terminal? what is the size? ui_term_init(); // Initialize VT100 if ANSI terminal - ui_statusbar_update(UI_UPDATE_ALL); + ui_statusbar_update_blocking(); } } diff --git a/src/ui/ui_statusbar.c b/src/ui/ui_statusbar.c index 8801447c..760750e4 100644 --- a/src/ui/ui_statusbar.c +++ b/src/ui/ui_statusbar.c @@ -11,6 +11,7 @@ #include "ui/ui_flags.h" #include "system_monitor.h" #include "display/scope.h" +#include "pirate/intercore_helpers.h" uint32_t ui_statusbar_info(char* buf, size_t buffLen) { uint32_t len = 0; @@ -209,8 +210,13 @@ uint32_t ui_statusbar_value(char* buf, size_t buffLen) { return (do_update ? len : 0); } +void ui_statusbar_update_blocking() { + BP_ASSERT_CORE0(); // if called from core1, this will deadlock + icm_core0_send_message_synchronous(BP_ICM_FORCE_LCD_UPDATE); +} +void ui_statusbar_update_from_core1(uint32_t update_flags) { + BP_ASSERT_CORE1(); -void ui_statusbar_update(uint32_t update_flags) { uint32_t len = 0; size_t buffLen = sizeof(tx_sb_buf); diff --git a/src/ui/ui_statusbar.h b/src/ui/ui_statusbar.h index 4ab0f71f..5a6980f4 100644 --- a/src/ui/ui_statusbar.h +++ b/src/ui/ui_statusbar.h @@ -1,3 +1,5 @@ -void ui_statusbar_update(uint32_t update_flags); +#pragma once +void ui_statusbar_update_blocking(); +void ui_statusbar_update_from_core1(uint32_t update_flags); void ui_statusbar_init(void); -void ui_statusbar_deinit(void); \ No newline at end of file +void ui_statusbar_deinit(void); diff --git a/src/usb_tx.c b/src/usb_tx.c index 216ac5a7..a628fd6a 100644 --- a/src/usb_tx.c +++ b/src/usb_tx.c @@ -45,30 +45,7 @@ void tx_fifo_init(void) { void tx_sb_start(uint32_t valid_characters_in_status_bar) { - // BUGBUG -- This is currently called from both cores. - // This means the buffer is being updated from both cores ... - // and there is no lock / protection on the buffer. - - // // Something updated the status bar buffer. - // // Which core is this on? - // static uint prior_core = 0xFFFFu; - // uint core = get_core_num(); - // if (prior_core == 0xFFFFu) { - // prior_core = core; - // } else if (prior_core > 2) { - // --prior_core; - // } else if (prior_core == 2) { - // prior_core = core; - // } - - // if ((prior_core < 2) && (prior_core != core)) { - // BP_DEBUG_PRINT(BP_DEBUG_LEVEL_FATAL, BP_DEBUG_CAT_CATCHALL, - // "tx_sb_start() is called from multiple cores\n" - // ); - // // prior_core = 10; // limit how often we see this message - // prior_core = core; // show this every time it changes - // } - + BP_ASSERT_CORE1(); BP_ASSERT(valid_characters_in_status_bar <= MAXIMUM_STATUS_BAR_BUFFER_BYTES); tx_sb_buf_cnt = valid_characters_in_status_bar; tx_sb_buf_ready = true;