Conversation
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
Review Summary by QodoAdd bidirectional DShot telemetry and configurable RC channel mirroring
WalkthroughsDescription• Add experimental bidirectional DShot telemetry support for motor RPM feedback - Implements GCR decoding and eRPM to RPM conversion - Supports up to 4 motors on TIM4 with DMA or interrupt capture - Integrates with RPM filter and telemetry systems • Add configurable RC channel mirroring to preserve original RX values before MSP override - Four copy rules allow routing pre-override channels to spare destinations - Enables external controllers to read both overridden and original stick data • Refactor RPM source abstraction to support multiple telemetry backends - New rpm_source module unifies ESC sensor and DShot bidir RPM access - Replaces direct ESC sensor calls throughout codebase • Enable DShot bidir on SpeedyBee F405 V3 target with validation Diagramflowchart LR
RX["RX Channels"] -->|store original| ORIG["Original Channels"]
RX -->|apply override| MSP["MSP Override"]
ORIG -->|copy rules| MIRROR["Mirrored Channels"]
DSHOT["DShot Motors"] -->|capture telemetry| BIDIR["Bidir Decoder"]
BIDIR -->|decode eRPM| RPM_SRC["RPM Source"]
ESC["ESC Sensor"] -->|fallback| RPM_SRC
RPM_SRC -->|provide RPM| FILTER["RPM Filter"]
RPM_SRC -->|provide RPM| TELEM["Telemetry"]
File Changes1. src/main/drivers/dshot_bidir.c
|
Code Review by Qodo
1. dshotBidirDecodeTelemetryPacket() OOB read
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| for (uint32_t i = 1; i <= count; i++) { | ||
| if (i < count) { | ||
| const int diff = buffer[i] - oldValue; | ||
| if (bits >= 21) { | ||
| break; | ||
| } | ||
| len = (diff + 8) / 16; | ||
| } else { | ||
| len = 21 - bits; | ||
| } | ||
|
|
||
| if (len <= 0 || (bits + len) > 21) { | ||
| return DSHOT_BIDIR_INVALID; | ||
| } | ||
|
|
||
| value <<= len; | ||
| value |= 1 << (len - 1); | ||
| oldValue = buffer[i]; | ||
| bits += len; |
There was a problem hiding this comment.
1. dshotbidirdecodetelemetrypacket() oob read 📘 Rule violation ⛨ Security
dshotBidirDecodeTelemetryPacket() unconditionally reads buffer[count] in the final loop iteration (i == count), which can go past captureBuffer when `count == DSHOT_BIDIR_CAPTURE_BUFFER_LEN`. This violates the requirement to length-check protocol/payload buffers before reads and can cause undefined behavior/crashes.
Agent Prompt
## Issue description
`dshotBidirDecodeTelemetryPacket()` can read past the end of `buffer` because the loop runs with `i <= count` and still reads `buffer[i]` when `i == count`.
## Issue Context
`dshotBidirDecodeCapture()` passes `edgeCount` from `dshotBidirGetCapturedEdgeCount()`, which can be equal to `DSHOT_BIDIR_CAPTURE_BUFFER_LEN`, making `buffer[count]` out-of-bounds.
## Fix Focus Areas
- src/main/drivers/dshot_bidir.c[143-168]
- src/main/drivers/dshot_bidir.c[451-466]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #include <string.h> | ||
|
|
||
| #include "platform.h" | ||
|
|
||
| #ifdef USE_DSHOT_BIDIR | ||
|
|
||
| #include "build/debug.h" | ||
|
|
||
| #include "common/time.h" |
There was a problem hiding this comment.
2. Missing <math.h> for lrintf 📘 Rule violation ⚙ Maintainability
dshot_bidir.c uses lrintf() without explicitly including <math.h>, relying on transitive includes. This can break builds on toolchains where lrintf is not declared unless <math.h> is included.
Agent Prompt
## Issue description
`lrintf()` is used without an explicit `<math.h>` include, which can cause build failures due to missing prototype declarations.
## Issue Context
Compliance requires avoiding reliance on transitive includes for macros/types/functions.
## Fix Focus Areas
- src/main/drivers/dshot_bidir.c[25-43]
- src/main/drivers/dshot_bidir.c[210-219]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #if defined(USE_ESC_SENSOR) | ||
| case OSD_ESC_RPM: | ||
| { | ||
| escSensorData_t * escSensor = escSensorGetData(); | ||
| if (escSensor && escSensor->dataAge <= ESC_DATA_MAX_AGE) { | ||
| osdFormatRpm(buff, escSensor->rpm); | ||
| uint32_t rpm = 0; | ||
| if (rpmSourceGetAverageRpm(&rpm)) { | ||
| osdFormatRpm(buff, rpm); | ||
| } | ||
| else { | ||
| osdFormatRpm(buff, 0); |
There was a problem hiding this comment.
3. Rpm paths still esc_sensor-gated 🐞 Bug ≡ Correctness
RPM consumers (OSD ESC_RPM, Jeti EX_RPM, MSP2_INAV_ESC_RPM/ESC_TELEM) are still compiled under #ifdef USE_ESC_SENSOR, so DShot-bidir-only targets (e.g. SPEEDYBEEF405V3 enables USE_DSHOT_BIDIR but not USE_ESC_SENSOR) won’t expose RPM via these features.
Agent Prompt
## Issue description
Even though `rpm_source` supports DShot-bidir RPM, multiple RPM output features are still gated by `#ifdef USE_ESC_SENSOR`, so builds with `USE_DSHOT_BIDIR` but without `USE_ESC_SENSOR` don’t include these RPM outputs.
## Issue Context
`escSensorData_t` is defined unconditionally (in `sensors/esc_sensor.h`), and RPM retrieval now goes through `rpmSourceGetAverageRpm()` / `rpmSourceGetMotorRpm()`, so these modules can be compiled without `USE_ESC_SENSOR` when DShot bidir is enabled.
## Fix Focus Areas
- src/main/io/osd.c[3924-3936]
- src/main/telemetry/jetiexbus.c[296-300]
- src/main/telemetry/jetiexbus.c[433-443]
- src/main/fc/fc_msp.c[1730-1776]
- src/main/target/SPEEDYBEEF405V3/target.h[149-155]
## Expected fix
Relax the compile-time guards for RPM-related features to include DShot bidir builds (e.g., `#if defined(USE_ESC_SENSOR) || defined(USE_DSHOT_BIDIR)`), and ensure any remaining ESC-temperature-only logic stays under `USE_ESC_SENSOR` where appropriate.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
No description provided.