-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
base: master
Are you sure you want to change the base?
Plane: handle guided path requests in GUIDED mode #26328
Conversation
b811f87
to
119ca3b
Compare
64bba35
to
4d07d90
Compare
b445ae6
to
72f40b4
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.
needs a CI test suite
ArduPlane/mode_guided.cpp
Outdated
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) { |
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.
this should use change_alt_frame()
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.
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) { |
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.
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; |
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.
fabs() ?
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.
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?
cmd.content.location.terrain_alt = false; | ||
switch (pos_target.coordinate_frame) | ||
{ | ||
case MAV_FRAME_GLOBAL_RELATIVE_ALT_INT: |
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.
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.
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.
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) |
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.
Try mavlink_coordinate_frame_to_location_alt_frame
i.e. extract as much useful stuff from our COMMAND_INT
/ COMMAND_LONG
parsing...
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.
Rebase on top of #28479 once that's merged.
bool _enter() override; | ||
bool _pre_arm_checks(size_t buflen, char *buffer) const override { return true; } | ||
|
||
private: | ||
|
||
SubMode _guided_mode; |
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.
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.
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.
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.
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.
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.
ArduPlane/mode_guided.cpp
Outdated
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) { |
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.
There are various altitude types so it might be best to instead use the Location class's method to change the altitude type.
@@ -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) { |
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.
how does this variable get unset?
72f40b4
to
65ce2e6
Compare
Location location_on_path_abs_alt = location_on_path; | ||
|
||
// add home alt if needed | ||
if (location_on_path_abs_alt.relative_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.
Instead of these bools, just call the altitude getter with the right frame.
default: | ||
gcs().send_text(MAV_SEVERITY_WARNING, "Unknown GUIDED mode"); | ||
break; | ||
} |
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.
Guided Mode already (logically) has another "sub mode" which is "heading".
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 |
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.
Heading
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.
Also "None"
Signed-off-by: Rhys Mainwaring <[email protected]>
- Implement method update_path - Update comment - Reduce look-ahead distance in update_path Signed-off-by: Rhys Mainwaring <[email protected]>
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]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
Signed-off-by: Rhys Mainwaring <[email protected]>
* Save flash Signed-off-by: Ryan Friedman <[email protected]>
Signed-off-by: Ryan Friedman <[email protected]>
Avoid !is_zero
fad521e
to
bce81f7
Compare
Add a new method
handle_guided_path_request
to the plane GUIDED mode and extra update functionupdate_path
inAP_Navigation
. A preliminary implementation is provided forAP_L1_Control
. The guided path mode is used to handleSET_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
ModeGuided
which supports sub-modes.WP
for the existinghandle_guided_request
, andPath
for the newhandled_guided_path_request
.update_path
is added toAP_Navigation
.AP_L1_Control
uses the existingupdate_waypoint
andupdate_loiter
functions.Testing
See the tests using the
terrain_navigation
package in:Tasks
Issues to resolve following further testing:
SET_POSITION_TARGET_GLOBAL_INT
type_mask
as the logic is not correct.POSITION_TARGET_XXX
enum from mavlinkcommon.h
to check the bit mask.