Proper unsigned value support for CRSF Telemetry

[CapnBry]

Alters the deserialization of CRSF telemetry to properly support unsigned values, and removes the check for values being -1.

Details

The CRSF telemetry parser currently unpacks every value as signed, regardless of what the spec says for that parameter. There are unsigned values in the spec though, and the current code (going all the way back) does not present them properly. For example the GPS packet's heading value is unsigned and that means that any value greater than 327.67 degrees shows up as negative. Betaflight telemetry example:

In addition, the current code checks for values where every byte sent is 0xff and discards the value if that condition is met. There is nothing in the CRSF spec that says "send 0xff... to not update the value", the only mention is on the 0x09 Barometric Altitude description where the example contains a comment stating "OpenTX counts any 0xFFFF [altitude] value as incorrect". Due to this code being used always, any telemetry parameter anywhere will ignore 0xff, 0xffff, 0xffffff, or 0xffffffff. This has been removed for also not matching the CRSF spec.

I also removed the template function used to do the parsing. I am not sure who was like ooh this is gonna really optimize performance when the compiler unrolls that loop for me! It's stupid, just pass the size.

Now-Unsigned Telemetry Items

• GPS - Ground Speed, Heading, Altitude (uses -1000 offset), satellites
• Baro Alt - Altitude (encoded)
• Airspeed
• RPM - Sensor ID
• Temp - Sensor ID
• Cells/Voltages - Sensor ID, Voltage (in both types)
• Link RX - RSSI percentage, RF Power dBm
• Link TX - RSSI percentage, RF Power dBm, FPS
• Battery - "Capacity" (mAh consumed), Percent remaining
• CRSFShot / OpenTX Sync - Update Interval

See? It works! Code size is also reduced by this change by 192 bytes.

[3djc]

To be fair, initial CRSF support was done with minimal documentation.

[pfeerick]

Wasn’t the initial support done by or in collaboration with TBS though? I thought that was the reason OpenTX and Betaflight implementations of CRSF was considered the defence implementation by many. Not taking into account the changes to the CRSF spec over time, or bugs in the initial implementation.  On a personal note, it will be nice to have non-negative headings... 😂

[CapnBry]

Wasn’t the initial support done by or in collaboration with TBS though?

Yeah it was done by a third party working for TBS I believe. I also think of this implementation as the reference, but also nobody still around knows why they were coded the way they were. I do believe that the -1 thing was unintentional baggage from an early prototype implementation, because no FC software I am aware of (Betaflight, iNav or Ardupilot) sends -1 to prevent a field from updating. I don't believe TBS sends internal telemetry (LINK_RX/TX) with -1 in the values, else it would be mentioned in the spec.

But! This is all just researched speculation on my part. As far as the non-TX-to-Handset telemetry items, it seems that that's all but guaranteed by the FC implementations listed. There could be some obscure legacy hardware I'm not aware of though.

[bkleiner]

As far as i can tell no crossfire version available today uses this mechanic, to be frank, i don't think it makes a lot of sense to begin with, if one doesn't want update, one might as well just re-send the last value.

As for its origin, appears it was somewhat haphazardly introduced here.

[raphaelcoeffic]

For RSSI1/RSSI2/SNR fields, a stale/no-data byte of 0xFF was previously suppressed; it will now show as -1 in telemetry widgets during link-down transitions. Cosmetic rather than functional.

This position is defensible (it's not in spec), but worth being aware of for release notes.

[raphaelcoeffic]

What about just adding this to avoid regressions?

  value = getCrossfireTelemetryValue(3, rxBuffer, 2, false);
  if (value == 0xFFFF) break;   // spec: sentinel for invalid

Since the spec says something like: "OpenTX counts any 0xFFFF [altitude] value as incorrect".

[raphaelcoeffic]

Old code required both update_interval AND offset to have at least one non-0xFF byte before calling getModuleSyncStatus(module).update(...). New code always calls it.

If a malformed/initial frame with all-0xFF ever shows up, update_interval = 0xFFFFFFFF / 10 ≈ 429M µs ≈ 7 min would be fed to the mixer scheduler. Depending on how update() clamps, this could skew the refresh period badly.

Probably not observed in practice (ELRS/TBS likely don't emit that), but the old defensive check cost nothing. Consider keeping a bounds check (e.g. if (update_interval && update_interval < REASONABLE_MAX)).