fix(trainer): SBUS trainer context ownership (#7699)

Co-authored-by: raphaelcoeffic <1050031+raphaelcoeffic@users.noreply.github.com>

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 ||

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)
 {

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);

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) {

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
 //

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);

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);

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;