Skip to content

Conversation

@gismo2004
Copy link

@gismo2004 gismo2004 commented Sep 14, 2025

User description

This adds RPM and temperature telemetry values to CRSF.

Basically the values are taken from connected ESCs and temperature sensors if available. Not sure if it's useful to add MCU temperature or BARO temperature.

Currently, all temperatures are sent within one package with ID = 0 --> 20 values are supported.


PR Type

Enhancement


Description

This description is generated by an AI tool. It may have inaccuracies

  • Add RPM telemetry support for ESC sensors

  • Add temperature telemetry for ESCs and sensors

  • Implement CRSF frame types 0x0C and 0x0D

  • Support up to 20 temperature values per frame


Diagram Walkthrough

flowchart LR
  ESC["ESC Sensors"] --> RPM["RPM Telemetry (0x0C)"]
  ESC --> TEMP["Temperature Telemetry (0x0D)"]
  TEMP_SENSORS["Temperature Sensors"] --> TEMP
  RPM --> CRSF["CRSF Protocol"]
  TEMP --> CRSF
Loading

File Walkthrough

Relevant files
Enhancement
crsf.h
Add RPM and temperature frame type definitions                     

src/main/rx/crsf.h

  • Add CRSF_FRAMETYPE_RPM (0x0C) frame type constant
  • Add CRSF_FRAMETYPE_TEMP (0x0D) frame type constant
+2/-0     
crsf.c
Implement RPM and temperature telemetry functions               

src/main/telemetry/crsf.c

  • Add crsfSerialize24() function for 24-bit serialization
  • Implement crsfRpm() function for ESC RPM telemetry
  • Implement crsfTemperature() function for temperature telemetry
  • Add frame scheduling for RPM and temperature frames
  • Include ESC sensor and temperature sensor headers
+97/-0   

@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
⚡ Recommended focus areas for review

Invalid Frame Emission

In process scheduling, CRSF frames for RPM and Temperature are finalized even when no payload is written (e.g., ESC sensor disabled or no temps collected). This can emit empty/invalid frames. Consider making crsfRpm/crsfTemperature return a boolean (or check bytes written) and only call crsfFinalize when a payload was actually serialized.

    if (currentSchedule & BV(CRSF_FRAME_RPM_INDEX)) {
        crsfInitializeFrame(dst);
        crsfRpm(dst);
        crsfFinalize(dst);
    }
#endif
#if defined(USE_ESC_SENSOR) || defined(USE_TEMPERATURE_SENSOR)
    if (currentSchedule & BV(CRSF_FRAME_TEMP_INDEX)) {
        crsfInitializeFrame(dst);
        crsfTemperature(dst);
        crsfFinalize(dst);
    }
#endif
Buffer Overflow

crsfTemperature accumulates ESC and sensor temperatures into a fixed 20-element array without bounding checks, risking buffer overrun and oversized payloads if more than 20 values are available. Cap additions at 20 (and drop/aggregate extras) before writing the frame; also ensure the invalid/sentinel value scale matches the deci-degree unit used.

    int16_t temperatures[20];

#ifdef USE_ESC_SENSOR
    uint8_t motorCount = getMotorCount();
    if (STATE(ESC_SENSOR_ENABLED) && motorCount > 0) {
        for (uint8_t i = 0; i < motorCount; i++) {
            const escSensorData_t *escState = getEscTelemetry(i);
            temperatures[tempCount++] = (escState) ? escState->temperature * 10 : TEMPERATURE_INVALID_VALUE;
        }
    }
#endif

#ifdef USE_TEMPERATURE_SENSOR
    for (uint8_t i = 0; i < MAX_TEMP_SENSORS; i++) {
        int16_t value;
        if (getSensorTemperature(i, &value))
            temperatures[tempCount++] = value;
    }
#endif
Protocol Limit Check

crsfRpm writes motorCount 24-bit values without enforcing the protocol’s stated limit (1–19 values). Add a cap (e.g., min(motorCount, 19)) and compute payload length accordingly to prevent violating CRSF payload size constraints.

    uint8_t motorCount = getMotorCount();

    if (STATE(ESC_SENSOR_ENABLED) && motorCount > 0) {
        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);
        }
    }
}

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Sep 14, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consolidating temperature data is ambiguous

The current implementation merges temperature data from ESCs and other sensors
into a single telemetry frame with a generic source ID. This makes it impossible
to identify the origin of each temperature reading. The suggestion is to use
separate frames or distinct source IDs for different sensor types to provide
meaningful and distinguishable data to the user.

Examples:

src/main/telemetry/crsf.c [329-361]
static void crsfTemperature(sbuf_t *dst)
{

    uint8_t tempCount = 0;
    int16_t temperatures[20];

#ifdef USE_ESC_SENSOR
    uint8_t motorCount = getMotorCount();
    if (STATE(ESC_SENSOR_ENABLED) && motorCount > 0) {
        for (uint8_t i = 0; i < motorCount; i++) {

 ... (clipped 23 lines)

Solution Walkthrough:

Before:

// src/main/telemetry/crsf.c
static void crsfTemperature(sbuf_t *dst)
{
    uint8_t tempCount = 0;
    int16_t temperatures[20];

    // Collect ESC temperatures
    if (STATE(ESC_SENSOR_ENABLED)) {
        for (uint8_t i = 0; i < motorCount; i++) {
            temperatures[tempCount++] = getEscTemperature(i);
        }
    }
    // Collect other sensor temperatures
    for (uint8_t i = 0; i < MAX_TEMP_SENSORS; i++) {
        temperatures[tempCount++] = getSensorTemperature(i);
    }

    if (tempCount > 0) {
        // Send all temperatures in one frame with a single, hardcoded source ID
        crsfSerialize8(dst, 0); // Source ID is always 0
        for (uint8_t i = 0; i < tempCount; i++)
            crsfSerialize16(dst, temperatures[i]);
    }
}

After:

// src/main/telemetry/crsf.c
static void crsfTemperature(sbuf_t *dst)
{
    // Option: Send separate frames for each source type

    // Frame for ESC temperatures
    if (escTempCount > 0) {
        // ... initialize frame ...
        crsfSerialize8(dst, 0); // Source ID for ESCs
        for (each esc_temp) {
            crsfSerialize16(dst, esc_temp);
        }
        // ... finalize and send frame ...
    }

    // Frame for other sensor temperatures
    if (sensorTempCount > 0) {
        // ... initialize frame ...
        crsfSerialize8(dst, 1); // Source ID for other sensors
        for (each sensor_temp) {
            crsfSerialize16(dst, sensor_temp);
        }
        // ... finalize and send frame ...
    }
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant design flaw where temperature data from different sources (ESCs, sensors) are merged, making it impossible for the user to distinguish them, which severely limits the feature's usefulness.

Medium
  • Update

@sensei-hacker
Copy link
Member

Thanks for making your first PR for INAV!

To avoid compile warnings, you may want to expand the scope of
#ifdef USE_ESC_SENSOR to cover the declaration of
crsfRpm etc.
Otherwise it looks like you're defining functions and variables that are never used, which to the compiler looks like typos.

@stronnag
Copy link
Collaborator

In order to pass CI, your changes should not emit warnings.
Setting -DWARNINGS_AS_ERRORS=ON in your CMake setup will help you catch these before pushing to the repo.

@gismo2004 gismo2004 changed the title [crsf] add temperature and RPM telemetry [crsf] add temperature, RPM and AirSpeed telemetry Sep 23, 2025
@sensei-hacker
Copy link
Member

Who has tested this, please?

@Jetrell
Copy link

Jetrell commented Nov 8, 2025

@gismo2004 I wanted to test this with my older radio running Opentx and a TBS CRSF module. To see if it had compatibility.
And noticed that I lost Vspd from #10694 when I had this firmware loaded. But after I flashed back to another master build from a few days ago, without these commits, Vspd returned. Not sure if it has anything to do with 0x09 payload size.
Any ideas ?

@gismo2004
Copy link
Author

@Jetrell honestly, I have no idea how these changes should influence other messages. VSpd is sent via message 0x07 which this PR does not touch at all. I don't know how OpenTx handles CRSF telemetry, but seems to be different to EdgeTx where it works for me at least. Not sure what you mean by 0x09 payload size? If you think it is related to message 0x09 Barometric Altitude & Vertical Speed which is NOT sending VSpeed in iNAV then we need to find out, why it is implemented like that currently. Additionally, if it is really message 0x09 that causes it, I am still unsure how my changes interact with that massage?!

@sensei-hacker any thoughts?

@MrD-RC
Copy link
Member

MrD-RC commented Nov 9, 2025

I can't see anything in this PR which would affect vspeed. However #11100 does make a change to that sensor. It wouldn't surprise me if the change breaks OpenTX. Which is why I asked for the legacy mode to be added.

It seems like OpenTX would really benefit from a small update to bring it in line with some protocol changes. I hope that happens. I really don't want to install EdgeTX.

@Jetrell
Copy link

Jetrell commented Nov 10, 2025

I hope that happens. I really don't want to install EdgeTX.

Me too. I still use OpenTX on my older X9D+ for testing. It saves wearing out the newer radios switches etc. I prefer to leave it for my better aircraft.
I also wonder about CRSF. Being that both TBS and ELRS may use the same protocol. But what about the handling of that data after that point by the individual software's.
I had also wondered how older TBS or ELRS firmware would handle the FC throwing telemetry data bits at it, at first boot, if those older RC link firmware's didn't recognize them. You would think it would ignore it. But what if it causes an error.
I raise this matter. Because I have had multiple TBS partial lockups or massive RC latency when the FC is powered and I switch off the RC TX, then switch it back on to allow re-connection. This started to occur after the implementation of #10694. But I also noticed it with this PR too. If not a bit worse.
One could say, why not update the TBS firmware. Which I have tried. But the older OpenTX scripts don't play well with CRSF V3.
Not sure ignoring sensor instants would help in this case ?
I'm sure I won't be the only one that still uses a 5 year old radio with OpenTX, who could also experience this issue. What concerns me, is if it causes an issue in INAV's handling in a FS condition, if that RX serial data becomes very latent. Could it cause an FC lockup ? I know TBS had an issue when telemetry data got flooded. It would loose its bind.

Feel free to chime in @MrD-RC @stronnag with your knowledge. of how INAV would handle this.

@MrD-RC
Copy link
Member

MrD-RC commented Nov 10, 2025

I agree with you. Personally I've not updated Crossfire in a long time. Because of all the problems that people started having with links being dropped. Is the latest firmware even reliable?

As for EdgeTX. I simply don't like it on my X10. I see it as a step backwards. The UI is unnecessarily oversized and a lot of things that were fixed in OpenTX were broken again in EdgeTX. Making it worse to use. It works ok on the Jumper T15. The fat UI is less noticeable on the smaller screen. But there are still the other issues. Text entry with the buttons is particularly annoying on Edge.

[EDIT]
I take that back. I created a model on the T15 the other day. It took three times as long as it would have on OpenTX. Because EdgeTX removed useful functionality and made other areas more painful to use. ETX sucks balls!

@sensei-hacker
Copy link
Member

I really don't want to install EdgeTX.

I still use OpenTX on my older X9D+

I'll third that. OpenTX here. The newest EdgeTX might have some feature that would be really cool if I didn't have INAV taking care of the cool stuff.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants