Skip to content

Conversation

@mha1
Copy link
Contributor

@mha1 mha1 commented Apr 28, 2025

@neoxic
Copy link

neoxic commented May 1, 2025

Just for the record, please note that a CRSF frame may be longer or smaller than the documented structure:
https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#frame-details

image

In other words, an RPM or temperature frame with only one value is a valid telemetry frame.

@CapnBry
Copy link
Member

CapnBry commented May 8, 2025

I tested this with my own python telemetry generator and an EdgeTX lua script to verify the values are passed through properly. Seems to work, as expected.

There's one insanely glaring problem with these new telemetry types though, due to the inefficient but simple slot-based queueing mechanism in the ExpressLRS RX. The queue only takes into account the type of packet and there is only one slot per type (apart from the special 2-slot workaround). If two different source_id are trying to send e.g. Temperature, they will fight for the queue slot and overwrite each other which will lead to "sensor lost" callouts on the handset when one source starves another. Fixing this would require a full reengineering of the RX's queuing system to have knowledge of the internals of telemetry packets and not just inspect the type.

This effect is visible in the test script I've attached where the RPM values for the two sources do not appear consistently. There should be one 4 motor RPM (id=0) and one with 19 RPM values (id=1)
Example command line: python standalone_test.py -P COM7 -b 420000 --ti 100
newtlm.zip
image

@pkendall64
Copy link
Collaborator

If two different source_id are trying to send e.g. Temperature, they will fight for the queue slot and overwrite each other which will lead to "sensor lost" callouts on the handset when one source starves another. Fixing this would require a full reengineering of the RX's queuing system to have knowledge of the internals of telemetry packets and not just inspect the type.

I 100% agree with this. We need to redo the whole slot-based system as it's causing quite a few issues now. There was the streaming PR that was done some time ago but has languished and is now wildly out of date. Perhaps we could revisit this PR.

@CapnBry
Copy link
Member

CapnBry commented May 9, 2025

I do agree with you @pkendall64 on the need to redo the telemetry queueing mechanism and perhaps the StubbornSender in more of a streaming fashion that employs the length and checksum bytes of the packets instead of relying on the completion packet (every piece of data requiring at least 2 tlm frames)?

I think that's a much bigger project for 4.0 though and at least we can take this PR in the interim so there's a known working code that can be used for testing with Radiomaster et al and be something the EdgeTX devs can use to be able to transfer these new official types through. Our current code throws them away because the CRSF packet type number is too low and changing that would just put them all in the 2 spare slots which could be worse behavior and I'm not sure if that would then cause other weird low-type packets to compete for that space as well.

As always, I am open to discussion if you think we could both resolve this in a better way more quickly, but I think this is a good quick solution until we have a better alternative.

@pkendall64
Copy link
Collaborator

I'm happy to approve this once the requested changes are made.

@mha1 mha1 requested a review from CapnBry May 10, 2025 10:05
Copy link
Member

@CapnBry CapnBry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again and working still even with existing types, both with iNav and my test script. Thanks for being the fastest new CRSF types implementer!

@mha1 mha1 force-pushed the PR_add_airspeed_temp_rpm_tlm branch from d302d75 to f589488 Compare May 11, 2025 08:08
@mha1 mha1 merged commit e88aeb3 into ExpressLRS:3.x.x-maintenance May 11, 2025
48 checks passed
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.

4 participants