Fix CRSF telemetry corruption from PR #11025 #11189

sensei-hacker · 2025-12-18T18:28:21Z

Summary

This PR fixes the bugs that caused PR #11025 to be immediately reverted (PR #11139) on November 28, 2025. The original implementation corrupted CRSF telemetry streams by emitting malformed frames when sensors were unavailable, breaking all telemetry (not just the new frames).

Root Cause

PR #11025 scheduled telemetry frames unconditionally (compile-time #ifdef only) but frame functions only wrote data when sensors were available (runtime checks). This resulted in frames containing only [SYNC][CRC] instead of proper [SYNC][LENGTH][TYPE][PAYLOAD][CRC], corrupting the CRSF protocol stream.

Impact: Receivers lost sync, causing all telemetry to stop working (GPS, battery, altitude, vspeed, ESC sensors).

Changes

1. RPM Frame

Fixed: Added conditional scheduling - only schedule if ESC_SENSOR_ENABLED && motorCount > 0
Impact: Prevents malformed frames when ESC sensors disabled
Location: src/main/telemetry/crsf.c:705-707

2. Temperature Frame

Fixed: Added conditional scheduling with hasTemperatureSources check
Impact: Prevents malformed frames when no temperature sensors available
Location: src/main/telemetry/crsf.c:709-732

3. Buffer Overflow Protection

Fixed: Added MAX_CRSF_TEMPS constant and bounds checking in loops
Impact: Prevents array overflow if >20 temperature sources configured
Location: src/main/telemetry/crsf.c:361, 368, 376

4. Protocol Limit Enforcement (Bug #5)

Fixed: Added MAX_CRSF_RPM_VALUES (19) and clamping logic
Impact: Enforces CRSF protocol specification (1-19 RPM values max)
Location: src/main/telemetry/crsf.c:337, 342-344

Implementation Pattern

Follows the GPS frame pattern:

✅ Conditional scheduling - only schedule if sensor available
✅ Unconditional writing - always write complete frame data when called

This ensures frames are only sent when valid data is available.

Testing

✅ Code compiles successfully (verified with SITL build)
✅ Comprehensive test script created: claude/test_tools/inav/crsf/test_pr11025_fix.sh
✅ Test validates all 5 bug fixes
✅ Test fails on buggy code (f8dc0bc): 3 bugs detected
✅ Test passes on fixed code (e5bfe79): 0 bugs detected

Related PRs

Original PR: [crsf] add temperature, RPM and AirSpeed telemetry #11025 (merged Nov 28, 2025)
Revert PR: Revert "[crsf] add temperature, RPM and AirSpeed telemetry" #11139 (same day - immediate revert)
Investigation: claude/developer/sent/pr11025-root-cause-analysis.md

Files Changed

src/main/telemetry/crsf.c - 34 insertions, 5 deletions

Additional Notes

This fix re-enables the valuable telemetry features from PR #11025 (airspeed, RPM, temperature) while ensuring they don't corrupt the CRSF stream when sensors are unavailable.

This commit fixes the critical bugs that caused PR iNavFlight#11025 to be reverted (PR iNavFlight#11139). The original implementation caused telemetry stream corruption by emitting malformed frames when sensors were unavailable. Root Cause: The original PR scheduled telemetry frames unconditionally (if feature compiled in) but frame functions only wrote data when sensors were available. This resulted in frames containing only [SYNC][CRC] instead of the proper [SYNC][LENGTH][TYPE][PAYLOAD][CRC] structure, corrupting the CRSF protocol stream and breaking ALL telemetry (not just new frames). Fixes Implemented: 1. RPM Frame (Bug #1 - CRITICAL): - Added conditional scheduling: only schedule if ESC_SENSOR_ENABLED and motorCount > 0 - Added protocol limit enforcement: max 19 RPM values per CRSF spec - Location: src/main/telemetry/crsf.c:705-707, 337-343 2. Temperature Frame (Bug #2 - CRITICAL): - Added conditional scheduling: only schedule if temperature sources are actually available (ESC sensors OR temperature sensors) - Added bounds checking: prevent buffer overflow beyond 20 temperatures - Location: src/main/telemetry/crsf.c:709-732, 361-381 3. Buffer Overflow Protection (Bug #4): - Added MAX_CRSF_TEMPS constant and bounds checks in loops - Prevents array overflow if >20 temperature sources configured - Location: src/main/telemetry/crsf.c:361, 368, 376 4. Protocol Limit Enforcement (Bug #5): - Added MAX_CRSF_RPM_VALUES constant (19 per CRSF spec) - Clamp motorCount to protocol limit before sending - Location: src/main/telemetry/crsf.c:337, 342-344 Implementation Pattern: Follows the GPS frame pattern: ✓ Conditional scheduling - only schedule if sensor available ✓ Unconditional writing - always write complete frame data when called Testing: - Code compiles successfully (verified with SITL build) - No syntax errors or warnings - All fixes follow existing code patterns in crsf.c Related Issues: - Original PR: iNavFlight#11025 (merged Nov 28, 2025, reverted same day) - Revert PR: iNavFlight#11139 - Investigation: claude/developer/sent/pr11025-root-cause-analysis.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

qodo-code-review · 2025-12-18T18:29:32Z

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

qodo-code-review · 2025-12-18T18:31:32Z

src/main/telemetry/crsf.c

+#ifdef USE_ESC_SENSOR
+    if (currentSchedule & BV(CRSF_FRAME_RPM_INDEX)) {
+        crsfInitializeFrame(dst);
+        crsfRpm(dst);
+        crsfFinalize(dst);
+    }
+#endif


Suggestion: In processCrsf, check the return value of crsfRpm and only call crsfFinalize if data was successfully written to the buffer. [possible issue, importance: 7]

Suggested change

#ifdef USE_ESC_SENSOR

if (currentSchedule & BV(CRSF_FRAME_RPM_INDEX)) {

crsfInitializeFrame(dst);

crsfRpm(dst);

crsfFinalize(dst);

}

#endif

#ifdef USE_ESC_SENSOR

if (currentSchedule & BV(CRSF_FRAME_RPM_INDEX)) {

crsfInitializeFrame(dst);

if (crsfRpm(dst)) {

crsfFinalize(dst);

}

}

#endif

qodo-code-review · 2025-12-18T18:31:32Z

src/main/telemetry/crsf.c

+    sbufWriteU8(dst, CRSF_FRAME_AIRSPEED_PAYLOAD_SIZE + CRSF_FRAME_LENGTH_TYPE_CRC);
+    crsfSerialize8(dst, CRSF_FRAMETYPE_AIRSPEED_SENSOR);
+    crsfSerialize16(dst, (uint16_t)(getAirspeedEstimate() * 36 / 100));


Suggestion: Fix the airspeed calculation in crsfFrameAirSpeedSensor by using floating-point division (e.g., 36.0f / 100.0f) to prevent the expression from evaluating to zero. [possible issue, importance: 8]

Suggested change

sbufWriteU8(dst, CRSF_FRAME_AIRSPEED_PAYLOAD_SIZE + CRSF_FRAME_LENGTH_TYPE_CRC);

crsfSerialize8(dst, CRSF_FRAMETYPE_AIRSPEED_SENSOR);

crsfSerialize16(dst, (uint16_t)(getAirspeedEstimate() * 36 / 100));

sbufWriteU8(dst, CRSF_FRAME_AIRSPEED_PAYLOAD_SIZE + CRSF_FRAME_LENGTH_TYPE_CRC);

crsfSerialize8(dst, CRSF_FRAMETYPE_AIRSPEED_SENSOR);

crsfSerialize16(dst, (uint16_t)(getAirspeedEstimate() * 36.0f / 100.0f));

src/main/telemetry/crsf.c

qodo-code-review · 2025-12-18T18:31:33Z

src/main/telemetry/crsf.c

+        sbufWriteU8(dst, 1 + (motorCount * 3) + CRSF_FRAME_LENGTH_TYPE_CRC);
+        crsfSerialize8(dst, CRSF_FRAMETYPE_RPM);
+        // 0 = FC including all ESCs
+        crsfSerialize8(dst, 0);
+
+        for (uint8_t i = 0; i < motorCount; i++) {
+            const escSensorData_t *escState = getEscTelemetry(i);
+            crsfSerialize24(dst, (escState) ? escState->rpm : 0);
+        }


Suggestion: Before writing variable-length frames, validate the computed payload/frame length against CRSF_FRAME_SIZE_MAX/CRSF_PAYLOAD_SIZE_MAX (and remaining sbuf capacity) and clamp counts or abort if it won’t fit. [Learned best practice, importance: 5]

Suggested change

sbufWriteU8(dst, 1 + (motorCount * 3) + CRSF_FRAME_LENGTH_TYPE_CRC);

crsfSerialize8(dst, CRSF_FRAMETYPE_RPM);

// 0 = FC including all ESCs

crsfSerialize8(dst, 0);

for (uint8_t i = 0; i < motorCount; i++) {

const escSensorData_t *escState = getEscTelemetry(i);

crsfSerialize24(dst, (escState) ? escState->rpm : 0);

}

const uint8_t payloadLen = 1 + (motorCount * 3); // source_id + int24 * N

const uint8_t frameLen = payloadLen + CRSF_FRAME_LENGTH_TYPE_CRC;

if ((payloadLen > CRSF_PAYLOAD_SIZE_MAX) || (frameLen > CRSF_FRAME_SIZE_MAX) || (sbufBytesRemaining(dst) < (1 + frameLen))) {

return false;

}

sbufWriteU8(dst, frameLen);

crsfSerialize8(dst, CRSF_FRAMETYPE_RPM);

crsfSerialize8(dst, 0);

for (uint8_t i = 0; i < motorCount; i++) {

const escSensorData_t *escState = getEscTelemetry(i);

crsfSerialize24(dst, (escState) ? escState->rpm : 0);

}

sensei-hacker · 2025-12-19T08:22:11Z

I did not test this against either EdgeTx or OpenTX, only against my own parser.

It needs to be tested with OpenTX and EdgeTX.

@error414 if you want to do some testing, that would be great.

error414 · 2025-12-19T08:47:51Z

OOk, I will test it over weekend. I will have to create fake sensors, I will test my code as well.

BTW: in my PR i Use dynamic caluclating of number of temp/rpm sensors

const uint8_t MAX_CRSF_TEMPS = 20;
vs
const uint8_t MAX_CRSF_TEMPS = CRSF_PAYLOAD_SIZE_MAX / 3;

Jetrell · 2025-12-19T11:48:47Z

I only have my old X9D+ test radio with me... So a ran some tests with it using OpenTX and TBS CRSF.
When Discover Sensors was enabled, 0x0C and 0x0D were not detected, even when the OSD was clearly showing the ESC telemetry RPM and Temperature.
I made a custom sensor just in case. But that too failed.

There was talk on the BF channels about RPM appearing as Vspd.. But its data did not correspond to RPM. Its output was typical of vertical climb speed.

gismo2004 · 2025-12-19T13:37:57Z

@sensei-hacker thank you for taking that further. I am on a business trip so no way to test things, should be possible next week though. I had a different approach (gismo2004@50f1e82) to not send the frame if there is no data available, but was not able to test it myself and didn't want to push this if I haven't tested myself. :-)

@Jetrell if I am not wrong you will not get the data shown on OpenTX as it is missing its implementation. At least there was a commit for EdgeTx to make that sensors available.

Jetrell · 2025-12-19T22:01:35Z

if I am not wrong you will not get the data shown on OpenTX as it is missing its implementation. At least there was a commit for EdgeTx to make that sensors available.

I don't have an EdgeTX radio with me. So I can't test that for a while. But having it not work with OpenTX is okay.
I'll be pleased if this prevents the CRSF telemetry stream from becoming corrupted.

error414 · 2025-12-19T22:15:40Z

I dont have any device with ESC telemetry, so I used fake sensors.

Transmitter: Radiomaster boxer, EdgeTx version from this year

#define DUMMY_ESC_COUNT 3
escSensorData_t NOINLINE * getEscTelemetry(uint8_t esc)
{
    static escSensorData_t dummyEscData[DUMMY_ESC_COUNT];
    static uint32_t counter = 0;

    counter++;

    if (esc >= DUMMY_ESC_COUNT) {
        return NULL;
    }

    // Create per-motor dummy data
    dummyEscData[esc].dataAge = 0;
    dummyEscData[esc].temperature = 40 + esc * 2 + (counter % 5);   // Slight per-motor offset
    dummyEscData[esc].voltage = 240;                                // 24.0V
    dummyEscData[esc].current = 400 + esc * 100;                    // Motor-specific load
    dummyEscData[esc].rpm = 9000 + esc * 1000 + (counter % 500);     // Different RPMs

    return &dummyEscData[esc];
}

it works for me
https://www.youtube.com/watch?v=qTnbQiuvyuk

gismo2004 and others added 5 commits December 18, 2025 12:07

[crsf] add temperature and RPM telemetry

5a2a0b8

make the compiler happy

710b36e

more compiler warnings

b39cc96

add AirSpeed

f8dc0bc

qodo-code-review bot added the Review effort 3/5 label Dec 18, 2025

qodo-code-review bot reviewed Dec 18, 2025

View reviewed changes

sensei-hacker mentioned this pull request Dec 18, 2025

[crsf] add temperature, RPM and AirSpeed telemetry #11025

Merged

error414 mentioned this pull request Dec 19, 2025

Refactor CRSF and SmartPort telemetry #11142

Draft

4 tasks

sensei-hacker added the Testing Required label Dec 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix CRSF telemetry corruption from PR #11025 #11189

Fix CRSF telemetry corruption from PR #11025 #11189

sensei-hacker commented Dec 18, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Dec 18, 2025

Uh oh!

qodo-code-review bot Dec 18, 2025

Uh oh!

qodo-code-review bot Dec 18, 2025

Uh oh!

Uh oh!

qodo-code-review bot Dec 18, 2025

Uh oh!

sensei-hacker commented Dec 19, 2025 •

edited

Loading

Uh oh!

error414 commented Dec 19, 2025

Uh oh!

Jetrell commented Dec 19, 2025

Uh oh!

gismo2004 commented Dec 19, 2025

Uh oh!

Jetrell commented Dec 19, 2025

Uh oh!

error414 commented Dec 19, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix CRSF telemetry corruption from PR #11025 #11189

Are you sure you want to change the base?

Fix CRSF telemetry corruption from PR #11025 #11189

Conversation

sensei-hacker commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Changes

1. RPM Frame

2. Temperature Frame

3. Buffer Overflow Protection

4. Protocol Limit Enforcement (Bug #5)

Implementation Pattern

Testing

Related PRs

Files Changed

Additional Notes

Uh oh!

qodo-code-review bot commented Dec 18, 2025

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

qodo-code-review bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qodo-code-review bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

sensei-hacker commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

error414 commented Dec 19, 2025

Uh oh!

Jetrell commented Dec 19, 2025

Uh oh!

gismo2004 commented Dec 19, 2025

Uh oh!

Jetrell commented Dec 19, 2025

Uh oh!

error414 commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sensei-hacker commented Dec 18, 2025 •

edited

Loading

sensei-hacker commented Dec 19, 2025 •

edited

Loading

error414 commented Dec 19, 2025 •

edited

Loading