From 7e00a6682d7141855c227a10dcdefa76c716165a Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Mon, 8 Jun 2026 09:57:58 +0200 Subject: [PATCH] ble race fixes --- zephcore/adapters/usb/ZephyrCompanionUSB.cpp | 20 ++--- zephcore/adapters/usb/ZephyrCompanionUSB.h | 5 +- zephcore/src/main_companion.cpp | 83 +++++++++++++++----- zephcore/src/main_repeater.cpp | 60 +++++++++++--- zephcore/src/main_room_server.cpp | 60 +++++++++++--- 5 files changed, 169 insertions(+), 59 deletions(-) diff --git a/zephcore/adapters/usb/ZephyrCompanionUSB.cpp b/zephcore/adapters/usb/ZephyrCompanionUSB.cpp index 360f9d4..698e59b 100644 --- a/zephcore/adapters/usb/ZephyrCompanionUSB.cpp +++ b/zephcore/adapters/usb/ZephyrCompanionUSB.cpp @@ -67,8 +67,12 @@ static uint8_t usb_tx_ring_buf_data[USB_TX_RING_BUF_SIZE]; static struct ring_buf usb_tx_ring_buf; static struct k_spinlock usb_tx_lock; -/* Work item for deferred V3 frame processing (set by init) */ -static struct k_work *s_rx_work; +/* Main-thread wake for assembled binary frames (set by init). The byte + * assembly below runs on sysworkq, but V3-protocol parsing must happen on the + * main thread (handleProtocolFrame mutates mesh state shared with loop()), so + * we post this event instead of running the parser here. */ +static struct k_event *s_mesh_events; +static uint32_t s_mesh_event_ble_rx; /* Session start/end callbacks (mirror BLE on_connected / on_disconnected), * set by main. start fires on first-frame claim, end on DTR drop. */ @@ -311,10 +315,10 @@ static void usb_rx_work_fn(struct k_work *work) } f; f.len = payload_len; memcpy(f.buf, payload, payload_len); - /* sysworkq handles V3-protocol parsing; main only wakes - * if downstream LoRa work gets enqueued (via TX_DRAIN). */ + /* Queue the frame and wake the main thread to parse it + * (parsing on sysworkq would race loop()). */ if (k_msgq_put(zephcore_ble_get_recv_queue(), &f, K_NO_WAIT) == 0) { - k_work_submit(s_rx_work); + k_event_post(s_mesh_events, s_mesh_event_ble_rx); } } @@ -479,15 +483,13 @@ void zephcore_usb_companion_write_text(const char *text, size_t len) } void zephcore_usb_companion_init(struct k_event *mesh_events, - struct k_work *rx_work, uint32_t mesh_event_ble_rx, void *board) { ARG_UNUSED(board); - ARG_UNUSED(mesh_events); - ARG_UNUSED(mesh_event_ble_rx); - s_rx_work = rx_work; + s_mesh_events = mesh_events; + s_mesh_event_ble_rx = mesh_event_ble_rx; /* The cdc_acm_uart DT node may be present without the class driver compiled * (shared esp32s3_usb_otg.dtsi exposes the node unconditionally; the class is diff --git a/zephcore/adapters/usb/ZephyrCompanionUSB.h b/zephcore/adapters/usb/ZephyrCompanionUSB.h index 50ab3ad..23abd15 100644 --- a/zephcore/adapters/usb/ZephyrCompanionUSB.h +++ b/zephcore/adapters/usb/ZephyrCompanionUSB.h @@ -21,12 +21,11 @@ extern "C" { * Call after device_is_ready() for the CDC ACM device. * * @param mesh_events Pointer to the k_event used by the mesh event loop - * @param rx_work Pointer to the rx_process work item (for BLE RX event) - * @param mesh_event_ble_rx Bitmask for MESH_EVENT_BLE_RX + * @param mesh_event_ble_rx Bitmask for MESH_EVENT_BLE_RX (posted to wake the + * main thread when a complete frame is assembled) * @param board Opaque pointer to ZephyrBoard (for DFU reboot) */ void zephcore_usb_companion_init(struct k_event *mesh_events, - struct k_work *rx_work, uint32_t mesh_event_ble_rx, void *board); diff --git a/zephcore/src/main_companion.cpp b/zephcore/src/main_companion.cpp index c02d679..77a654e 100644 --- a/zephcore/src/main_companion.cpp +++ b/zephcore/src/main_companion.cpp @@ -133,9 +133,12 @@ static void request_rtc_save(uint32_t epoch) } /* Work items for event-driven processing */ -static void rx_process_work_fn(struct k_work *work); +static void process_companion_rx(void); /* runs on MAIN thread (see ble_on_rx_frame) */ static void contact_iter_work_fn(struct k_work *work); static void housekeeping_timer_fn(struct k_timer *timer); +#if ZEPHCORE_USB_STACK +static void companion_cli_run(const char *line); /* main-thread text-CLI exec */ +#endif #if IS_ENABLED(CONFIG_ZEPHCORE_UI_DESIGN_JOYSTICK) static JoystickUITask joystick_ui_task; @@ -151,9 +154,18 @@ static void joystick_signal_tx(void) } #endif -K_WORK_DEFINE(rx_process_work, rx_process_work_fn); K_WORK_DEFINE(contact_iter_work, contact_iter_work_fn); +#if ZEPHCORE_USB_STACK +/* Completed USB text-CLI lines are run on the MAIN thread (the USB adapter + * assembles them on sysworkq). CommonCLI::handleCommand mutates mesh state + * shared with loop(), so it can't run on sysworkq — same hazard as the + * binary protocol path. Drained on MESH_EVENT_BLE_RX. */ +#define CLI_LINE_BUF_SIZE 256 +struct companion_cli_line { char buf[CLI_LINE_BUF_SIZE]; }; +K_MSGQ_DEFINE(companion_cli_queue, sizeof(struct companion_cli_line), 4, 4); +#endif + #if ZEPHCORE_USB_STACK /* USB TX-drained callback (mirrors BLE on_tx_idle): the TX ring emptied, so * resume the contact pump to queue the next batch. Runs in the CDC TX @@ -197,13 +209,12 @@ static void ble_on_rx_frame(const uint8_t *data, uint16_t len) return; } - /* Hand the frame to sysworkq for V3-protocol parsing. Main thread - * doesn't need a direct wake here: if handleProtocolFrame ends up - * enqueueing an outbound LoRa packet, notifyTxQueued() schedules - * tx_drain_work which posts MESH_EVENT_TX_DRAIN — that's the only - * signal main needs to drive the dispatcher. BLE-only commands - * (login, time, app-start, …) are handled entirely on sysworkq. */ - k_work_submit(&rx_process_work); + /* Wake the MAIN thread to parse the frame. handleProtocolFrame() + * mutates the lock-free packet pool / dispatcher that loop() also + * touches, so it MUST run on the main thread — parsing on sysworkq + * races the main loop (the source of the stuck-"Sending…" bug when an + * inbound reply is processed while a send command is parsed). */ + k_event_post(&mesh_events, MESH_EVENT_BLE_RX); } /* BLE TX idle callback — called when TX queue is empty. @@ -340,10 +351,12 @@ static void push_callback(uint8_t code, const uint8_t *data, size_t len) } /* RX processing work - handles received BLE/USB frames */ -static void rx_process_work_fn(struct k_work *work) +/* Parse inbound BLE/USB binary frames on the MAIN thread. Called from the + * event loop on MESH_EVENT_BLE_RX (formerly a sysworkq work item — moved to + * the main thread so handleProtocolFrame's mesh-state mutation can't race + * loop()). */ +static void process_companion_rx(void) { - ARG_UNUSED(work); - struct { uint16_t len; uint8_t buf[MAX_FRAME_SIZE]; @@ -458,6 +471,19 @@ static void mesh_event_loop(void) gps_process_event(); } + /* Parse inbound BLE/USB frames + USB text-CLI lines HERE (main + * thread) before loop() drains any outbound they enqueued — keeps + * all mesh-state mutation on one thread (see ble_on_rx_frame). */ + if (events & MESH_EVENT_BLE_RX) { + process_companion_rx(); +#if ZEPHCORE_USB_STACK + struct companion_cli_line c; + while (k_msgq_get(&companion_cli_queue, &c, K_NO_WAIT) == 0) { + companion_cli_run(c.buf); + } +#endif + } + /* Packet processing — only on radio/BLE/TX events */ if (companion_mesh_ptr && (events & (MESH_EVENT_LORA_RX | MESH_EVENT_LORA_TX_DONE | @@ -733,7 +759,10 @@ static bool handle_autoshutdown_cli(const char *line, char *reply) } #endif -static void companion_cli_dispatch(const char *line) +/* Executes a text-CLI line — runs on the MAIN thread (drained from + * companion_cli_queue in the event loop). CommonCLI::handleCommand touches + * mesh state shared with loop(), so it must not run on sysworkq. */ +static void companion_cli_run(const char *line) { char reply[CLI_REPLY_SIZE]; reply[0] = '\0'; @@ -754,6 +783,20 @@ static void companion_cli_dispatch(const char *line) zephcore_usb_companion_write_text("\r\n", 2); } +/* USB-adapter callback (runs on sysworkq). Just queue the line and wake the + * main thread — the actual handleCommand happens in companion_cli_run(). */ +static void companion_cli_dispatch(const char *line) +{ + struct companion_cli_line c; + strncpy(c.buf, line, sizeof(c.buf) - 1); + c.buf[sizeof(c.buf) - 1] = '\0'; + if (k_msgq_put(&companion_cli_queue, &c, K_NO_WAIT) == 0) { + k_event_post(&mesh_events, MESH_EVENT_BLE_RX); + } else { + zephcore_usb_companion_write_text("\r\n -> busy\r\n", 13); + } +} + #endif /* ZEPHCORE_USB_STACK */ #endif @@ -1093,7 +1136,7 @@ int main(void) * via CONFIG_ZEPHCORE_COMPANION_USB (e.g. prod builds on USB-capable boards). */ #if ZEPHCORE_USB_STACK - zephcore_usb_companion_init(&mesh_events, &rx_process_work, MESH_EVENT_BLE_RX, + zephcore_usb_companion_init(&mesh_events, MESH_EVENT_BLE_RX, &zephyr_board); /* Mirror BLE connect/disconnect UI + cleanup for USB sessions. */ zephcore_usb_companion_set_session_start_cb(usb_on_session_start); @@ -1123,16 +1166,14 @@ int main(void) * - MESH_EVENT_LORA_RX: LoRa packet received (from RX async callback) * - MESH_EVENT_LORA_TX_DONE: LoRa TX complete (from TX poll work -> callback) * - MESH_EVENT_HOUSEKEEPING: Periodic maintenance (noise floor, etc.) - * - MESH_EVENT_TX_DRAIN: outbound LoRa packet queued (covers BLE/USB-driven - * mesh.send paths — sysworkq parses incoming frames and posts this - * when it actually enqueues something) + * - MESH_EVENT_BLE_RX: BLE/USB frame (or USB text-CLI line) ready to parse. + * The BLE callback / USB adapter only assemble + queue the frame off-main; + * process_companion_rx() (and companion_cli_run()) parse it HERE on the + * main thread so mesh-state mutation never races loop(). + * - MESH_EVENT_TX_DRAIN: outbound LoRa packet queued — wakes loop() to drain * - MESH_EVENT_UI_ACTION / GPS_ACTION / PREFS_DIRTY: deferred work * from non-main threads * - * BLE/USB RX frames are parsed entirely on sysworkq (rx_process_work) — - * no direct main-thread wake is needed; if the parsed command produces - * outbound LoRa traffic, notifyTxQueued() routes it via TX_DRAIN. - * * Other processing still uses work queues: * - BLE TX: write_frame -> tx_drain_work (event-driven via notify callback) * - Contact iteration: contact_iter_work (on TX queue space) diff --git a/zephcore/src/main_repeater.cpp b/zephcore/src/main_repeater.cpp index c6331f2..bfcad43 100644 --- a/zephcore/src/main_repeater.cpp +++ b/zephcore/src/main_repeater.cpp @@ -117,6 +117,14 @@ static char cli_line_buf[CLI_LINE_BUF_SIZE]; static char cli_reply_buf[256]; static uint16_t cli_line_idx; +/* Completed CLI lines are handed to the MAIN thread for execution. Byte + * assembly + echo (cli_rx_work_fn) runs on sysworkq and touches no mesh + * state, but handleCommand() mutates the lock-free packet pool / dispatcher + * that loop() also touches — running it on sysworkq races the main loop. + * So we queue the finished line and let the event loop run handleCommand(). */ +struct cli_cmd_line { char buf[CLI_LINE_BUF_SIZE]; }; +K_MSGQ_DEFINE(cli_cmd_queue, sizeof(struct cli_cmd_line), 4, 4); + /* Work items for event-driven processing */ static void cli_rx_work_fn(struct k_work *work); static void housekeeping_timer_fn(struct k_timer *timer); @@ -178,22 +186,22 @@ static void cli_rx_work_fn(struct k_work *work) LOG_INF("CLI cmd len=%d: %.40s%s", cli_line_idx, cli_line_buf, cli_line_idx > 40 ? "..." : ""); - /* Process CLI command */ -#ifdef ZEPHCORE_LORA - if (repeater_mesh_ptr) { - cli_reply_buf[0] = '\0'; - repeater_mesh_ptr->handleCommand(0, cli_line_buf, cli_reply_buf); - if (cli_reply_buf[0] != '\0') { - /* Arduino format: newline, then " -> reply" */ - cli_print("\r\n -> "); - cli_print(cli_reply_buf); - } + /* Hand the command to the main thread (see cli_cmd_queue). + * The reply + trailing newline are emitted there, exactly + * matching the previous inline output order. */ + struct cli_cmd_line c; + strncpy(c.buf, cli_line_buf, sizeof(c.buf) - 1); + c.buf[sizeof(c.buf) - 1] = '\0'; + if (k_msgq_put(&cli_cmd_queue, &c, K_NO_WAIT) == 0) { + k_event_post(&mesh_events, MESH_EVENT_CLI_RX); + } else { + cli_print("\r\n -> busy\r\n"); } -#endif cli_line_idx = 0; + } else { + /* Empty line — just emit the newline (no command to run) */ + cli_print("\r\n"); } - /* New line for next command */ - cli_print("\r\n"); } else if (byte == 0x7F || byte == 0x08) { /* Backspace - echo backspace sequence */ if (cli_line_idx > 0) { @@ -214,6 +222,25 @@ static void cli_rx_work_fn(struct k_work *work) } } +#ifdef ZEPHCORE_LORA +/* Run queued CLI commands on the MAIN thread (see cli_cmd_queue). Output + * order matches the old inline path exactly: "\r\n -> reply" when there is + * a reply, then a trailing "\r\n". */ +static void process_cli_commands(void) +{ + struct cli_cmd_line c; + while (repeater_mesh_ptr && k_msgq_get(&cli_cmd_queue, &c, K_NO_WAIT) == 0) { + cli_reply_buf[0] = '\0'; + repeater_mesh_ptr->handleCommand(0, c.buf, cli_reply_buf); + if (cli_reply_buf[0] != '\0') { + cli_print("\r\n -> "); + cli_print(cli_reply_buf); + } + cli_print("\r\n"); + } +} +#endif + /* Housekeeping timer callback - signals event to wake mesh loop periodically */ static void housekeeping_timer_fn(struct k_timer *timer) { @@ -358,6 +385,13 @@ static void repeater_event_loop(void) } #ifdef ZEPHCORE_LORA + /* Run queued CLI commands here (main thread) BEFORE loop() drains + * any outbound packets they enqueued — keeps all mesh-state + * mutation on the main thread (see cli_cmd_queue). */ + if (events & MESH_EVENT_CLI_RX) { + process_cli_commands(); + } + /* Packet processing — only on radio/CLI/TX events */ if (repeater_mesh_ptr && (events & (MESH_EVENT_LORA_RX | MESH_EVENT_LORA_TX_DONE | diff --git a/zephcore/src/main_room_server.cpp b/zephcore/src/main_room_server.cpp index feebfc4..2ef60be 100644 --- a/zephcore/src/main_room_server.cpp +++ b/zephcore/src/main_room_server.cpp @@ -118,6 +118,14 @@ static char cli_line_buf[CLI_LINE_BUF_SIZE]; static char cli_reply_buf[256]; static uint16_t cli_line_idx; +/* Completed CLI lines are handed to the MAIN thread for execution. Byte + * assembly + echo (cli_rx_work_fn) runs on sysworkq and touches no mesh + * state, but handleCommand() mutates the lock-free packet pool / dispatcher + * that loop() also touches — running it on sysworkq races the main loop. + * So we queue the finished line and let the event loop run handleCommand(). */ +struct cli_cmd_line { char buf[CLI_LINE_BUF_SIZE]; }; +K_MSGQ_DEFINE(cli_cmd_queue, sizeof(struct cli_cmd_line), 4, 4); + /* Work items for event-driven processing */ static void cli_rx_work_fn(struct k_work *work); static void housekeeping_timer_fn(struct k_timer *timer); @@ -191,22 +199,22 @@ static void cli_rx_work_fn(struct k_work *work) LOG_INF("CLI cmd len=%d: %.40s%s", cli_line_idx, cli_line_buf, cli_line_idx > 40 ? "..." : ""); - /* Process CLI command */ -#ifdef ZEPHCORE_LORA - if (room_mesh_ptr) { - cli_reply_buf[0] = '\0'; - room_mesh_ptr->handleCommand(0, cli_line_buf, cli_reply_buf); - if (cli_reply_buf[0] != '\0') { - /* Arduino format: newline, then " -> reply" */ - cli_print("\r\n -> "); - cli_print(cli_reply_buf); - } + /* Hand the command to the main thread (see cli_cmd_queue). + * The reply + trailing newline are emitted there, exactly + * matching the previous inline output order. */ + struct cli_cmd_line c; + strncpy(c.buf, cli_line_buf, sizeof(c.buf) - 1); + c.buf[sizeof(c.buf) - 1] = '\0'; + if (k_msgq_put(&cli_cmd_queue, &c, K_NO_WAIT) == 0) { + k_event_post(&mesh_events, MESH_EVENT_CLI_RX); + } else { + cli_print("\r\n -> busy\r\n"); } -#endif cli_line_idx = 0; + } else { + /* Empty line — just emit the newline (no command to run) */ + cli_print("\r\n"); } - /* New line for next command */ - cli_print("\r\n"); } else if (byte == 0x7F || byte == 0x08) { /* Backspace - echo backspace sequence */ if (cli_line_idx > 0) { @@ -227,6 +235,25 @@ static void cli_rx_work_fn(struct k_work *work) } } +#ifdef ZEPHCORE_LORA +/* Run queued CLI commands on the MAIN thread (see cli_cmd_queue). Output + * order matches the old inline path exactly: "\r\n -> reply" when there is + * a reply, then a trailing "\r\n". */ +static void process_cli_commands(void) +{ + struct cli_cmd_line c; + while (room_mesh_ptr && k_msgq_get(&cli_cmd_queue, &c, K_NO_WAIT) == 0) { + cli_reply_buf[0] = '\0'; + room_mesh_ptr->handleCommand(0, c.buf, cli_reply_buf); + if (cli_reply_buf[0] != '\0') { + cli_print("\r\n -> "); + cli_print(cli_reply_buf); + } + cli_print("\r\n"); + } +} +#endif + /* Housekeeping timer callback - signals event to wake mesh loop periodically */ static void housekeeping_timer_fn(struct k_timer *timer) { @@ -375,6 +402,13 @@ static void room_event_loop(void) } #ifdef ZEPHCORE_LORA + /* Run queued CLI commands here (main thread) BEFORE loop() drains + * any outbound packets they enqueued — keeps all mesh-state + * mutation on the main thread (see cli_cmd_queue). */ + if (events & MESH_EVENT_CLI_RX) { + process_cli_commands(); + } + /* Packet processing — only on radio/CLI/TX events */ if (room_mesh_ptr && (events & (MESH_EVENT_LORA_RX | MESH_EVENT_LORA_TX_DONE |