Skip to content

telemetry: guard CRSF ESC RPM/temp against stale data#11536

Open
sensei-hacker wants to merge 3 commits intoiNavFlight:maintenance-9.xfrom
sensei-hacker:fix-esc-telemetry-random-values
Open

telemetry: guard CRSF ESC RPM/temp against stale data#11536
sensei-hacker wants to merge 3 commits intoiNavFlight:maintenance-9.xfrom
sensei-hacker:fix-esc-telemetry-random-values

Conversation

@sensei-hacker
Copy link
Copy Markdown
Member

Summary

ESC RPM and temperature forwarded over CRSF showed random/garbage values when the FC was disarmed and when individual ESCs lost contact in multi-motor setups. The previous null-guard on getEscTelemetry() was dead code — the function always returns a valid pointer into a static array — so stale last-seen values were unconditionally forwarded regardless of data age.

Changes

  • crsfRpm(): check escState->dataAge < ESC_DATA_INVALID before forwarding RPM; send 0 for stale/uninitialized slots
  • crsfTemperature(): same check before forwarding temperature; send TEMPERATURE_INVALID_VALUE for stale/uninitialized slots

ESC_DATA_INVALID (255) is already the initialization value set by escSensorInitialize() and is the value used consistently throughout esc_sensor.c for this purpose.

Testing

  • SITL build confirmed clean (no warnings)
  • Code analysis confirmed all four bug scenarios from the issue map directly to the missing dataAge guard:
    • Single ESC disarmed → stale last-seen RPM forwarded (now sends 0)
    • Multi-ESC in flight → divergent per-motor dataAge causes mixed fresh/stale values (now stale slots send 0)
    • RPM stays elevated after disarm → dataAge saturates at 255 but was never checked (now checked)
    • Extra CRSF data points on octocopter → all motor slots emitted regardless of age (now filtered)
  • Hardware testing with physical ESCs not performed; requesting confirmation from reporter

Related Issues

Fixes #11517

ESC sensor slots are initialized with dataAge=ESC_DATA_INVALID (255)
and age toward 255 when no frames arrive. The previous null-guard on
getEscTelemetry() was dead code (it always returns a valid pointer),
so stale last-seen RPM and temperature values were forwarded to CRSF
regardless of data age — causing random values on disarm and when
individual ESCs lose contact in multi-ESC setups.

Check dataAge < ESC_DATA_INVALID before forwarding; send 0 /
TEMPERATURE_INVALID_VALUE for stale or uninitialized slots instead.

Fixes iNavFlight#11517
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Guard CRSF ESC telemetry against stale sensor data

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Guard CRSF ESC RPM/temperature against stale data
• Check dataAge < ESC_DATA_INVALID before forwarding values
• Send 0 for stale RPM; send TEMPERATURE_INVALID_VALUE for stale temperature
• Fixes random/garbage values on disarm and multi-motor contact loss
Diagram
flowchart LR
  A["ESC Sensor Data"] --> B["dataAge Check"]
  B -->|Fresh| C["Forward RPM/Temp"]
  B -->|Stale| D["Send Default Values"]
  C --> E["CRSF Frame"]
  D --> E
Loading

Grey Divider

File Changes

1. src/main/telemetry/crsf.c 🐞 Bug fix +2/-2

Add dataAge validation to ESC telemetry functions

• Modified crsfRpm() to check escState->dataAge < ESC_DATA_INVALID before forwarding RPM values
• Modified crsfTemperature() to check escState->dataAge < ESC_DATA_INVALID before forwarding
 temperature values
• Sends 0 for stale RPM slots instead of forwarding last-seen values
• Sends TEMPERATURE_INVALID_VALUE for stale temperature slots with explicit type cast

src/main/telemetry/crsf.c


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 4, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. crsfRpm() uses 0 sentinel 📘 Rule violation ☼ Reliability
Description
When ESC telemetry is stale (dataAge >= ESC_DATA_INVALID), the new code serializes RPM as 0,
which can be interpreted as a valid stopped-motor RPM rather than “no data”. This violates the
requirement to use explicit invalid/no-data sentinels for outputs to avoid ambiguous interpretation.
Code

src/main/telemetry/crsf.c[353]

+            crsfSerialize24(dst, escState->dataAge < ESC_DATA_INVALID ? escState->rpm : 0);
Evidence
PR Compliance ID 5 requires using explicit invalid/no-data sentinels instead of potentially-valid
defaults like zero; the changed line explicitly emits 0 RPM when ESC data is stale.

src/main/telemetry/crsf.c[353-353]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`crsfRpm()` currently emits `0` RPM when `escState->dataAge >= ESC_DATA_INVALID`. `0` can be a valid value (motor stopped), so it is an ambiguous “no data” indicator.
## Issue Context
Compliance requires explicit invalid/no-data sentinels for outputs where possible, instead of potentially-valid defaults like `0`.
## Fix Focus Areas
- src/main/telemetry/crsf.c[351-354]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Stale RPM still forwarded🐞 Bug ≡ Correctness
Description
crsfRpm() considers any ESC telemetry with dataAge < ESC_DATA_INVALID (255) valid, so after an ESC
stops responding CRSF can continue forwarding last-seen RPM until dataAge saturates at 255
(potentially tens of seconds+ with multi-motor polling). This is inconsistent with other
telemetry/UI paths that treat ESC data stale after ESC_DATA_MAX_AGE (10), and can keep showing
incorrect RPM long after disarm/ESC dropout.
Code

src/main/telemetry/crsf.c[353]

+            crsfSerialize24(dst, escState->dataAge < ESC_DATA_INVALID ? escState->rpm : 0);
Evidence
ESC telemetry ages per motor on each request timeout/failure, with a 50ms timeout and round-robin
selection across motors, so reaching ESC_DATA_INVALID (255) can take 255 * 50ms * motorCount. Other
parts of the codebase gate ESC-derived values at ESC_DATA_MAX_AGE (10), indicating that is the
intended freshness threshold for consumers.

src/main/telemetry/crsf.c[351-355]
src/main/sensors/esc_sensor.h[42-45]
src/main/sensors/esc_sensor.c[54-57]
src/main/sensors/esc_sensor.c[97-113]
src/main/sensors/esc_sensor.c[252-289]
src/main/telemetry/sbus2.c[79-88]
src/main/io/osd.c[3920-3929]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`crsfRpm()` currently forwards RPM for any ESC slot with `dataAge < ESC_DATA_INVALID (255)`. That suppresses never-initialized slots, but still forwards very stale last-seen values until `dataAge` saturates at 255, which can take a long time with round-robin polling.
### Issue Context
Other telemetry/UI paths treat ESC telemetry as valid only when `dataAge <= ESC_DATA_MAX_AGE`.
### Fix Focus Areas
- src/main/telemetry/crsf.c[335-358]
### Suggested change
Gate RPM with `escState->dataAge <= ESC_DATA_MAX_AGE` (or an equivalent freshness threshold) instead of `< ESC_DATA_INVALID`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

ESC_DATA_INVALID (255) is too lenient as a freshness threshold —
with 50ms poll cycles it allows stale data to persist 12+ seconds
(single motor) before being suppressed. All other ESC data consumers
(OSD, battery, jetiexbus, sbus2) use ESC_DATA_MAX_AGE (10) as the
freshness cutoff, giving a ~500ms window per motor.

Switch CRSF RPM and temperature guards to match.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Test firmware build ready — commit 76a6748

Download firmware for PR #11536

234 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

In listenOnly mode only escSensorData[0] is ever populated.
crsfRpm() and crsfTemperature() were using getMotorCount() to
size their frames, emitting N-1 zero/invalid slots alongside
the single real motor entry — confusing ground station displays.

Export getTelemetryMotorCount() from esc_sensor and use it in
both CRSF functions so the frame contains only the slots that
the ESC sensor actually populates.
@Jetrell
Copy link
Copy Markdown

Jetrell commented May 4, 2026

I gave it a quick flight test. It seems to occur less. However it still happens on the occasion.

RPM.Telemetry.glitch.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants