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: support SET_GPS_GLOBAL_ORIGIN message #6967

Merged
merged 9 commits into from
Sep 19, 2017

Conversation

rmackay9
Copy link
Contributor

@rmackay9 rmackay9 commented Sep 18, 2017

This PR resolves an issue that could cause vehicles to fly to extremely high altitudes during autonomous modes including RTL, Auto, Guided in cases where the ground station has sent an inaccurate altitude in a DO_SET_HOME message before the vehicle's EKF origin has been set (i.e. before a good GPS lock).

The following changes are included:

  • DO_SET_HOME commands (received either within a COMMAND_LONG message or SET_HOME_POSITION messages) only set the AHRS home, they no longer set the EKF origin.
  • SET_GPS_GLOBAL_ORIGIN messages are consumed and used to set the EKF origin. When the origin is set a GPS_GLOBAL_ORIGIN message is sent to the ground station
  • GET_HOME commands return both home and EKF origin to the caller (there is currently no command that allows requesting the origin)
  • when home is set (either automatically or when specified by the user) both the home and origin are sent to all mavlink channels
  • a new arming check is added that checks that the AHRS's reported position is within 10m of the GPS position. This check is within the GPS arming checks so the check will not run in cases where the user is not using a GPS (i.e. a manual flight mode or GPS is not being used)

These changes mostly only affect Copter (the AP_Arming check affects all vehicles) but I will extend the changes to at least Rover and Tracker (and Plane if people want) if the peer review is happy with them.

tridge
tridge previously requested changes Sep 18, 2017
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, with minor exception of struct init
thanks!

if (!check_latlng(packet.latitude, packet.longitude)) {
break;
}
Location ekf_origin;
Copy link
Contributor

Choose a reason for hiding this comment

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

this in unsafe stack init of a structure. Need to zero, or set options

@rmackay9 rmackay9 dismissed tridge’s stale review September 18, 2017 07:03

Issue raised by Tridge has been addressed in follow up commit so dismissing.

plane.set_ekf_origin(ekf_origin);
break;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this code be moved to the GCS_Mavlink library ?

Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

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

Besides my inline comment, LGTM.

I agree with @amilcarlucas though, I think we should be friendly to @peterbarker who has been moving identical code into GCS_Common; at least the MAVLink parsing could be there (the set_ekf_origin is also almost equal to all, but requires more changes to be common code).

// returns true on success
bool Copter::set_ekf_origin(const Location& loc)
{
// only allow setting origin if motors are disarmed
Copy link
Member

Choose a reason for hiding this comment

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

Why? The origin will be set if GPS fix is acquired while flying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also wondered if we should allow setting the ekf origin while moving. I can't think of a reason why we can't so I'll remove that restriction.

@amilcarlucas
Copy link
Contributor

For this to be completed I guess #6720 should get merged at some point :)

@OXINARF
Copy link
Member

OXINARF commented Sep 18, 2017

@amilcarlucas I don't see how that PR is in any way connected to this one...?

@amilcarlucas
Copy link
Contributor

That one also does a lot of stuff with origin and home.

@tridge
Copy link
Contributor

tridge commented Sep 18, 2017

I'd suggest one more small change to DO_SET_HOME. I think we should make param1=2 mean "set latitude and longitude, but not altitude".
For apps that want to do "return to my device" I think mostly they will want to only specify a latitude/longitude and not an altitude. To do that now they have to first fetch HOME_POSITION, then convert the altitude units then send back DO_SET_HOME with the fetched altitude. That is racy and also more difficult than it should be. Instead sending a DO_SET_HOME which doesn't set altitude is easier.

also remove setting of origin from DO_SET_HOME command
initialise ekf_origin location when consuming SET_GPS_GLOBAL_ORIGIN
set_ekf_origin performs sanity check on location
previously the home position could be set from DCM
This makes the setting of home slower but more accurate
@rmackay9 rmackay9 merged commit 3d7b6dd into ArduPilot:master Sep 19, 2017
@rmackay9 rmackay9 deleted the do-set-home-fix branch September 19, 2017 01:30
// check AHRS and GPS are within 10m of each other
Location gps_loc = ahrs.get_gps().location();
Location ahrs_loc;
if (ahrs.get_position(ahrs_loc)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to break the plane usage scenario where you move the aircraft from where it was booted then attempt to arm. If I left the plane at the new location long enough it was okay, however that took far longer then makes sense/would confuse a user in the field. (Confused me at least)

@rmackay9 rmackay9 moved this from PRs to 3.5.4 in Copter 3.5 Backports Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants