-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[WIP] FW navigation first order hold move to position controller #8883
Conversation
92dd5c0
to
fb5f092
Compare
TODO: unify FW position + loiter handling in position controller
|
973ed76
to
fc98262
Compare
@Stifael this kills off the navigator FOH entirely. The MissionBlock change is rather large. Additional review would be greatly appreciated. |
_global_pos.lat, _global_pos.lon, _global_pos.alt, | ||
&dist_xy, &dist_z); | ||
|
||
/* close to waypoint, but altitude error greater than twice acceptance */ |
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.
Because I just recently started to change comments type in another PR, I was wondering why you choose here /*
but above you chose the if //
-comment?
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.
It's a carry over from moving the old code. I use c++ style // for any new comments.
if ((dist >= 0.0f) | ||
&& (dist_z > 2 * _parameters.climbout_diff) | ||
&& (dist_xy < 2 * pos_sp_curr.acceptance_radius)) { | ||
|
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.
why two times the acceptance radius?
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.
Enough distance to cleanly enter the loiter radius. It's a bit arbitrary.
@tstastny might be cleaner to use L1 distance here?
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.
yes I would. This is anyway when the controller would start commanding a turn.
const float d_current = get_distance_to_next_waypoint(pos_sp_curr.lat, pos_sp_curr.lon, curr_pos(0), curr_pos(1)); | ||
|
||
/* Save distance to waypoint if it is the smallest ever achieved, however make sure that | ||
* min_current_sp_distance_xy is never larger than the distance between the current and the previous wp */ |
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.
What do you mean with
Save distance to waypoint if it is the smallest ever achieved
min_current_sp_distance _xy is always d_current as long as the distance is below distance_current_previous
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.
It's possible for a plane to miss waypoint acceptance and have to turn around and come back.
if (min_current_sp_distance_xy > pos_sp_curr.acceptance_radius) { | ||
|
||
/* update the altitude sp of the 'current' item in the sp triplet, but do not update the altitude sp | ||
* of the mission item as it is used to check if the mission item is reached |
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 assume this comment is a leftover from the navigator, since mission_item
does not exist in the position-controller.
@@ -198,10 +198,6 @@ Mission::on_active() | |||
set_mission_items(); | |||
} | |||
|
|||
} else if (_mission_type != MISSION_TYPE_NONE && _param_altmode.get() == MISSION_ALTMODE_FOH) { |
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 assume this param_altmode parameter is now not used anymore
Param no longer exists
src/modules/navigator/mission.cpp
Outdated
@@ -1058,14 +1032,6 @@ Mission::heading_sp_update() | |||
_mission_item.yaw = _wrap_pi(yaw + M_PI_F); | |||
pos_sp_triplet->current.yaw = _mission_item.yaw; | |||
|
|||
} else if (_param_yawmode.get() == MISSION_YAWMODE_FRONT_TO_WAYPOINT |
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.
whay is this one gone?
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 was assuming/anticipating you handling it entirely in the MC position controller. This was done before #9146 and needs review.
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 ROI portion should have been kept.
// set required yaw from bearing to the next mission item | ||
if (_mission_item.force_heading) { | ||
struct position_setpoint_s next_sp = _navigator->get_position_setpoint_triplet()->next; | ||
if (!_navigator->get_vstatus()->is_rotary_wing) { |
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 think it should be without negation
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.
Yes, good catch.
|
||
if (fabsf(yaw_err) > math::radians(_navigator->get_yaw_threshold())) { | ||
success = false; |
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 success and yaw_required boolean are a bit confusing to me.
Should not first be checked that yaw is required, and if required (https://github.com/PX4/Firmware/pull/8883/files#diff-7a824e3d1e9a5420071c8c9bbe27b8e7R353) it then needs to be checked for success?
Testing required
|
93ba6f1
to
953c4e9
Compare
@Antiheavy thanks for the thorough test. We need to think about how the switch between regular waypoint navigation and achieve waypoint altitude via loiter (once arriving) is handled. It might need to latch between one mode or the other a little more carefully. |
I'd like to pick this up again, starting by reviewing actual FW requirements. |
8c92f1a
to
885ee4f
Compare
40b47d5
to
54ad73d
Compare
@RomanBapst @sfuhrer let's take another look at this one. |
Closing as stale. |
This was finally completed in #15677. |
Navigator mission first order hold (FOH) altitude setpoint was limited to non-rotary wing missions within navigator. Given those restrictions it's a much better fit within the fixed wing position controller.