-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adds Airspeed, RPM, TEMP, Cells telemetry packets #3197
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
Adds Airspeed, RPM, TEMP, Cells telemetry packets #3197
Conversation
|
Just for the record, please note that a CRSF frame may be longer or smaller than the documented structure:
In other words, an RPM or temperature frame with only one value is a valid telemetry frame. |
|
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) |
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. |
|
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. |
|
I'm happy to approve this once the requested changes are made. |
CapnBry
left a comment
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.
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!
d302d75 to
f589488
Compare


https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#0x0a-airspeed
https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#0x0c-rpm
https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#0x0d-temp
https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#0x0e-cells-sensor