-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Populate error_msg for all action result messages (#4341) #4460
base: main
Are you sure you want to change the base?
Conversation
d3be8c2
to
da5e15a
Compare
Let me know if you have any questions - but seems like we're covering that in the issue thread instead |
756982f
to
cdccbdd
Compare
bd49b55
to
54f0402
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.
Sorry for the delay here - it is still draft so it was lower on my priority list while getting Jazzy out. It seems generally complete, but there seems to be a couple of things missing. When might this be ready for a full review?
See below for an initial review of things that jumped out
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/spin_action.hpp
Show resolved
Hide resolved
I can rip through these fixups today. |
2fdbaea
to
19c5b10
Compare
@aosmw, your PR has failed to build. Please check CI outputs and resolve issues. |
Builds seem to fail from your additions of the error messages:
Sounds good! In that case, I can give it a full review, I got through about ~half of it yesterday and I can do the other half right now. |
nav2_docking/opennav_docking_bt/include/opennav_docking_bt/undock_robot.hpp
Outdated
Show resolved
Hide resolved
nav2_simple_commander/nav2_simple_commander/example_nav_to_pose.py
Outdated
Show resolved
Hide resolved
5ace62c
to
135bbb5
Compare
5a1e7aa
to
0461cfc
Compare
Status Report: I am still dog fooding this in my own backport to humble. I am successfully propagating useful error messages to our navigator action clients. Tip for any zenoh users (zenoh-bridge-ros2dds 0.11.0-stable), default actions timeout after 5 seconds and you won't get a result, here is a snippet of zenoh config that may help.
I still need to hunt down the exception handling triggering
|
That's a classic decoding issue in the BT. I'm guessing you have the wrong type set for the input/output port. All the blackboard and port objects gets stringified to store, so its having a problem with that process. I'd look into the types and make sure that (A) they're types that have conversion functions implemented (or basic types) and (B) you're both setting the port correctly and setting its value to the right type in the BT node. I think we ran into an issue with a couple being wrong, so maybe you just missed one more. Exciting update! Let me know how the dogfooding goes! |
0461cfc
to
bcb0638
Compare
Finally found the "basic_string: construction from null is not valid" - details of how in the commit message bcb0638 |
Woof, I see that error, that's an annoying one to find. I'm glad you got that unblocked - copy paste errors can be the worst! Let me know if I'm blocking you :-) |
You are not blocking me. I am STILL hunting down what I think is a cancellation that gets reported by the navigator action server as an abort. The result error_code = 0 and error_msg is empty. |
Use SFINAE to log get_error_details_if_availble() in ActionT::Result Ideally all Action messages would have error_code and error_msg fields in their Result. BUT the tests use standard messages that do not and will not. i.e. test_msgs/action/Fibonacci.action Some Action messages only have error_code and not error_msg Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
…uested() (4341)(4602) Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
Key point is only use the internal_error_(code|msg)_ if the action result error_code is 0. Signed-off-by: Mike Wake <[email protected]>
…on#4341) Clear output path for ComputePathThroughPoses similar to ComputePathToPose Explicitly note NOT to clear error_code_id and error_msg behavior tree output ports. Otherwise we cannot read them when the navigator reports errors. Signed-off-by: Mike Wake <[email protected]>
…s-navigation#4341) Make it consistent with NavigateThroughPoses. This was introduced when changing logic to remove internal exception. Signed-off-by: Mike Wake <[email protected]>
…ion#4341) Allow of reason for missed waypoint as error_msg. Signed-off-by: Mike Wake <[email protected]>
Include error_msg in reason for missed waypoint Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
…ros-navigation#4341) Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
…#4341) Signed-off-by: Mike Wake <[email protected]>
…#4341) It was a red herring in search for error_msg clearing bug on abort Signed-off-by: Mike Wake <[email protected]>
…-navigation#4341) Signed-off-by: Mike Wake <[email protected]>
…ion#4341) Signed-off-by: Mike Wake <[email protected]>
…ros-navigation#4341) Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
…avigation#4341) Signed-off-by: Mike Wake <[email protected]>
…avigation#4341) Signed-off-by: Mike Wake <[email protected]>
Use -R to invoke the regular expresion. Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
Signed-off-by: Mike Wake <[email protected]>
This allows setting a default failure error_code and message in nav2_system_tests behavior_tree dummy_action_server Signed-off-by: Mike Wake <[email protected]>
b305e08
to
30b36da
Compare
@aosmw I got the ping on the push - should I give this another look through or ready? |
Not ready, just rebasing and transferring between dev environments. Homing in on where I am likely to implement some system testing. maybe have something leading up to and/or over new year break. It's been functional for us for a while. Just still missing system tests to prove it in CI. ASIDE The following hint was finding and mixing previous devcontainer environments leading ... somehow to local_costmap timeouts and amcl not able to make progress.
https://docs.nav2.org/development_guides/devcontainer_docs/devcontainer_guide.html |
Basic Info
Description of contribution in a few bullet points
First pass at populating error_msg for all action result messages.
Description of documentation updates required from your changes
Adds some new error codes to FollowWaypoints + FollowGPSWaypoints
Future work that may be required in bullet points
For Maintainers: