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

Fix GPS Rescue throttle PID calculation #7884

Merged
merged 1 commit into from Mar 29, 2019

Conversation

Projects
None yet
6 participants
@etracer65
Copy link
Member

etracer65 commented Mar 27, 2019

Fixes an extraneous "*" in the throttle PID calculation that caused the integral portion to be multiplied by the derivative calc instead of adding it.

Credit to @IllusionFpv for noticing.

Fix GPS Rescue throttle PID calculation
Fixes an extraneous "*" in the throttle PID calculation that caused the integral portion to be multiplied by the derivative calc instead of adding it.

@etracer65 etracer65 added this to the 3.5.8 milestone Mar 27, 2019

@etracer65

This comment has been minimized.

Copy link
Member Author

etracer65 commented Mar 27, 2019

Frankly I'm amazed the previous throttle PID logic even worked well enough to maintain altitude. The calculation has a couple of hardcoded divisors (/10 on the integral and /20 overall). I don't know how these were determined and the fear is that they might have been chosen experimentally based on what "worked well". With the calculation being incorrect this then might make these questionable, but I don't know.

So we need some people to test this to make sure the throttle behavior and altitude control still function correctly (and hopefully better).

@McGiverGim, @TonyBlit, @IllusionFpv can anybody help test?

@TonyBlit

This comment has been minimized.

Copy link
Contributor

TonyBlit commented Mar 28, 2019

Indeed, it's amazing how this piece of code has been there all the time, seemingly working ok.
However, I'm not convinced there's a real need to change it for 3.5.x. I mean, the code is clearly wrong, but nobody has complained about not doing its job.
On the other hand, people routinely complains about this particular code to be difficult to configure. Nobody understands the PID values (and now we know why), and the only useful/understandable parameter is max_throttle.
So, I don't think is worth trying to understand what those magic numbers are and finding new PID values for this code, aside from the potential mess of breaking configs in a 3.5.x release.
My proposal is to calculate the throttle around a target vertical speed (in m/s), so it would be way easier to configure depending on how powerful is your quad. People with weak batteries will easily choose a slower vertical speed without having to worry about battery sag, keeping max_throttle as a safety net. There would be some PID values as well, but aside from developers/testers I expect nobody else would ever need to tune them. This change can be introduced in BF 4.1 as it will be breaking the config.

@etracer65

This comment has been minimized.

Copy link
Member Author

etracer65 commented Mar 28, 2019

Well the short term goal is just to test the behavior with the fix. We don't know if the the magic numbers or throttle PID values need tuning. I suspect that previously it was acting primarily as a "P controller" only as it would likely to expect the altitude derivative to be small between loops and since it was multiplied into I it probably nullified the integral effect as well.

@TonyBlit

This comment has been minimized.

Copy link
Contributor

TonyBlit commented Mar 28, 2019

Fair enough. I'll test this weekend.

@IllusionFpv

This comment has been minimized.

Copy link
Contributor

IllusionFpv commented Mar 28, 2019

Tested, now it's much better, nice and smooth throttle control (and today there are 45km/h of wind here)

flight video:
https://www.youtube.com/watch?v=mg8cbwQyiHI&feature=youtu.be

log with debug_mode = RTH
btfl_001.zip

@McGiverGim

This comment has been minimized.

Copy link
Member

McGiverGim commented Mar 28, 2019

I don't know if I will have time this weekend to go to fly :( If I can I will test it the sunday, but the test of @IllusionFpv seems good :)

@mikeller mikeller merged commit fa3fd56 into betaflight:master Mar 29, 2019

@mikeller mikeller modified the milestones: 3.5.8, 4.0 Mar 29, 2019

@TonyBlit

This comment has been minimized.

Copy link
Contributor

TonyBlit commented Mar 30, 2019

Today I have done a few runs with and without the fix, to be sure I could compare the change in the same conditions. I can confirm the throttle control goes butter smooth now with this change. \o/
While the throttle curve was so coarse before the change, now it's a nice and smooth typical PID-controlled curve, at a very low frequency (way below 1 Hz). Indeed it looks like both the spikes and coarseness were caused by small errors amplified by the infiltrated mult operand, like if the P-controller was being switched on and off randomly. Actually, I've removed the I and D terms completely, just for fun, and the resulting P-controlled curve is also super smooth and just a bit slower to converge. So it's indeed amazing how the previously glitched P-controller could even work, somehow. :-)

@etracer65 etracer65 deleted the etracer65:gps_rescue_throttle_pid_fix branch Apr 3, 2019

@aowi7280

This comment has been minimized.

Copy link

aowi7280 commented Apr 3, 2019

Tried RC5 today. Throttle is pretty smooth now, it used to go from min to max and back. The altitude overshoots the setpoint by 8-10m; it always has. It does not come close to holding that altitude and drops by 15m when transitioning to forward flight. I have max angle set to 45 degrees and it seemed to go to max angle, then by the time it got the correct forward speed, it had descended a lot. The heading did not align very well with where the real home is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.