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
Add telemetryStatus bit to MSP+SYNC OTA packets #2088
base: master
Are you sure you want to change the base?
Conversation
The sync package is sent every 5000ms so it does next to nothing and I would not change the sync message for such a tiny improvement. Did you measure the throughput for msp messages while sending rc data? Because the assumption was that you always have rc data beeing sent and thus the confirmation bit is just embedded in the data that is always sent. TLM messages are not sent twice but we have the link package with a relativily high frequency so that would explain some reduced throughput. |
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.
Minor formatting requested as per comments
@schugabe The measurements are while RC and MSP and TLM data is being sent, also RC combined with bi-directional telemetry.
Yes, you are right, the 4 byte linkstat data (which contains the TelemetryStatus/Confirm bit for MSP) is added to the TLM packages, and certainly not helping the downlink TLM speed. But I don't think it explains the full drop. Some of the drop might be related to #2095 and #2103 . Time permitting, I'll do some further investigations. |
Looks like this is breaking OTA compatibility and should be labelled for V4? |
EDIT: I retested and a TX with this version is unable to retrieve all the Lua information from an RX, so dI do not think it is compatible and should be deferred to 4.0. |
Add telemetryStatus bit to MSP and SYNC OTA packets to improve bi-directional telemetry throughput.
Telemetry OTA messages need an acknowledgement from the radio, which is transmitted as the telemetryStatus bit in the RCDATA OTA packet. MSP and SYNC messages do not have this status bit. When using MSP messages to send uplink telemetry from the radio to the flight controller, the telemetryStatus is not sent and the downlink telemetry throughput is reduced all the way down to zero in worst case.
This PR adds a telemetryStatus bit to the MSP and SYNC packets to improve throughput for bi-directional telemetry.
I'm not 100% sure the bit for the SYNC message is needed/helping. It can only be added to the "full" packets without changing the OTA packet format. I did not see measurable improvement in throughput after adding it, but left it in this PR.
Below my throughput measurements before and after this PR.
ul/dl
uplink/downlink: stable throughput in packets/second when sending MSP messages from the Radio Controller to the FC and simultaneously sending TELEM messages from the FC to the Radio Controllerdlo
downlink only: stable throughput in packets/second when sending TELEM messages from the FC to the Radio Controller (no MSP messages)CRSF
is the CRSF packet as sent over-the-air, including 5 bytes CRSF headers: sync length type destination and crcpayload
is the netto telemetry payload inside the CRSF message.The uplink rates match the theoretical expectations.
The PR improves the downlink throughput for longer packages in full modes. But, the downlink throughput is still less than half of what I was hoping for. What is going on? Is every TELEM packet sent twice ota? Link stats or sync every second downlink packet? I did not investigate further. Any ideas on this?
I used the attached Lua scripts on EdgeTX and Ardupilot to do the measurements:
https://github.com/qqqlab/ardupilot/blob/pr/csrf_pop/libraries/AP_Scripting/applets/CRSF/CRSF-Rate-E.lua
https://github.com/qqqlab/ardupilot/blob/pr/csrf_pop/libraries/AP_Scripting/applets/CRSF/CRSF-Rate-A.lua