Copter: fix unconstrained avoidance backup speed #26858
Conversation
IamPete1
force-pushed
the
unconstrained_backup
branch
from
951f9e0 to
dd54484
Compare
|
Rebased and added a auto test which fails on master and passes with this PR. |
ArduCopter/mode.cpp
Outdated
| // Ensure back up action keeps to the climbrate limits of the mode | ||
| // Apply 50% knock down to ensure backup is not too exciting | ||
| const float knock_down = 0.5; | ||
| return constrain_float(target_rate, pos_control->get_max_speed_down_cms() * knock_down, pos_control->get_max_speed_up_cms() * knock_down); |
There was a problem hiding this comment.
Do we really need this additional constraint? Is this going to change how "backing up" feels to the users already using it?
I ask this since we are already constraining the back up in AC_Avoid (added in this PR)
There was a problem hiding this comment.
The thinking here is that there is only one back up speed in AC_Avoid. So a user might set that faster with horizontal speed in mind, say 5m/s. That would be fine in XY for most vehicles but could be too much for descending. Ultimately the vehicle should never move faster than the limits set for the given mode. If I could only have one constrain it would be this one. I can see a argument for not using the 50% knock down tho....
IamPete1
force-pushed
the
unconstrained_backup
branch
from
dd54484 to
ea1d443
Compare
|
I have reworked to take a pos control pointer from copter which allows the constrain to pos control limits to be done inside AC_Avoid resulting in a much smaller change. AC_Avoid is also used by rover which will not set the pointer so will not try and use pos control. A alternative method might be to add a pos control singleton. |
IamPete1
force-pushed
the
unconstrained_backup
branch
2 times, most recently
from
052c57d to
ac2508e
Compare
|
I'm slightly surprised that the vertical speed limits weren't being applied in AC_PosControl but perhaps that's because we are not using "input shaping" |
The pos controller does not apply any limits, its on the caller to restrict what they ask for. I do think it should (#24100) this is a case where having hard pos control limits would have been a good idea. |
|
We discussed this on the call and agreed to use a new AVOID_BACKZ_SPD parameter. |
IamPete1
force-pushed
the
unconstrained_backup
branch
from
ac2508e to
e275638
Compare
|
Reworked to separate the speed limit into horizontal and vertical components. I also found I had omitted to apply the Z limit in the 3D case which I have now fixed. |
|
@peterbarker, I think we can merge this unless you're comment above re the autotest is a blocker... so up to you. |
This fix is the result of a log where home changed altitude resulting in a huge breach of the max altitude fence. There was no fence action configured, however avoidance was enabled resulting in the vehicle trying to back away from the max altitude fence at 15 m/s. The configured climb rate limits were +-2.5 m/s.
Firstly this fixes the avoidance to constrain the velocity to
AVOID_BACKUP_SPDparameter.Secondly this constrains Z backup to half the set pos controller speeds. This helps keep the speeds in balance since the the avoid backup speed is probably set with XY speed in mind and could be too large for Z.
I have not tested this yet. We might want a minimal fix to just apply the backup speed in avoid for a backport.