ardupilot
ardupilot copied to clipboard
ArduPilot
Plane: RTL: min climb fix and add loiter to alt before RTL option
This fixes a bug where if RTL_CLIMB_MIN is set we always climb before turn. That is due to this line not checking if the climb before turn option is set
https://github.com/ArduPilot/ardupilot/blob/c27e3f0c353940291829d90b512b8a64788ce1cf/ArduPlane/mode_rtl.cpp#L46-L52
Seems to have been broken since https://github.com/ArduPilot/ardupilot/pull/16823, excitingly the auto-test added in https://github.com/ArduPilot/ardupilot/pull/16565 does not work with the fixed behavior. It is still climbing by the time it gets to the halfway point. So this fixes the test too.
This then adds a new option to loiter to alt before returning, Fixes #12170
Since we now have two options I have moved both to a new RTL_TYPE param and done the param conversion. All options now also obey RTL_CLIMB_MIN.
This also removes a pointless current mode log in the do_RTL function.
This work is sponsored by ARACE.
It could also not be a bug, and I have just not understood. I don't understand the point of the CLIMB_BEFORE_TURN flight option if we also force that behavior when doing RTL_CLIMB_MIN.
I guess you might want to hold level for RTL_CLIMB_MIN but ALT_HOLD_RTL could be above that altitude? So once you reached RTL_CLIMB_MIN you turn home even though you may still be climbing? I could add another RTL_TYPE for that.
@skorokithakis what behavior were you trying to get with the CLIMB_BEFORE_TURN option?
I can explain, I've been flying this for a while and it works perfectly.
Basically, the behavior of CLIMB_BEFORE_TURN is to climb to ALT_HOLD_RTL and only turn when you're above it. RTL_CLIMB_MIN does not achieve this behavior (and I'm not entirely clear on what exactly it achieves).
I'm a bit confused on what the bug is exactly, can you elaborate a bit?
@skorokithakis Currently we force no roll during the RTL_CLIMB_MIN. So you might be at 50m, have ALT_HOLD_RTL set 100m and RTL_CLIMB_MIN set too 100m. Current master will climb to 150m (current + climb min) with no roll. Then decend during the return to 100m. So in effect you get CLIMB_BEFORE_TURN weather you asked for it or not.
My understanding was that RTL_CLIMB_MIN would climb on the return leg if CLIMB_BEFORE_TURN was not set, it currently does not.
This PR preserves the behavior you describe.
As I recall, the RTL_CLIMB_MIN was to prevent wingtip strikes if RTL engages while at low altitude....then it should allow the turn and the climb to continue....it was implemented simply and if above ATL_HOLD_RTL it would still climb holding wings level and then proceed....it could have been more complex and only be active if below that return alt....but was not the CLIMB_BEFORE_TURN option was to provide yet another variation, in which the climb to ALT_HOLD_RTL is completed before turning home...this was for when flying like below a cliff and a reasonable value of RTL_CLIMB_MIN would not guarantee a crash into the cliff face....so you have options of climbing a little to prevent issues close to the ground or surfing through obstacles....and one for the cliff runners out there...
I think having only a RTL_CLIMB_MIN and setting it such that it only works at starting alts below ALT_HOLD_RTL is not a good solution....you want to be able to set that to 10M to avoid wingtip strikes, but not 100M which would force slow climbers a LOT further away from HOME on an RTL.....fixing RTL_CLIMB_MIN to work only below ALT_HOLD_RTL is fine, but you would not set it at the ATL_HOLD_RTL height to emulate the CLIMB_BEFORE_TURN feature IMHO....
I think the important question to ask here is "what problem does RTL_CLIMB_MIN solve?". @Hwurzburg provides the context here, though, as far as I know (from a third-hand source, admittedly) RTL_CLIMB_MIN was intended to clear obstacles before turning back (and the best guess for a clear path at any time is forward).
For the set of users who need the behaviour I describe (going straight to clear all obstacles and only turning back at a safe altitude, ALT_HOLD_RTL), RTL_CLIMB_MIN is entirely superseded by CLIMB_BEFORE_TURN, so we don't have to consider that set at all (they can just use CLIMB_BEFORE_TURN, and it will suit their use case perfectly).
For the set of users @Hwurzburg describes, RTL_CLIMB_MIN solves their problem as it is now.
I don't know what problem turning before climb solves if RTL_CLIMB_MIN is set, presumably you have set your ALT_HOLD_RTL correctly and don't need some "extra" altitude over ALT_HOLD_RTL (I put "extra" in quotes because you don't really know if you'll go over ALT_HOLD_RTL, you might be very low and just reach ALT_HOLD_RTL second).
With my PR, I wanted to preserve the old behaviour of RTL_CLIMB_MIN and just supersede it (for my use case) with CLIMB_BEFORE_TURN. That's why I specifically didn't touch the behaviour of RTL_CLIMB_MIN at all, and didn't make it check whether CLIMB_BEFORE_TURN was set.
Given the above, I think RTL_CLIMB_MIN gaining some extra altitude before turning is correct in all cases.
Please let me know if you disagree, I'd also be happy to elaborate on my PR or use case.
It seems that RTL_CLIMB_MIN ever climbing above ALT_HOLD_RTL is wasting energy (this is the current behavior). In which case CLIMB_BEFORE_TURN is better. The only use case we would loose is where your low and current alt + RTL_CLIMB_MIN is lower than ALT_HOLD_RTL.
I can also see the argument with the new loiter behavior that climb min is useful because it would give a combination. It would allow you to keep level for say 50m then start loitering up the rest of the way so you don't fly so far in what might be the wrong direction.
I think RTL_CLIMB_MIN is confusing name. Maybe we rename it to RTL_LEVEL_CLIMB and limit it to never climb above ALT_HOLD_RTL. In which case the CLIMB_BEFORE_TURN behavior could be achieved by setting this new param to a value larger than ALT_HOLD_RTL.
@IamPete1 I agree with everything in your comment, but I'm personally in the "if it ain't broke" camp on this one. I've been flying CLIMB_BEFORE_TURN a lot and it works perfectly, and I don't know if it's worth changing it (maybe it is).
This is an aside, but the one place where it doesn't work well, unfortunately, is terrain following, where it gets confused because of AMSL and CLIMB_BEFORE_TURN doesn't trigger if you're more than ALT_HOLD_RTL meters above MSL. I tried to fix it, but I don't know what CLIMB_BEFORE_TURN should do when you have a steeply-sloping mountain ahead of you, so I left it.
After thinking about this a bit, I think what would really be the desired function is as follows:
- fix RTL_CLIMB__MIN to work only below ALT_HOLD_RTL...this gets rid of the weird climb above ALT_HOLD_RTL in most cases
- leave the option CLIMB_BEFORE_TURN as legacy, but
- add an RCx_OPTION 3Pos, that if implemented would give [low] current behaviour,[mid]climb before turn,[high]loiter up to ATL_HOLD_RTL and then return
this would allow current behaviour, but allow a pilot to have RTL_CLIMB_MIN set low just to clear ground or low obstacles, allow him to chose always CLIMB_BEFORE_TURN as an option, or be able to change the behavior situationaly with a switch...normal , or if cliff skimming or flying behind mountains (CLIMB_BEFORE_TURN or LOITER TO ALT) without forcing longer RTLs on slow climbers by requiring RTL_CLIMB_MIN to be set high to emulate CLIMB_BEFORE_TURN
Will this also fix terrain following, by any chance?
what is wrong with terrain following currently?
@pompecukor this bit:
This is an aside, but the one place where it doesn't work well, unfortunately, is terrain following, where it gets confused because of AMSL and CLIMB_BEFORE_TURN doesn't trigger if you're more than ALT_HOLD_RTL meters above MSL. I tried to fix it, but I don't know what CLIMB_BEFORE_TURN should do when you have a steeply-sloping mountain ahead of you, so I left it.
Looks like the problem lines that @IamPete1 identified are still in place.
@IamPete1 do you believe this to still be a problem?
I'm afraid I'm still not really sure what problem was identified; the description is a little ambiguous. If you can describe how to reproduce the problem I could write an autotest against master to replicate (or not, hopefully!). Might be worth doing this before attempting to rebase this one?
@timtuxworth might be interested in this one, given his canyon-flying shenanigans
The partner I am working with often flies from a home location on a mountain top that could be 500m above the valley being surveyed by the plane. What they wanted was RTL to immediately trigger a loiter up to RTL_ALTITUDE before returning.
Tridge gave us the suggestion of adding a LOITER_TO_ALT with lat/long 0/0 after a DO_LAND_START which does exactly what we want.
I can't help but wondering if all of the special cases mentioned here could be solved by using DO_LAND_START followed by some combination of mission items. It would certainly reduce the complexity for the user and possibly the c++ code complexity/size too.
Many users are overwhelmed by the idea of mission planning...Takeoff mode is completely unnecessary since it can easily be easily setup as a mission...but having a mode is easy for newbees