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

Copter: surface tracking fix #18531

Merged
merged 7 commits into from Sep 6, 2021

Conversation

lthall
Copy link
Contributor

@lthall lthall commented Aug 31, 2021

This PR updates the surface tracking code to use the new terrain following algorithms in the position controller.

The existing code relies on the slower response of the target following approach and now causes overshoot as seen in this report:
https://discuss.ardupilot.org/t/altitude-overshoot-on-ascent-descent-tuning-help-please/74756

@lthall lthall requested a review from rmackay9 August 31, 2021 13:23
@rmackay9 rmackay9 changed the title 20210831 surface tracking fix Copter: surface tracking fix Aug 31, 2021
ArduCopter/Copter.h Outdated Show resolved Hide resolved
@rmackay9
Copy link
Contributor

rmackay9 commented Sep 2, 2021

I'm put some suggestions for non-functional changes but in general this is looking pretty good to me.

@lthall lthall force-pushed the 20210831_SurfaceTrackingFix branch 3 times, most recently from a773339 to 17c281f Compare September 3, 2021 04:11
Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this and it seems to be working well.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep most of the removed consts. Looks like making

void update_vel_accel(float& vel, float accel, float dt, float limit)

void update_vel_accel(float& vel, const float accel, const float dt, const float limit)

Would allow the consts to remain.

Looks like we can also remove RNGFND_GAIN param as a result of this.

@rmackay9
Copy link
Contributor

rmackay9 commented Sep 3, 2021

@IamPete1,

Thanks very much for the review. Removing RNGFND_GAIN is a bit historic I think.

We don't need any of those consts. There was one outright mistake where it was using a const reference ("const float &") but never changing the value and the rest are unnecessary const-ing of pass-by-value arguments.

@rmackay9
Copy link
Contributor

rmackay9 commented Sep 4, 2021

@IamPete1, fixed, this resolves your comments i guess? Thanks again for the feedback on this complex PR

@tatsuy
Copy link
Contributor

tatsuy commented Sep 4, 2021

Thank you for this improvement.
There are no problem with normal flight.

When I use upward facing terrain following, DSAlt shows negative value.
And it is sometimes intermittent.

@lthall
Copy link
Contributor Author

lthall commented Sep 4, 2021

@tatsuy If you are using upward facing You should be below it right? So it should read negative.

Can you send me this log? Maybe make it available on the discuss forums?

Comment on lines +57 to 60
TUNING_RANGEFINDER_GAIN = 41, // unused
TUNING_EKF_VERTICAL_POS = 42, // unused
TUNING_EKF_HORIZONTAL_POS = 43, // unused
TUNING_EKF_ACCEL_NOISE = 44, // unused
Copy link
Member

@IamPete1 IamPete1 Sep 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TUNING_RANGEFINDER_GAIN = 41, // unused
TUNING_EKF_VERTICAL_POS = 42, // unused
TUNING_EKF_HORIZONTAL_POS = 43, // unused
TUNING_EKF_ACCEL_NOISE = 44, // unused

If we just delete all of these then we can be sure nowhere in the code is trying to use them.

the param values need a update here:

// @Values: 0:None,1:Stab Roll/Pitch kP,4:Rate Roll/Pitch kP,5:Rate Roll/Pitch kI,21:Rate Roll/Pitch kD,3:Stab Yaw kP,6:Rate Yaw kP,26:Rate Yaw kD,56:Rate Yaw Filter,55:Motor Yaw Headroom,14:AltHold kP,7:Throttle Rate kP,34:Throttle Accel kP,35:Throttle Accel kI,36:Throttle Accel kD,12:Loiter Pos kP,22:Velocity XY kP,28:Velocity XY kI,10:WP Speed,25:Acro RollPitch kP,40:Acro Yaw kP,45:RC Feel,13:Heli Ext Gyro,38:Declination,39:Circle Rate,41:RangeFinder Gain,46:Rate Pitch kP,47:Rate Pitch kI,48:Rate Pitch kD,49:Rate Roll kP,50:Rate Roll kI,51:Rate Roll kD,52:Rate Pitch FF,53:Rate Roll FF,54:Rate Yaw FF,58:SysID Magnitude

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the Parameter description!
I have not removed the lines in the defines.h file as that is a style change and best off left as a separate commit from people with style.

libraries/AC_AttitudeControl/AC_PosControl.cpp Outdated Show resolved Hide resolved
@tatsuy
Copy link
Contributor

tatsuy commented Sep 4, 2021

@lthall Previously, it was displayed as positive values.
Current master
image

This branch
image
this is the log.
SurfaceTrackingFix.zip
I've changed a bit to test this in SITL. and then I enabled Proximity avoidance.
https://github.com/tatsuy/ardupilot/commits/20210831_SurfaceTrackingFix

About being intermittent, sorry, this is my mistake.
I checked again, and there was no problem.

@lthall
Copy link
Contributor Author

lthall commented Sep 4, 2021

@tatsuy, I see what you are saying. I will chat to Randy about this.

Does everything work with the upwards facing lidar even though the log is backwards compared to what it was before?

@tatsuy
Copy link
Contributor

tatsuy commented Sep 4, 2021

@lthall

Does everything work with the upwards facing lidar even though the log is backwards compared to what it was before?

yes,it works with upward facing terrain following and proximity

@lthall
Copy link
Contributor Author

lthall commented Sep 4, 2021

@tatsuy I have reversed the log so it should match the old behaviour now. Thanks for highlighting this. Could you take another look?

@tatsuy
Copy link
Contributor

tatsuy commented Sep 5, 2021

@lthall Everything is working fine for me. Thank you!

@rmackay9 rmackay9 dismissed IamPete1’s stale review September 6, 2021 05:06

removed the extra param so dismissin PEter's review, thanks!

@rmackay9 rmackay9 merged commit 97decc4 into ArduPilot:master Sep 6, 2021
@lthall lthall deleted the 20210831_SurfaceTrackingFix branch September 6, 2021 07:35
@tatsuy
Copy link
Contributor

tatsuy commented Sep 6, 2021

@rmackay9 Can we add this to Copter-4.1?

@rmackay9
Copy link
Contributor

rmackay9 commented Sep 6, 2021

@tatsuy, yes, this will be included in the next beta release hopefully later this week. Txs very much for uncovering that issue!

@tatsuy
Copy link
Contributor

tatsuy commented Sep 6, 2021

Great, thank you.

@rmackay9 rmackay9 added this to pending in Copter 4.1 Sep 9, 2021
@rmackay9 rmackay9 moved this from pending to 4.1.0-rc1 in Copter 4.1 Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Copter 4.1
4.1.0-rc1
Plane 4.1
Awaiting triage
Rover 4.1
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants