Copter: remove position-vector methods #10587
Conversation
ArduCopter/GCS_Mavlink.cpp
Outdated
| if (!AP::ahrs().home_is_set()) { | ||
| break; | ||
| } | ||
| const Location &origin = inertial_nav.get_origin(); |
There was a problem hiding this comment.
you could even use ahrs directly instead of inertial_nav
// check EKF origin has been set
Location ekf_origin;
if (!ahrs.get_origin(ekf_origin)) {
return false;
}
|
On Thu, 21 Feb 2019, Pierre Kancir wrote:
you could even use ahrs directly instead of inertial_nav
I didn't want to have that fight here :-)
|
peterbarker
force-pushed
the
pr/no-posvec
branch
from
d4ff332
to
164ef47
Compare
|
No problem, merge just as it is, it is already a good step forward ! |
|
So the change in behaviour here is that it requires the home to have been set. Previously if home hadn't been set then it would still accept the position target but the altitude would be interpreted as an altitude above the EKF origin. I'm not too sure it matters though because I don't think in other completely separation places in the code we require the home to be set before the autonomous flight modes will work. |
|
Can I also ask what testing has been done? We're touching a critical feature but it's not one that is used very often so we need to be sure we're not breaking it. In general I think merges are easier if two things are done:
I may add this to the developer wiki section which discusses how to get PRs merged faster |
peterbarker
force-pushed
the
pr/no-posvec
branch
from
164ef47
to
ce8ea63
Compare
|
@rmackay9 Actually, the old behaviour was "use undefined values", AFAICS. That would probably mean using a home altitude of zero (absolute), so you would end up with a very large number for your desired altitude.... I'm suspicious of any place which uses get_home() without checking if it is set first. In this case, you're probably right in that lots of places probably ensure that's the case before we get here. I have tidied up the test I was using for this and pushed it up. It does NOT attempt to trigger the apparently problem, just exercises the code. |
|
@peterbarker, OK great, I'll merge once it passes travis.. |
|
.. seems to be failing travis.. not sure if this is caused by this change or something unrelated.. |
peterbarker
force-pushed
the
pr/no-posvec
branch
from
ce8ea63
to
3a84b11
Compare
| @@ -1033,7 +1033,12 @@ void GCS_MAVLINK_Copter::handleMessage(mavlink_message_t* msg) | |||
| pos_vector += copter.inertial_nav.get_position(); | |||
| } else { | |||
| // convert from alt-above-home to alt-above-ekf-origin | |||
| pos_vector.z = copter.pv_alt_above_origin(pos_vector.z); | |||
There was a problem hiding this comment.
I've got to say this was actually incorrect because this message is for local frame, which means it should be an altitude above origin and not above home.
We've actually debated this before and it's my fault this isn't corrected along with LOCAL_POSITION_NED (see #6830 and #6893 for reminders).
I'll recommend to merge as is and I'll make a PR to fix the things above as we've agreed before.
ArduCopter/inertia.cpp
Outdated
| } | ||
| current_loc.set_alt_cm(inertial_nav.get_altitude(), frame); | ||
| current_loc.change_alt_frame(Location::ALT_FRAME_ABOVE_ORIGIN); |
There was a problem hiding this comment.
Although I do agree with this, everyone is aware this is a real change from using altitude above home 99% of the time to using altitude above origin 100% of the time, right?
And this means that keeping the code above for using home altitude makes no sense since we'll always end up with altitude above origin.
There was a problem hiding this comment.
Changed this to home-relative. Insane.
There was a problem hiding this comment.
I thought it was too easy, why did I say something? 😄
I wished the code to get the altitude used the AHRS but I know I won't convince you 😞😛
peterbarker
force-pushed
the
pr/no-posvec
branch
2 times, most recently
from
b55c2f2
to
4482fb3
Compare
Both were used in just one place
peterbarker
force-pushed
the
pr/no-posvec
branch
from
4482fb3
to
79b430c
Compare
|
Looks ok to me. Txs for the fix |
|
This looks like the PR that caused this bug in Copter-4.0. #13441 Not trying to point fingers, I'm involved here too. Just want us to gain understanding of how bugs are getting in so we can do better next time. |
|
@rmackay9 I would say rushed reviews some times do it. It took me 5 seconds now to see the issue, but I clearly didn't see it when I reviewed this. |
Both were used in just one place