-
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
Corrrect SmartRTL internal error by preserving home srtl point #17419
base: master
Are you sure you want to change the base?
Corrrect SmartRTL internal error by preserving home srtl point #17419
Conversation
The actual test guts failing is this:
|
The proposed fix now included here stops us from removing the home point from the list of points in the return path. Since that home position is explicitly written to using set_home, it doesn't make sense to remove it from the path as it will never get reset. |
94e2f9c
to
a318ca6
Compare
wp_nav->set_wp_destination_NED(dest_NED); | ||
} else { | ||
INTERNAL_ERROR(AP_InternalError::error_t::flow_of_control); |
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.
we probably need a comment here saying why this shouldn't happen.
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've added comments in this case and the similar case below it.
a318ca6
to
779f65d
Compare
if (g2.smart_rtl.get_num_points() == 0) { | ||
// this is the very last point, add 2m to the target alt and move to pre-land state | ||
dest_NED.z -= 2.0f; | ||
HAL_Semaphore &sem = g2.smart_rtl.path_semaphore(); |
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 thought we always used a WITH_SEMAPHORE macro instead of writing the take and give directly.
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.
In this case we're not taking this semaphore in a blocking fashion - we're using take_nonblocking. As we're in the main thread and the path-tidying thread can take the path sempahore for prolonged periods of time so we don't want to block.
Doing it this way does mean we have to be extra-special-careful to release the semaphore, of course.
Note that this is pre-existing. I've just moved the "take" of the semaphore to be out around all of the work the main thread is doing to ensure consistency of the path count with the pop/peek operations.
Closes #17416 |
779f65d
to
3cba43c
Compare
3cba43c
to
752e49f
Compare
752e49f
to
c14ce7d
Compare
@peterbarker could you rebase this? It will probably resolve this issue #17416 |
c14ce7d
to
92b432a
Compare
I've pushed up a rebased version; for the most part I continuously rebase all my PRs. I haven't looked at this branch in years, so it does need some study before merge. OTOH, the SmartRTL stuff hasn't been touch in that long either! |
92b432a
to
149213c
Compare
This home position is explicitly set when set_home is called (e.g. via the mavlink interface). It doesn't make any sense to pop that home position from the list as it won't ever get reset.
149213c
to
398a1ee
Compare
yeah, we should try and get this in.. |
It looks like there's a valid failure in the autotest to do with python cleanliness and an unused variable. |
@peterbarker maybe you could rebase this again? We seem to have a new PR #28284 trying to resolve issue #22780 so maybe we should try and get this one in instead |
this is possibly triggering a bug in the state management of SmartRTL’s home or waypoint preservation system |
This test is expected to fail.
It's based on another PR which adds a simple, working, test for SmartRTL.