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

Plane: handle guided path requests in GUIDED mode #26328

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Feb 26, 2024

Add a new method handle_guided_path_request to the plane GUIDED mode and extra update function update_path in AP_Navigation. A preliminary implementation is provided for AP_L1_Control. The guided path mode is used to handle SET_POSITION_TARGET_GLOBAL_INT mavlink commands.

This is a simplified version of:

It excludes the NPFG controller and modifies the existing ModeGuided rather than introduce a new mode.

Details

  • The implementation follows the approach used in Rover's ModeGuided which supports sub-modes.
  • Two modes are provided: WP for the existing handle_guided_request, and Path for the new handled_guided_path_request.
  • A new virtual update method update_path is added to AP_Navigation.
  • An implementation in AP_L1_Control uses the existing update_waypoint and update_loiter functions.
  • Add a flag to disable loiter radius scaling. The scaling causes the vehicle to overshoot turns and loiter at a larger radius than commanded by the off-board planner. For terrain planning this may take the vehicle outside the safe regions.

Testing

See the tests using the terrain_navigation package in:

Tasks

Issues to resolve following further testing:

  • Rework the handling of the SET_POSITION_TARGET_GLOBAL_INT type_mask as the logic is not correct.
  • Use the POSITION_TARGET_XXX enum from mavlink common.h to check the bit mask.
  • Add CI test suite.
  • Must make sure that MAVProxy (changealt) and MissionPlanner can still change the vehicle's altitude

ArduPlane/mode.h Outdated Show resolved Hide resolved
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.

needs a CI test suite

bool ModeGuided::handle_guided_path_request(Location position_on_path, Vector2f unit_path_tangent, const float path_curvature, const bool direction_is_ccw)
{
// add home alt if needed
if (position_on_path.relative_alt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should use change_alt_frame()

Copy link
Contributor

Choose a reason for hiding this comment

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

refuse -ve curvature?

@@ -515,3 +519,34 @@ void AP_L1_Control::update_level_flight(void)

_data_is_stale = false; // status are correctly updated with current waypoint data
}

// update L1 control for path following
void AP_L1_Control::update_path(const class Location &position_on_path, Vector2f unit_path_tangent, float path_curvature, int8_t direction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment on units, check sign of curvature
explain what zero means
explain what direction means

if (!is_zero(path_curvature)) {
// moving along arc of circle - loiter about wp located at
// centre of curvature.
float radius_m = 1.0 / path_curvature;
Copy link
Contributor

Choose a reason for hiding this comment

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

fabs() ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The curvature is computed as path_curvature = accel_lat.length() / vel.length_squared(); which is always positive.
Should we add a comment that curvature is assumed to be positive in the header?

libraries/AP_L1_Control/AP_L1_Control.cpp Outdated Show resolved Hide resolved
cmd.content.location.terrain_alt = false;
switch (pos_target.coordinate_frame)
{
case MAV_FRAME_GLOBAL_RELATIVE_ALT_INT:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes we move these types of checks right to the top of the function. This just gets it out of the way and allows us to remove some indenting lower down.

@tridge tridge removed the DevCallEU label Mar 13, 2024
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Must make sure that MAVProxy (changealt) and MissionPlanner can still change the vehicle's altitude!

cmd.content.location.alt = pos_target.alt * 100;
cmd.content.location.relative_alt = false;
cmd.content.location.terrain_alt = false;
switch (pos_target.coordinate_frame)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try mavlink_coordinate_frame_to_location_alt_frame

i.e. extract as much useful stuff from our COMMAND_INT / COMMAND_LONG parsing...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rebase on top of #28479 once that's merged.

ArduPlane/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
ArduPlane/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
bool _enter() override;
bool _pre_arm_checks(size_t buflen, char *buffer) const override { return true; }

private:

SubMode _guided_mode;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check that the vehicle always ends up in the WP sub mode when it is first switched to Guided mode? So it probably always does the first time the vehicle is switched to Guided but it may not if the vehicle is in Path sub mode (in Guided), then switched to another mode, then back to Guided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it's desireable for guided mode to preserve any state when you switch out of it and back into guided?

For example, if you had sent a heading command, switch to RTL for 0.5 seconds, and back to guided, it seems the vehicle should chose the default WP sub mode and drop a point where it enters guided rather than go back to the old stale heading command.

Copy link
Contributor

@timtuxworth timtuxworth Oct 25, 2024

Choose a reason for hiding this comment

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

Do you think it's desireable for guided mode to preserve any state when you switch out of it and back into guided?

This is why I added a HEADING_TYPE_DEFAULT mavlink option in #27428 - so you can explicitly switch to GUIDED_HEADING_NONE, because it should definitely not preserve state when switching out of the mode and then back again.

bool ModeGuided::handle_guided_path_request(Location position_on_path, Vector2f unit_path_tangent, const float path_curvature, const bool direction_is_ccw)
{
// add home alt if needed
if (position_on_path.relative_alt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are various altitude types so it might be best to instead use the Location class's method to change the altitude type.

ArduPlane/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
ArduPlane/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
@@ -147,6 +147,10 @@ float AP_L1_Control::turn_distance(float wp_radius, float turn_angle) const

float AP_L1_Control::loiter_radius(const float radius) const
{
if (_disable_loiter_radius_scaling) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this variable get unset?

@srmainwaring srmainwaring force-pushed the prs/pr-plane-guided-path branch from 72f40b4 to 65ce2e6 Compare March 13, 2024 08:19
Location location_on_path_abs_alt = location_on_path;

// add home alt if needed
if (location_on_path_abs_alt.relative_alt) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of these bools, just call the altitude getter with the right frame.

libraries/AP_L1_Control/AP_L1_Control.cpp Outdated Show resolved Hide resolved
ArduPlane/mode.h Show resolved Hide resolved
default:
gcs().send_text(MAV_SEVERITY_WARNING, "Unknown GUIDED mode");
break;
}
Copy link
Contributor

@timtuxworth timtuxworth Oct 24, 2024

Choose a reason for hiding this comment

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

Guided Mode already (logically) has another "sub mode" which is "heading".

ArduPilot/ardupilot_wiki#6096

void set_radius_and_direction(const float radius, const bool direction_is_ccw);

void update_target_altitude() override;

protected:

enum class SubMode: uint8_t {
WP,
Path
Copy link
Contributor

Choose a reason for hiding this comment

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

Heading

Copy link
Contributor

Choose a reason for hiding this comment

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

Also "None"

srmainwaring and others added 21 commits October 28, 2024 14:12
- Implement method update_path
- Update comment
- Reduce look-ahead distance in update_path

Signed-off-by: Rhys Mainwaring <[email protected]>
- Add guided path sub-mode

- Extend handling of SET_POSITION_TARGET_GLOBAL_INT to include path guidance
- Check for non-zero velocity and acceleration
- Calculate path curvature and direction
- Force update of adjust_altitude_target.
- Remove duplicate check for guided mode.

Signed-off-by: Rhys Mainwaring <[email protected]>
* Save flash

Signed-off-by: Ryan Friedman <[email protected]>
@srmainwaring srmainwaring force-pushed the prs/pr-plane-guided-path branch from fad521e to bce81f7 Compare October 28, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

6 participants