From 9d6d63ee3c9e80cca89d08423147630b3021c88d Mon Sep 17 00:00:00 2001 From: nerdCopter <56646290+nerdCopter@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:34:47 -0500 Subject: [PATCH] fix(drivers): resolve serial_escserial.c TX-pin, HAL-timer, and RX ownership defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports BF 4.5-maintenance's tag-based timerAllocate() ownership check onto openEscSerial()'s RX timer resolution (IT #1408), which was still using a raw timerHardware[] array index. Fixes four bugs found in the same file during that port, confirmed on real HELIOSPRING (F4) and STM32F7X2/FOXEERF722V4 (F7/HAL) hardware: - escprog silently did nothing when no dedicated ESCSERIAL TX pin was configured (82/228 USE_ESCSERIAL targets). Ports BF master ae975492a's fallback to the RX motor pin for both directions. - H7 never got the timer alternate-function config in escSerialGPIOConfig() (guard was STM32F7-only); escserial RX was structurally dead on H7 targets. - TIM_DeInit() was a no-op stub on HAL builds; the motor timer was never reset before reuse and stayed running at the escserial timebase after passthrough exit. - configTimeBase()'s "already configured" guard silently skipped the period/prescaler rewrite when escserial repurposed an already-initialized timer on HAL builds. Ports BF's timerReconfigureTimeBase() to bypass it. escEnablePassthrough() now returns bool and threads motorConfig through, matching BF; the CLI caller reports failure instead of failing silently. The legacy first_output-scan-and-add offset is removed — both CLI and MSP callers already pass a plain, bounds-checked motor index. A related motor-timer ownership conflict was found but not fixed here (no release mechanism exists in EF or BF for any STM32 target): tracked as IT #1426, alongside the four fixes above. Fixes IT #1408. Co-Authored-By: Claude Sonnet 5 --- src/main/drivers/serial_escserial.c | 104 ++++++++++++++++------------ src/main/drivers/serial_escserial.h | 3 +- src/main/drivers/timer.c | 4 ++ src/main/drivers/timer.h | 1 + src/main/drivers/timer_hal.c | 10 +++ src/main/interface/cli.c | 4 +- src/main/interface/msp.c | 2 +- 7 files changed, 81 insertions(+), 47 deletions(-) diff --git a/src/main/drivers/serial_escserial.c b/src/main/drivers/serial_escserial.c index 436b4d380b..2dc3691172 100644 --- a/src/main/drivers/serial_escserial.c +++ b/src/main/drivers/serial_escserial.c @@ -135,10 +135,9 @@ enum { #define STOP_BIT_MASK (1 << 0) #define START_BIT_MASK (1 << (RX_TOTAL_BITS - 1)) -// XXX No TIM_DeInit equivalent in HAL driver??? #ifdef USE_HAL_DRIVER static void TIM_DeInit(TIM_TypeDef *tim) { - UNUSED(tim); + LL_TIM_DeInit(tim); } #endif @@ -173,7 +172,7 @@ static void escSerialGPIOConfig(const timerHardware_t *timhw, ioConfig_t cfg) { return; } IOInit(IOGetByTag(tag), OWNER_MOTOR, 0); -#ifdef STM32F7 +#if defined(STM32F7) || defined(STM32H7) IOConfigGPIOAF(IOGetByTag(tag), cfg, timhw->alternateFunction); #else IOConfigGPIO(IOGetByTag(tag), cfg); @@ -322,6 +321,7 @@ static void serialTimerTxConfigBL(const timerHardware_t *timerHardwarePtr, uint8 } } } while (isTimerPeriodTooLarge(timerPeriod)); + timerReconfigureTimeBase(timerHardwarePtr->tim, timerPeriod, clock); timerConfigure(timerHardwarePtr, timerPeriod, clock); timerChCCHandlerInit(&escSerialPorts[reference].timerCb, onSerialTimerBL); timerChConfigCallbacks(timerHardwarePtr, &escSerialPorts[reference].timerCb, NULL); @@ -370,6 +370,7 @@ static void onSerialRxPinChangeBL(timerCCHandlerRec_t *cbRec, captureCompare_t c static void serialTimerRxConfigBL(const timerHardware_t *timerHardwarePtr, uint8_t reference, portOptions_e options) { // start bit is usually a FALLING signal TIM_DeInit(timerHardwarePtr->tim); + timerReconfigureTimeBase(timerHardwarePtr->tim, 0xFFFF, SystemCoreClock / 2); timerConfigure(timerHardwarePtr, 0xFFFF, SystemCoreClock / 2); timerChConfigIC(timerHardwarePtr, (options & SERIAL_INVERTED) ? ICPOLARITY_RISING : ICPOLARITY_FALLING, 0); timerChCCHandlerInit(&escSerialPorts[reference].edgeCb, onSerialRxPinChangeBL); @@ -467,6 +468,7 @@ static void onSerialTimerEsc(timerCCHandlerRec_t *cbRec, captureCompare_t captur static void escSerialTimerTxConfig(const timerHardware_t *timerHardwarePtr, uint8_t reference) { uint32_t timerPeriod = 34; TIM_DeInit(timerHardwarePtr->tim); + timerReconfigureTimeBase(timerHardwarePtr->tim, timerPeriod, MHZ_TO_HZ(1)); timerConfigure(timerHardwarePtr, timerPeriod, MHZ_TO_HZ(1)); timerChCCHandlerInit(&escSerialPorts[reference].timerCb, onSerialTimerEsc); timerChConfigCallbacks(timerHardwarePtr, &escSerialPorts[reference].timerCb, NULL); @@ -535,6 +537,7 @@ static void onSerialRxPinChangeEsc(timerCCHandlerRec_t *cbRec, captureCompare_t static void escSerialTimerRxConfig(const timerHardware_t *timerHardwarePtr, uint8_t reference) { // start bit is usually a FALLING signal TIM_DeInit(timerHardwarePtr->tim); + timerReconfigureTimeBase(timerHardwarePtr->tim, 0xFFFF, MHZ_TO_HZ(1)); timerConfigure(timerHardwarePtr, 0xFFFF, MHZ_TO_HZ(1)); timerChConfigIC(timerHardwarePtr, ICPOLARITY_FALLING, 0); timerChCCHandlerInit(&escSerialPorts[reference].edgeCb, onSerialRxPinChangeEsc); @@ -552,10 +555,15 @@ static void resetBuffers(escSerial_t *escSerial) { escSerial->port.txBufferHead = 0; } -static serialPort_t *openEscSerial(escSerialPortIndex_e portIndex, serialReceiveCallbackPtr callback, uint16_t output, uint32_t baud, portOptions_e options, uint8_t mode) { +static serialPort_t *openEscSerial(const motorDevConfig_t *motorConfig, escSerialPortIndex_e portIndex, serialReceiveCallbackPtr callback, uint16_t output, uint32_t baud, portOptions_e options, uint8_t mode) { escSerial_t *escSerial = &(escSerialPorts[portIndex]); if (mode != PROTOCOL_KISSALL) { - escSerial->rxTimerHardware = &(timerHardware[output]); + const ioTag_t tag = motorConfig->ioTags[output]; + const timerHardware_t *timerHardware = timerAllocate(tag, OWNER_MOTOR, 0); + if (timerHardware == NULL) { + return NULL; + } + escSerial->rxTimerHardware = timerHardware; // N-Channels can't be used as RX. if (escSerial->rxTimerHardware->output & TIMER_OUTPUT_N_CHANNEL) { return NULL; @@ -563,15 +571,29 @@ static serialPort_t *openEscSerial(escSerialPortIndex_e portIndex, serialReceive #ifdef USE_HAL_DRIVER escSerial->rxTimerHandle = timerFindTimerHandle(escSerial->rxTimerHardware->tim); #endif - } - escSerial->mode = mode; - escSerial->txTimerHardware = timerAllocate(escSerialConfig()->ioTag, OWNER_MOTOR, 0); - if (escSerial->txTimerHardware == NULL) { - return NULL; - } + // Workaround to ensure that the timerHandle is configured before use, timer will be reconfigured to a different frequency below + // this prevents a null-pointer dereference in __HAL_TIM_CLEAR_FLAG called by timerChClearCCFlag and similar accesses of timerHandle without the Instance being configured first. + timerConfigure(escSerial->rxTimerHardware, 0xffff, 1); + + // Allocate separate TX pin if configured, otherwise use RX pin for both TX and RX + if (escSerialConfig()->ioTag != IO_TAG_NONE) { + escSerial->txTimerHardware = timerAllocate(escSerialConfig()->ioTag, OWNER_MOTOR, 0); + if (escSerial->txTimerHardware == NULL) { + return NULL; + } #ifdef USE_HAL_DRIVER - escSerial->txTimerHandle = timerFindTimerHandle(escSerial->txTimerHardware->tim); + escSerial->txTimerHandle = timerFindTimerHandle(escSerial->txTimerHardware->tim); #endif + timerConfigure(escSerial->txTimerHardware, 0xffff, 1); + } else { + // No separate TX pin configured - use RX pin for both TX and RX (fallback mode) + escSerial->txTimerHardware = escSerial->rxTimerHardware; +#ifdef USE_HAL_DRIVER + escSerial->txTimerHandle = escSerial->rxTimerHandle; +#endif + } + } + escSerial->mode = mode; escSerial->port.vTable = escSerialVTable; escSerial->port.baudRate = baud; escSerial->port.mode = MODE_RXTX; @@ -605,22 +627,28 @@ static serialPort_t *openEscSerial(escSerialPortIndex_e portIndex, serialReceive memset(&escOutputs, 0, sizeof(escOutputs)); pwmOutputPort_t *pwmMotors = pwmGetMotors(); for (volatile uint8_t i = 0; i < MAX_SUPPORTED_MOTORS; i++) { - if (pwmMotors[i].enabled) { - if (pwmMotors[i].io != IO_NONE) { - for (volatile uint8_t j = 0; j < USABLE_TIMER_CHANNEL_COUNT; j++) { - if (pwmMotors[i].io == IOGetByTag(timerHardware[j].tag)) { - escSerialOutputPortConfig(&timerHardware[j]); - if (timerHardware[j].output & TIMER_OUTPUT_INVERTED) { - escOutputs[escSerial->outputCount].inverted = 1; - } - break; + if (pwmMotors[i].enabled && pwmMotors[i].io != IO_NONE) { + const ioTag_t tag = motorConfig->ioTags[i]; + if (tag != IO_TAG_NONE) { + const timerHardware_t *timerHardware = timerAllocate(tag, OWNER_MOTOR, 0); + if (timerHardware) { + // Workaround to ensure that the timerHandle is configured before use, timer will be reconfigured to a different frequency below + // this prevents a null-pointer dereference in __HAL_TIM_CLEAR_FLAG called by timerChClearCCFlag and similar accesses of timerHandle without the Instance being configured first. + timerConfigure(timerHardware, 0xffff, 1); + escSerialOutputPortConfig(timerHardware); + escOutputs[escSerial->outputCount].io = pwmMotors[i].io; + if (timerHardware->output & TIMER_OUTPUT_INVERTED) { + escOutputs[escSerial->outputCount].inverted = 1; } + escSerial->txTimerHardware = timerHardware; + escSerial->outputCount++; } - escOutputs[escSerial->outputCount].io = pwmMotors[i].io; - escSerial->outputCount++; } } } + if (escSerial->outputCount == 0) { + return NULL; + } setTxSignalEsc(escSerial, ENABLE); serialTimerTxConfigBL(escSerial->txTimerHardware, portIndex, baud); } else if (mode == PROTOCOL_CASTLE) { @@ -780,9 +808,9 @@ static bool processExitCommand(uint8_t c) { } -void escEnablePassthrough(serialPort_t *escPassthroughPort, uint16_t output, uint8_t mode) { +bool escEnablePassthrough(serialPort_t *escPassthroughPort, const motorDevConfig_t *motorConfig, uint16_t output, uint8_t mode) { bool exitEsc = false; - uint8_t motor_output = 0; + uint8_t motor_output = output; LED0_OFF; LED1_OFF; //StopPwmAllMotors(); @@ -800,26 +828,14 @@ void escEnablePassthrough(serialPort_t *escPassthroughPort, uint16_t output, uin escBaudrate = BAUDRATE_NORMAL; break; } - if ((mode == PROTOCOL_KISS) && (output == 255)) { - motor_output = 255; + if ((mode == PROTOCOL_KISS) && (motor_output == 255)) { mode = PROTOCOL_KISSALL; - } else { - uint8_t first_output = 0; - for (unsigned i = 0; i < USABLE_TIMER_CHANNEL_COUNT; i++) { - if (timerHardware[i].usageFlags & TIM_USE_MOTOR) { - first_output = i; - break; - } - } - //doesn't work with messy timertable - motor_output = first_output + output; - if (motor_output >= USABLE_TIMER_CHANNEL_COUNT) { - return; - } + } else if (motor_output >= MAX_SUPPORTED_MOTORS) { + return false; } - escPort = openEscSerial(ESCSERIAL1, NULL, motor_output, escBaudrate, 0, mode); + escPort = openEscSerial(motorConfig, ESCSERIAL1, NULL, motor_output, escBaudrate, 0, mode); if (!escPort) { - return; + return false; } uint8_t ch; while (1) { @@ -846,9 +862,9 @@ void escEnablePassthrough(serialPort_t *escPassthroughPort, uint16_t output, uin serialWrite(escPassthroughPort, 0xF4); serialWrite(escPassthroughPort, 0xF4); closeEscSerial(ESCSERIAL1, mode); - return; + return true; } - if (mode == PROTOCOL_BLHELI) { + if (mode == PROTOCOL_BLHELI || mode == PROTOCOL_KISS || mode == PROTOCOL_KISSALL) { serialWrite(escPassthroughPort, ch); // blheli loopback } serialWrite(escPort, ch); diff --git a/src/main/drivers/serial_escserial.h b/src/main/drivers/serial_escserial.h index 16ae567d6a..6a6f713d69 100644 --- a/src/main/drivers/serial_escserial.h +++ b/src/main/drivers/serial_escserial.h @@ -37,7 +37,8 @@ typedef enum { } escProtocol_e; // serialPort API -void escEnablePassthrough(serialPort_t *escPassthroughPort, uint16_t output, uint8_t mode); +struct motorDevConfig_s; +bool escEnablePassthrough(serialPort_t *escPassthroughPort, const struct motorDevConfig_s *motorConfig, uint16_t output, uint8_t mode); typedef struct escSerialConfig_s { ioTag_t ioTag; diff --git a/src/main/drivers/timer.c b/src/main/drivers/timer.c index 3f2b7a5cb9..6664b39506 100644 --- a/src/main/drivers/timer.c +++ b/src/main/drivers/timer.c @@ -309,6 +309,10 @@ void configTimeBase(TIM_TypeDef *tim, uint16_t period, uint32_t hz) { TIM_TimeBaseInit(tim, &TIM_TimeBaseStructure); } +void timerReconfigureTimeBase(TIM_TypeDef *tim, uint16_t period, uint32_t hz) { + configTimeBase(tim, period, hz); +} + // old interface for PWM inputs. It should be replaced void timerConfigure(const timerHardware_t *timerHardwarePtr, uint16_t period, uint32_t hz) { configTimeBase(timerHardwarePtr->tim, period, hz); diff --git a/src/main/drivers/timer.h b/src/main/drivers/timer.h index 406cb88d11..4de5827f62 100644 --- a/src/main/drivers/timer.h +++ b/src/main/drivers/timer.h @@ -152,6 +152,7 @@ typedef enum { } channelType_t; void timerConfigure(const timerHardware_t *timHw, uint16_t period, uint32_t hz); // This interface should be replaced. +void timerReconfigureTimeBase(TIM_TypeDef *tim, uint16_t period, uint32_t hz); void timerChConfigIC(const timerHardware_t *timHw, bool polarityRising, unsigned inputFilterSamples); void timerChConfigICDual(const timerHardware_t* timHw, bool polarityRising, unsigned inputFilterSamples); diff --git a/src/main/drivers/timer_hal.c b/src/main/drivers/timer_hal.c index 1acf0e02c9..4610962a2c 100644 --- a/src/main/drivers/timer_hal.c +++ b/src/main/drivers/timer_hal.c @@ -344,6 +344,16 @@ void configTimeBase(TIM_TypeDef *tim, uint16_t period, uint32_t hz) { } } +void timerReconfigureTimeBase(TIM_TypeDef *tim, uint16_t period, uint32_t hz) { + TIM_HandleTypeDef *handle = timerFindTimerHandle(tim); + if (handle == NULL) { + return; + } + handle->Init.Period = (period - 1) & 0xffff; // AKA TIMx_ARR + handle->Init.Prescaler = (timerClock(tim) / hz) - 1; + TIM_Base_SetConfig(handle->Instance, &handle->Init); +} + // old interface for PWM inputs. It should be replaced void timerConfigure(const timerHardware_t *timerHardwarePtr, uint16_t period, uint32_t hz) { uint8_t timerIndex = lookupTimerIndex(timerHardwarePtr->tim); diff --git a/src/main/interface/cli.c b/src/main/interface/cli.c index e04a5be177..645c0993ee 100644 --- a/src/main/interface/cli.c +++ b/src/main/interface/cli.c @@ -2946,7 +2946,9 @@ static void cliEscPassthrough(char *cmdline) { pos++; pch = strtok_r(NULL, " ", &saveptr); } - escEnablePassthrough(cliPort, escIndex, mode); + if (!escEnablePassthrough(cliPort, &motorConfig()->dev, escIndex, mode)) { + cliPrintErrorLinef("Error starting ESC connection"); + } } #endif diff --git a/src/main/interface/msp.c b/src/main/interface/msp.c index a7fffe9974..971b071a99 100644 --- a/src/main/interface/msp.c +++ b/src/main/interface/msp.c @@ -211,7 +211,7 @@ uint8_t escPortIndex; #ifdef USE_ESCSERIAL static void mspEscPassthroughFn(serialPort_t *serialPort) { - escEnablePassthrough(serialPort, escPortIndex, escMode); + escEnablePassthrough(serialPort, &motorConfig()->dev, escPortIndex, escMode); } #endif