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

AP_Frsky_Telem: replaced the passthrough scheduler with a WFQ one with rate limiting #12532

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

yaapu
Copy link
Contributor

@yaapu yaapu commented Oct 10, 2019

This replaces the default scheduler with a WFQ one with a rate limiter for selected packets, thus preventing link saturation.

It will be the base for adding more on top of the scheduler, like bidirectional telemetry for instance and AP_Scripting support.

It improves the library compatibility with external frsky sensors and fixes issue #12450 where adding GAS Suite and FLVSS on the s.port bus would eventually lead to starvation for lower priority packets like GPS ones.

more info on the dedicated discuss thread

@floaledm please can you check this against flightdeck, I'd like to maintain full backward compatibility!

This replaces the default scheduler with a WFQ one
@yaapu yaapu requested a review from floaledm October 10, 2019 07:47
@yaapu yaapu changed the title Ap_Frsky_Telem: replaced the passthrough scheduler with a WFQ one with rate limiting AP_Frsky_Telem: replaced the passthrough scheduler with a WFQ one with rate limiting Oct 10, 2019
@floaledm
Copy link
Contributor

Thanks for your work on this and for asking me to review. A brief bench test seems to show it works fine with FlightDeck.

I did not read the code changes really but I read the discuss thread and share the concern about having message chunks transmitted twice, or even just once in some circumstances. I understand you're trying to juggle a lot on a very small bandwidth... not sure that having garbled messages as a result is a good tradeoff for a slightly faster update rate on other things. But overall I like the improvements so I'm not going to bicker over that. Especially since two-way will fix any problem of lost message chunks...

Great job!

@yaapu
Copy link
Contributor Author

yaapu commented Oct 10, 2019

@floaledm thanks for testing my patches!

I read the discuss thread and share the concern about having message chunks transmitted twice, or even just once in some circumstances. I understand you're trying to juggle a lot on a very small bandwidth... not sure that having garbled messages as a result is a good tradeoff for a slightly faster update rate on other things

I'm aware that's a tradeoff but at 1 message every 4 seconds (worst case scenario with gas suite, 2 more sensors and sending messages 3 times) messages would be discarded by the force_push call because the queue would not be emptied fast enough, maybe in this case is better to send them and risk corruption vs losing them, this at least was the rationale behind my proposal.
Anyway we might add a parameter in the future and let the user decide.

@rmackay9
Copy link
Contributor

this has been added to Copter-4.0.0-rc3

@rmackay9 rmackay9 moved this from PRs to 4.0.0-rc3 in Copter 4.0 backports Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Rover 4.0 backports
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants