-
Notifications
You must be signed in to change notification settings - Fork 19k
Plane: fixed terrain guided target intermediate alt handling #29685
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
Conversation
f720093
to
fe91b01
Compare
Have you seen this @Ryanf55 ? Any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK, this altitude stuff is always tricky, I the key thing is just to test each case.
} else { | ||
loc.alt = baro.get_altitude() * 100 + AP::ahrs().get_home().alt; | ||
alt_cm = baro.get_altitude() * 100 + AP::ahrs().get_home().alt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is existing code, but I wonder if it should be baro.get_altitude_AMSL()
The difference is home altitude vs the set field elevation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that mean we'd have home alt in there twice?
I also think it should be a separate PR if we change it
need to fix RockBlock.lua |
for QGuided mode with terrain target we were applying the home -> current terrain offset twice
fixed bug in intermediate altitude for DO_REPOSITION with terrain
the Location object does not handle this combination. Throw an internal error and try to cope as best we can
fe91b01
to
9492720
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We flight-tested this today
Id like to understand how it was flight tested, and whether there are more cases we want to test. It's not clear from Peter's comment what they tested, and Pete said we want to test each case. Does Peter want us to assume that he tested each case in flight? How do we know if we don't know what was tested. If we have time:
I'm happy to help with any of these in a week. I really like the premise of the PR, but I feel entirely unqualified to approve it without doing any of the above. |
@Ryanf55 it has had extensive testing in:
The key cases are:
|
introduces a new set_target_altitude_proportion_terrain() which handles the case of a terrain target destination with a source as absolute alt or terrain alt
This also removes (and bans) the use of terrain_alt=1 with relative_alt=0. That combination is not handled by the Location object but was used by plane.
The fundamental problem is that in the Location object, if you have terrain_alt=1 and relative_alt=0 then the get_alt_m() function will get an altitude that is the height of terrain at that location plus the alt field. Plane has always assumed that in this case the altitude is the height of terrain at that location plus the alt field plus the home altitude. This is because we have a rule that when relative_alt=0 we assume that home.alt has been incorporated into the alt field. To square the circle we need to make this combination impossible, which is a major change in the plane code.