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

QuadPlane: Precision Landing(v2) #11339

Closed
wants to merge 8 commits into from

Conversation

lukedempsey
Copy link

This is a carry-on from @jonathan84clark's original PR (#10378) - adding the precision landing functionality to quadplanes.

I have got it landing consistently in SITL - I'm yet to do a hardware test.

I haven't squashed commits etc yet, as I still have a couple of questions about the code, I'll add them as comments below.

@lukedempsey lukedempsey changed the title Precision Landing for Quadplane (v2) QuadPlane: Precision Landing(v2) May 13, 2019
#endif

if ((poscontrol.state == QPOS_POSITION2 && plane.auto_state.wp_distance < 2) ||
(doing_precision_landing && target.length() < 2)) {
Copy link
Author

Choose a reason for hiding this comment

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

There is similar code in control_loiter (lines 1231-1257) which was previously written that also controls landing state transitions. Wasn't sure to put it there or here - where there is a QPOS_LAND_DESCEND transition already defined.

@@ -2177,6 +2243,23 @@ void QuadPlane::vtol_position_controller(void)
} else {
pos_control->set_xy_target(poscontrol.target.x, poscontrol.target.y);
}

#if PRECISION_LANDING == ENABLED
// run precision landing
Copy link
Author

Choose a reason for hiding this comment

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

I'm not fully around pos_control, but it seems that when I override the it with pos_control->set_xy_target(target_pos.x, target_pos.y); there is less authority to move the craft to the target location - it can hover for many seconds approx 3m away from target coordinates horizontally, delaying the Q_POS_DESCEND transition

libraries/SITL/SIM_QuadPlane.cpp Outdated Show resolved Hide resolved
@tridge
Copy link
Contributor

tridge commented May 27, 2019

thanks for this work!
It does need a rebase, can you do that or do you want me to do that for you?

@lukedempsey
Copy link
Author

thanks for this work!
It does need a rebase, can you do that or do you want me to do that for you?

No problem, I'll do this tomorrow

Copy link
Contributor

@tridge tridge 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 added lots of nitpick comments, but this is really pretty good! I look fwd to hearing about testing on a real vehicle

ArduPlane/Log.cpp Outdated Show resolved Hide resolved
ArduPlane/Parameters.h Outdated Show resolved Hide resolved
ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
if (land_icengine_cut != 0) {
plane.g2.ice_control.engine_control(0, 0, 0);
float height_above_ground = plane.relative_ground_altitude(plane.g.rangefinder_landing);
if (doing_precision_landing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the different descent rate handling for precland to normal land?
also, it omits the ICE control in precland case

ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
pos_control->set_alt_target_from_climb_rate(-land_speed_cms, plane.G_Dt, true);
case QPOS_LAND_FINAL: {
cmb_rate = compute_descent_rate(0.5f, doing_precision_landing);
pos_control->set_alt_target_from_climb_rate_ff(cmb_rate, plane.G_Dt, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

again, why change to _ff, and why change rate at all?

#endif

if (poscontrol.state == QPOS_POSITION2 &&
((plane.auto_state.wp_distance < 2) || (doing_precision_landing && target.length() < 2))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could abstract this into a function that gets the XY distance to target

ArduPlane/sensors.cpp Outdated Show resolved Hide resolved
int32_t height_above_ground_cm = current_loc.alt;

// use range finder altitude if it is valid, else try to get terrain alt
if (rangefinder_alt_ok()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use the existing Plane::relative_ground_altitude() function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite sure we shouldn't do this - and should stick to passing rangefinder data through to the library here.

If we were to drop down to using the terrain database the errors could be really quite large. Better to be safe here and solely rely on rangefinder data IMO - at least for an initial cut.

@lukedempsey
Copy link
Author

thanks for this work!
It does need a rebase, can you do that or do you want me to do that for you?

No problem, I'll do this tomorrow

@tridge this branch has previously been kept up to date but merging master into it. Not sure how to squash/rebase this easily. Any advice on what to do? I don't have much experience rebasing.

I'll work through the rest of your notes in the meantime

@tridge tridge force-pushed the precLandQuadPlane branch 3 times, most recently from 1433142 to d158944 Compare May 31, 2019 10:15
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

looks good now!

@jo-m
Copy link

jo-m commented Aug 1, 2019

@lukedempsey is this going to be merged soon?

@davidbuzz
Copy link
Collaborator

I poked tridge during the dev call, and he said it needs a rebase, and he might have a try....

@CBMUniversal
Copy link

Is this still being worked on? Is there a timeline for this being merged?

@jonathan84clark
Copy link

jonathan84clark commented Oct 29, 2019 via email

@rmackay9
Copy link
Contributor

I'm not responsible for the plane code but in general a good way to escalate a PR (give it a push towards getting merged) is to add the "DevCallTopic" label (or ask someone like me, tridge, amilcarlucas, etc to add it) and then attend the weekly dev call. I know the timing of the weekly call can be difficult for some locations... but just trying to give some help on the process.

mumble server used for weekly dev call: http://ardupilot.org/dev/docs/ardupilot-mumble-server.html

@jonathan84clark
Copy link

jonathan84clark commented Oct 30, 2019 via email

@jonathan84clark
Copy link

I should be clear; I have no intention of completing all of the requested changes and features that were recommended when I submitted this pull request. I personally feel that those requests are outside the scope of my task; implementing precision landing in quadplane and represent scope creep.
Additionally it would be extremely high-risk for me to make any additional changes as I no longer have a platform to test on.
I stand by my original code change; it worked on the day I submitted my pull request. I have documented video evidence of its functionality. However without access to test platforms I cannot confirm it will work today (I can't deny it either).

@tridge
Copy link
Contributor

tridge commented Aug 2, 2020

I have rebased this on master

@ohitstarik
Copy link
Contributor

ohitstarik commented Mar 30, 2021

Any updates on this? I'm trying to build tree precLand, i've also tried c131054c1a5959233a078725691ddad2070cb016
and as it sits and it falls apart when it gets to uavcan on both ends

image
image

i'd love to get this running and help out if possible

edit: turns out it was just a problem with the way i was cloning the git. i was doing git download and doing git submodule udpate --recursive after, just doing git clone --recursive fixed this.

coming across this in the precland tree next

image

edit2: again a problem with how i was setting up my build environment (didnt update submodules after changing trees, a folder name had a space in it and make didnt like that, etc etc)

got it to compile and put it onto a cubeBlack board. Accidentally compiled and uploaded arducopter first, worked great, compiled arduplane after and missionplanner wont pick up any heartbeats now.

@peterbarker
Copy link
Contributor

I've rebased this on master, having already merged some of the patches (which needed @rmackay9 's approval)

I've also fixed several things I spotted - the update rate we pass into AC_PrecLand on construction must be the same as that in the scheduler table; the library itself takes the minimum of that and the scheduler loop rate to determine its buffer sizes.

There are still several more issues to cross here.

@peterbarker peterbarker force-pushed the precLandQuadPlane branch 2 times, most recently from 0c08c96 to bc74036 Compare April 14, 2021 06:19
@ddomit
Copy link

ddomit commented May 26, 2021

I've rebased this on master, having already merged some of the patches (which needed @rmackay9 's approval)

I've also fixed several things I spotted - the update rate we pass into AC_PrecLand on construction must be the same as that in the scheduler table; the library itself takes the minimum of that and the scheduler loop rate to determine its buffer sizes.

There are still several more issues to cross here.

@peterbarker Hi peter! Just wondering if there is anything i can help with to have this feature implemented into quadplane

@davidbuzz
Copy link
Collaborator

i think if @tridge and @peterbarker have both massaged the code thru forced-pushes AND tridge has written 'looks good' , then this is simply a merge-on-ci-pass now.

@peterbarker
Copy link
Contributor

I'm removing MergeOnCIPass. At the very least the scope of rebasing required would mean this would need another thorough review.

@markcfong561
Copy link

Is this going to be merge to master anytime soon?

@ddomit
Copy link

ddomit commented Aug 1, 2022

Is this going to be merge to master anytime soon?

+1

@markcfong561
Copy link

Are there at least any updates on whether this will be added to the next release of arduplane?

@julled
Copy link

julled commented Dec 20, 2022

+1 i would be also very interested in getting this into mainline

@tridge
Copy link
Contributor

tridge commented Mar 3, 2024

i've decided to do this with lua scripts for flexibility: #26380

@tridge tridge closed this Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet