-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Revert "[crsf] add temperature, RPM and AirSpeed telemetry" #11139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
| CRSF_FRAME_GPS_INDEX, | ||
| CRSF_FRAME_VARIO_SENSOR_INDEX, | ||
| CRSF_FRAME_BAROMETER_ALTITUDE_INDEX, | ||
| CRSF_FRAME_TEMP_INDEX, | ||
| CRSF_FRAME_RPM_INDEX, | ||
| CRSF_FRAME_AIRSPEED_INDEX, | ||
| CRSF_SCHEDULE_COUNT_MAX | ||
| } crsfFrameTypeIndex_e; | ||
|
|
||
| static uint8_t crsfScheduleCount; | ||
| static uint16_t crsfSchedule[CRSF_SCHEDULE_COUNT_MAX]; | ||
| static uint8_t crsfSchedule[CRSF_SCHEDULE_COUNT_MAX]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add a compile-time assert that the highest used index fits in the schedule mask width and update any docs/comments to reflect removed frames to prevent future mismatches. [Learned best practice, importance: 6]
New proposed code:
typedef enum {
CRSF_FRAME_ATTITUDE_INDEX,
CRSF_FRAME_BATTERY_SENSOR_INDEX,
CRSF_FRAME_FLIGHT_MODE_INDEX,
CRSF_FRAME_DEVICE_INFO_INDEX,
CRSF_FRAME_GPS_INDEX,
CRSF_FRAME_VARIO_SENSOR_INDEX,
CRSF_FRAME_BAROMETER_ALTITUDE_INDEX,
CRSF_SCHEDULE_COUNT_MAX
} crsfFrameTypeIndex_e;
static uint8_t crsfScheduleCount;
static uint8_t crsfSchedule[CRSF_SCHEDULE_COUNT_MAX];
+
+_Static_assert(CRSF_SCHEDULE_COUNT_MAX <= 8, "crsfSchedule bitmask exceeds uint8_t width");
...
const uint8_t currentSchedule = crsfSchedule[crsfScheduleIndex];
if (currentSchedule & BV(CRSF_FRAME_ATTITUDE_INDEX)) {
...
}
if (currentSchedule & BV(CRSF_FRAME_BATTERY_SENSOR_INDEX)) {
...
}
if (currentSchedule & BV(CRSF_FRAME_FLIGHT_MODE_INDEX)) {
...
}
...
if (currentSchedule & BV(CRSF_FRAME_VARIO_SENSOR_INDEX)) {
...
}
if (currentSchedule & BV(CRSF_FRAME_BAROMETER_ALTITUDE_INDEX)) {
...
}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>
User description
Reverts PR #11025 which added CRSF temperature, RPM, and AirSpeed telemetry support.
Changes Reverted
src/main/rx/crsf.h: Removed frame type definitions (CRSF_FRAMETYPE_AIRSPEED_SENSOR,CRSF_FRAMETYPE_RPM,CRSF_FRAMETYPE_TEMP) and payload size constantsrc/main/telemetry/crsf.c: RemovedcrsfFrameAirSpeedSensor(),crsfRpm(),crsfTemperature()functions, helpercrsfSerialize24(), and associated frame scheduling logicCreated via
git revert -m 1 acefeca61de5ef01883c14d262688dbd4cd3d298.Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
PR Type
Bug fix
Description
Reverts CRSF telemetry additions that caused regressions
Removes airspeed, RPM, and temperature frame support
Removes associated sensor includes and helper functions
Simplifies frame scheduling logic and reduces payload complexity
Diagram Walkthrough
File Walkthrough
crsf.h
Remove CRSF telemetry frame type definitionssrc/main/rx/crsf.h
CRSF_FRAME_AIRSPEED_PAYLOAD_SIZEconstant definitionCRSF_FRAMETYPE_AIRSPEED_SENSOR,CRSF_FRAMETYPE_RPM,CRSF_FRAMETYPE_TEMPcrsf.c
Remove CRSF telemetry functions and sensor supportsrc/main/telemetry/crsf.c
flight/mixer.h,sensors/esc_sensor.h,sensors/pitotmeter.h,sensors/temperature.hcrsfFrameAirSpeedSensor(),crsfRpm(),crsfTemperature(), and helpercrsfSerialize24()telemetry
crsfSchedulearray fromuint16_ttouint8_ttypeairspeed sensors
initCrsfTelemetry()for ESC andpitot sensors