From 878f414975c4c50ef77791f2036f702b0e120176 Mon Sep 17 00:00:00 2001 From: Raphael Coeffic Date: Wed, 26 Aug 2026 05:06:17 +0200 Subject: [PATCH] fix(trainer): SBUS trainer context ownership (#7699) Co-authored-by: raphaelcoeffic <1050031+raphaelcoeffic@users.noreply.github.com> --- radio/src/gui/gui_common.cpp | 2 +- radio/src/sbus.cpp | 61 +++++++++++++------ radio/src/sbus.h | 21 ++++--- radio/src/serial.cpp | 43 +++++++++++-- radio/src/serial.h | 9 +++ .../common/arm/stm32/stm32_serial_driver.cpp | 3 +- .../common/arm/stm32/stm32_usart_driver.cpp | 6 ++ radio/src/trainer.cpp | 51 ++++++++++++---- 8 files changed, 152 insertions(+), 44 deletions(-) diff --git a/radio/src/gui/gui_common.cpp b/radio/src/gui/gui_common.cpp index 3bd0d2281..21c800231 100644 --- a/radio/src/gui/gui_common.cpp +++ b/radio/src/gui/gui_common.cpp @@ -1176,7 +1176,7 @@ bool isTrainerModeAvailable(int mode) #endif if (mode == TRAINER_MODE_MASTER_SERIAL) { - return serialGetModePort(UART_MODE_SBUS_TRAINER) >= 0; + return serialGetSbusTrainerPort() >= 0; } if ((mode == TRAINER_MODE_MASTER_BLUETOOTH || diff --git a/radio/src/sbus.cpp b/radio/src/sbus.cpp index 5a86b2353..10bcab832 100644 --- a/radio/src/sbus.cpp +++ b/radio/src/sbus.cpp @@ -36,32 +36,16 @@ #define SBUS_CH_CENTER 0x3E0 +// Current SBUS trainer input source. Only ever written from the main task +// (see sbusTrainerAcquire() / sbusTrainerRelease()), only ever read from the +// USART IRQ, hence no atomics: what makes this safe is the arm/disarm order. static const etx_serial_driver_t* _sbus_drv = nullptr; static void* _sbus_ctx = nullptr; -static bool _sbus_aux_enabled = false; static void sbusProcessFrame(int16_t* pulses, uint8_t* sbus, uint32_t size); -void sbusSetReceiveCtx(void* ctx, const etx_serial_driver_t* drv) +static void sbusFrameReceived(void*) { - _sbus_ctx = ctx; - _sbus_drv = drv; -} - -void sbusAuxFrameReceived(void*) -{ - if (!_sbus_aux_enabled) return; - sbusFrameReceived(nullptr); -} - -void sbusAuxSetEnabled(bool enabled) { _sbus_aux_enabled = enabled; } - -void sbusFrameReceived(void*) -{ - if (!_sbus_drv || !_sbus_ctx || !_sbus_drv->copyRxBuffer || - !_sbus_drv->getBufferedBytes) - return; - if (_sbus_drv->getBufferedBytes(_sbus_ctx) != SBUS_FRAME_SIZE) { _sbus_drv->clearRxBuffer(_sbus_ctx); return; @@ -74,6 +58,43 @@ void sbusFrameReceived(void*) sbusProcessFrame(trainerInput, frame, received); } +bool sbusTrainerAcquire(void* ctx, const etx_serial_driver_t* drv) +{ + sbusTrainerRelease(); + + // sbusFrameReceived() relies on all of these being available + if (!ctx || !drv || !drv->getBufferedBytes || !drv->copyRxBuffer || + !drv->clearRxBuffer || !drv->setIdleCb) + return false; + + _sbus_ctx = ctx; + _sbus_drv = drv; + + // Arm last: the IDLE IRQ may fire as soon as this returns + drv->setIdleCb(ctx, sbusFrameReceived, nullptr); + return true; +} + +void sbusTrainerRelease() +{ + auto drv = _sbus_drv; + auto ctx = _sbus_ctx; + if (!drv || !ctx) return; + + // Disarm first: no further callback can start once this returns + drv->setIdleCb(ctx, nullptr, nullptr); + + _sbus_ctx = nullptr; + _sbus_drv = nullptr; +} + +void sbusTrainerReleaseCtx(void* ctx) +{ + if (ctx && ctx == _sbus_ctx) sbusTrainerRelease(); +} + +bool sbusTrainerActive() { return _sbus_ctx != nullptr; } + // Range for pulses (ppm input) is [-512:+512] static void sbusProcessFrame(int16_t* pulses, uint8_t* sbus, uint32_t size) { diff --git a/radio/src/sbus.h b/radio/src/sbus.h index e8089d0fc..4c79db862 100644 --- a/radio/src/sbus.h +++ b/radio/src/sbus.h @@ -25,13 +25,20 @@ #define SBUS_BAUDRATE 100000 -// SBUS serial driver + context -void sbusSetReceiveCtx(void* ctx, const etx_serial_driver_t* drv); +// The SBUS trainer receiver is a single consumer (trainerInput[]): only one +// source can feed it at a time. Which one is decided by the trainer mode, so +// the trainer owns the context and arms/disarms the IDLE callback itself. -// SBUS AUX idle callback -void sbusAuxFrameReceived(void* param); +// Claim the SBUS trainer receiver for a serial port. +// Returns false if the port cannot be used as SBUS trainer input. +bool sbusTrainerAcquire(void* ctx, const etx_serial_driver_t* drv); -// Enable / disable SBUS AUX -void sbusAuxSetEnabled(bool enabled); +// Release the SBUS trainer receiver (no-op if nothing is claimed) +void sbusTrainerRelease(); -void sbusFrameReceived(void* param); +// Is a source currently feeding the SBUS trainer receiver? +bool sbusTrainerActive(); + +// Release the SBUS trainer receiver, but only if 'ctx' is the current owner. +// Used when tearing down a serial port that may or may not be in use. +void sbusTrainerReleaseCtx(void* ctx); diff --git a/radio/src/serial.cpp b/radio/src/serial.cpp index 3148ca55a..e6d77b740 100644 --- a/radio/src/serial.cpp +++ b/radio/src/serial.cpp @@ -223,10 +223,8 @@ static void serialSetCallBacks(int mode, void* ctx, const etx_serial_port_t* por case UART_MODE_SBUS_TRAINER: case UART_MODE_SBUS_TRAINER_INV: - sbusSetReceiveCtx(ctx, drv); - if (drv && drv->setIdleCb) { - drv->setIdleCb(ctx, sbusAuxFrameReceived, nullptr); - } + // Nothing to do: the trainer claims the port when (and only when) the + // trainer mode selects it as input source. See sbusTrainerAcquire(). break; case UART_MODE_TELEMETRY: @@ -438,6 +436,10 @@ void serialInit(uint8_t port_nr, int mode) if (!port) return; if (state->port) { +#if !defined(BOOT) + // Drop the trainer input before the driver context goes away + sbusTrainerReleaseCtx(state->usart_ctx); +#endif auto drv = state->port->uart; if (drv && drv->deinit && state->usart_ctx) { drv->deinit(state->usart_ctx); @@ -499,6 +501,35 @@ void serialInit(uint8_t port_nr, int mode) #endif } +#if !defined(BOOT) +int serialGetSbusTrainerPort() +{ + int port_nr = serialGetModePort(UART_MODE_SBUS_TRAINER); + +#if defined(STM32H7) || defined(STM32H7RS) || defined(STM32H5) + // Ports without a hardware inverter rely on the USART inverting RX itself, + // which only exists on these families (see stm32_serial_init()). Elsewhere + // the mode cannot work, so do not offer such a port as trainer input even + // if a configuration written on another radio selects it. + if (port_nr < 0) port_nr = serialGetModePort(UART_MODE_SBUS_TRAINER_INV); +#endif + + return port_nr; +} + +bool serialGetPortCtx(uint8_t port_nr, void** ctx, + const etx_serial_driver_t** drv) +{ + auto state = getSerialPortState(port_nr); + if (!state || !state->port || !state->usart_ctx || !state->port->uart) + return false; + + *ctx = state->usart_ctx; + *drv = state->port->uart; + return true; +} +#endif + void initSerialPorts() { for (uint8_t port_nr = 0; port_nr < MAX_AUX_SERIAL; port_nr++) { @@ -544,6 +575,10 @@ void serialStop(uint8_t port_nr) if (!state) return; if (state->port) { +#if !defined(BOOT) + // Drop the trainer input before the driver context goes away + sbusTrainerReleaseCtx(state->usart_ctx); +#endif auto port = state->port; auto drv = port->uart; if (drv && drv->deinit) { diff --git a/radio/src/serial.h b/radio/src/serial.h index 47a307d2b..f579d31a1 100644 --- a/radio/src/serial.h +++ b/radio/src/serial.h @@ -45,6 +45,15 @@ void initSerialPorts(); void serialInit(uint8_t port_nr, int mode); void serialStop(uint8_t port_nr); +#if !defined(BOOT) +// Serial port configured as SBUS trainer input (inverted or not), or -1 +int serialGetSbusTrainerPort(); + +// Fetch the driver instance backing a serial port. False if not initialised. +bool serialGetPortCtx(uint8_t port_nr, void** ctx, + const etx_serial_driver_t** drv); +#endif + // // Functions used by debug.h // diff --git a/radio/src/targets/common/arm/stm32/stm32_serial_driver.cpp b/radio/src/targets/common/arm/stm32/stm32_serial_driver.cpp index b37eaa659..2a097327e 100644 --- a/radio/src/targets/common/arm/stm32/stm32_serial_driver.cpp +++ b/radio/src/targets/common/arm/stm32/stm32_serial_driver.cpp @@ -315,7 +315,8 @@ static void* stm32_serial_init(void* hw_def, const etx_serial_init* params) static void stm32_serial_deinit(void* ctx) { auto st = (stm32_serial_state*)ctx; - if (!st) return; + // !st->sp: state already freed, make de-init idempotent + if (!st || !st->sp) return; stm32_usart_deinit(st->sp->usart); stm32_serial_free_state(st); diff --git a/radio/src/targets/common/arm/stm32/stm32_usart_driver.cpp b/radio/src/targets/common/arm/stm32/stm32_usart_driver.cpp index 01aca83d1..e4d394e06 100644 --- a/radio/src/targets/common/arm/stm32/stm32_usart_driver.cpp +++ b/radio/src/targets/common/arm/stm32/stm32_usart_driver.cpp @@ -528,7 +528,13 @@ void stm32_usart_deinit(const stm32_usart_t* usart) } if ((int32_t)(usart->IRQn) >= 0) { + // An IRQ latched before (or in flight across) NVIC_DisableIRQ() would + // still be taken, and would then access a de-initialised, clock-gated + // peripheral. Barrier + clear pending closes that window. NVIC_DisableIRQ(usart->IRQn); + __DSB(); + __ISB(); + NVIC_ClearPendingIRQ(usart->IRQn); } LL_USART_DeInit(usart->USARTx); disable_usart_clock(usart->USARTx); diff --git a/radio/src/trainer.cpp b/radio/src/trainer.cpp index c845d459c..6b1f11575 100644 --- a/radio/src/trainer.cpp +++ b/radio/src/trainer.cpp @@ -22,6 +22,7 @@ #include "hal/trainer_driver.h" #include "edgetx.h" +#include "serial.h" // Timer gets decremented in per10ms() #define TRAINER_IN_VALID_TIMEOUT 100 // 1s @@ -90,6 +91,7 @@ void checkTrainerSignalWarning() } } +static bool trainer_init_aux_sbus(); static void trainer_init_module_sbus(); static void trainer_stop_module_sbus(); @@ -102,7 +104,7 @@ void stopTrainer() break; case TRAINER_MODE_MASTER_SERIAL: - sbusAuxSetEnabled(false); + sbusTrainerRelease(); break; case TRAINER_MODE_MASTER_CPPM_EXTERNAL_MODULE: @@ -139,7 +141,7 @@ void checkTrainerSettings() break; case TRAINER_MODE_MASTER_SERIAL: - sbusAuxSetEnabled(true); + trainer_init_aux_sbus(); break; case TRAINER_MODE_MASTER_CPPM_EXTERNAL_MODULE: @@ -155,6 +157,14 @@ void checkTrainerSettings() _on_change_cb(currentTrainerMode, requiredTrainerMode); } currentTrainerMode = requiredTrainerMode; + + } else if (currentTrainerMode == TRAINER_MODE_MASTER_SERIAL && + !sbusTrainerActive()) { + // The serial port backing this mode can be (re-)configured at any time + // from the radio settings, which drops us as its user. Pick it up again + // as soon as it is usable, rather than relying on every caller of + // serialInit() to notify us. + trainer_init_aux_sbus(); } } @@ -173,6 +183,20 @@ static const etx_serial_init sbusTrainerParams = { static etx_module_state_t* sbus_trainer_mod_st = nullptr; +// SBUS trainer input from an AUX serial port +static bool trainer_init_aux_sbus() +{ + int port_nr = serialGetSbusTrainerPort(); + if (port_nr < 0) return false; + + void* ctx; + const etx_serial_driver_t* drv; + if (!serialGetPortCtx(port_nr, &ctx, &drv)) return false; + + return sbusTrainerAcquire(ctx, drv); +} + +// SBUS trainer input from the external module bay static void trainer_init_module_sbus() { if (sbus_trainer_mod_st) return; @@ -186,23 +210,28 @@ static void trainer_init_module_sbus() EXTERNAL_MODULE, ETX_MOD_PORT_SPORT, &sbusTrainerParams, false); } - if (sbus_trainer_mod_st) { - // configure IDLE IRQ callback - auto drv = modulePortGetSerialDrv(sbus_trainer_mod_st->rx); - auto ctx = modulePortGetCtx(sbus_trainer_mod_st->rx); + if (!sbus_trainer_mod_st) return; - sbusSetReceiveCtx(ctx, drv); - if (drv && drv->setIdleCb) - drv->setIdleCb(ctx, sbusFrameReceived, nullptr); + auto drv = modulePortGetSerialDrv(sbus_trainer_mod_st->rx); + auto ctx = modulePortGetCtx(sbus_trainer_mod_st->rx); - // switch ON - modulePortSetPower(EXTERNAL_MODULE, true); + if (!sbusTrainerAcquire(ctx, drv)) { + modulePortDeInit(sbus_trainer_mod_st); + sbus_trainer_mod_st = nullptr; + return; } + + // switch ON + modulePortSetPower(EXTERNAL_MODULE, true); } static void trainer_stop_module_sbus() { if (!sbus_trainer_mod_st) return; + + // release before modulePortDeInit() invalidates the driver context + sbusTrainerRelease(); + modulePortDeInit(sbus_trainer_mod_st); modulePortSetPower(EXTERNAL_MODULE,false); sbus_trainer_mod_st = nullptr;