Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qqqlab
Copy link
Contributor

@qqqlab qqqlab commented Feb 18, 2023

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 Controller

dlo 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 crc

payload is the netto telemetry payload inside the CRSF message.

ExpressLRS 3.1.2 telemetry rate 1:2 :

CRSF:10 bytes    20 bytes     best B/s   best B/s
payl: 5 bytes    15 bytes       CRSF     payload
Mode ul/dl dlo   ul/dl dlo     ul/dl      ul/dl
 50   5/ 3   5    3/ 1   2     60/ 20     45/ 15
100F 13/ 9  12   11/ 0  10    130/ 90     65/ 45
333F 42/28  45   30/ 0  36    420/280    210/140
500  63/35  61   31/14  33    630/350    465/210

After PR MSP:

 50   6/ 3   4    2/ 2   2     60/ 30     30/ 30
100F 12/12  12    8/ 8   9    160/160    120/120
333F 42/28  45   31/31  35    620/620    465/465
500  63/43  60   30/16  32    630/430    450/240

After PR MSP+SYNC:

 50   6/ 3   4    2/ 2   2     60/ 30     30/ 30
100F 12/10  10    8/ 6   9    160/120    120/ 60
333F 42/40  40   31/31  35    620/620    465/465
500  63/43  60   30/16  32    630/430    450/240

Theoretical CRSF B/s ul/dl: (assuming alternating RCDATA/MSP on uplink)

 50   62/ 125
100F 250/ 500
333F 833/1666
500  625/1250

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

@schugabe
Copy link
Contributor

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.

src/src/rx_main.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@wvarty wvarty left a 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

@qqqlab
Copy link
Contributor Author

qqqlab commented Mar 7, 2023

@schugabe The measurements are while RC and MSP and TLM data is being sent, also RC combined with bi-directional telemetry.

ul measures MSP telemetry going from handset to FC, dl measures TLM telemetry going from FC to handset.

dlo measures TLM telemetry with RC, also no MSP. Comparing dlo with dl makes it clear that sending uplink MSP messages interferes with the downlink TLM speed.

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.

@JyeSmith
Copy link
Member

JyeSmith commented Mar 7, 2023

Looks like this is breaking OTA compatibility and should be labelled for V4?

@pkendall64
Copy link
Collaborator

pkendall64 commented Mar 8, 2023

Looks like this is breaking OTA compatibility and should be labelled for V4?

Whilst technically true, it seems to operate quite happily if just the RX or the TX is upgraded. So it appears that it is cross-universe compatible!

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.

@pkendall64 pkendall64 added the V4.0 label Mar 8, 2023
@qqqlab qqqlab mentioned this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants