Avoid segfault and improve error warnings#603
Conversation
rhaschke
left a comment
rhaschke
left a comment
There was a problem hiding this comment.
I don't yet see the need for these extra checks. Can you please elaborate on your error condition.
|
|
||
| link = scene->getCurrentState().getRigidlyConnectedParentLinkModel(ik_pose_msg.header.frame_id); | ||
|
|
||
| if (!link) { |
There was a problem hiding this comment.
knowsFrameTransform(), a few lines above, should have validated that the frame exists.
Under what circumstances do you run into a segfault here?
I cannot imagine a valid frame that returns NULL here.
There was a problem hiding this comment.
There seem to be cases under which getRigidlyConnectedParentLinkModel returns a nullptr. Are they all covered by knowsFrameTransform?
There was a problem hiding this comment.
Not sure if I am understanding it correctly. I tried to add comments to the code to make it more comprehensible in https://github.com/moveit/moveit2/pull/2969/files. Do you think they are accurate? My current understanding is that this function returns a nullptr when there is nothing connected to the parent or when the transform between the parent body and the given frame is unknown.
There was a problem hiding this comment.
There seem to be cases under which getRigidlyConnectedParentLinkModel returns a nullptr. Are they all covered by
knowsFrameTransform?
Yes, they should be covered by knowsFrameTransform. If not, that's probably a bug. Please state your concrete problem.
There was a problem hiding this comment.
My current understanding is that this function returns a nullptr when there is nothing connected to the parent or when the transform between the parent body and the given frame is unknown.
IMO, this function should return NULL only if the given frame is unknown. Note that this function finds the parent link, which frame is connected to.
|
|
||
| if (!link) { | ||
| RCLCPP_WARN_STREAM( | ||
| LOGGER, fmt::format("ik frame '{}' is not rigidly connected to any link", ik_pose_msg.header.frame_id)); |
There was a problem hiding this comment.
The error message doesn't make sense: an existing frame needs to be connected to some link.
|
|
||
| if (!link) { | ||
| RCLCPP_WARN_STREAM( | ||
| LOGGER, fmt::format("ik frame '{}' is not rigidly connected to any link", ik_pose_msg.header.frame_id)); |
There was a problem hiding this comment.
Would this be more precise?
| LOGGER, fmt::format("ik frame '{}' is not rigidly connected to any link", ik_pose_msg.header.frame_id)); | |
| LOGGER, fmt::format("ik frame '{}' is not connected to any parent link", ik_pose_msg.header.frame_id)); |
There was a problem hiding this comment.
As I said, I expect this to return NULL if and only if frame doesn't exist. Do you have other evidence?
There was a problem hiding this comment.
@sjahr: I'm still curious to learn about your concrete issue.
In which case does getRigidlyConnectedParentLinkModel returns NULL, which is not handled before by knowsFrameTransform?
There was a problem hiding this comment.
Sorry, I cannot share more info on this at the moment 😞 I'll try debugging it further and hopefully figure out what the real source behind this is.
There was a problem hiding this comment.
I'm not expecting a detailed error analysis, but ideally a code snippet reproducing your segfault - I assume you observed a segfault in some code.
There was a problem hiding this comment.
Yes, unfortunately I don't have access to the code that causes the error and it seems to happen only in a HW setup. If you'd prefer that I can close this PR for now and come back later with more solid changes.
|
|
||
| namespace { | ||
|
|
||
| const rclcpp::Logger LOGGER = rclcpp::get_logger("moveit_task_constructor_visualization.execute_task_solution"); |
There was a problem hiding this comment.
Please revert those changes too.
|
Closing this for now and I'll open a PR / issue when I have a better understanding of the problem or a solution. Thanks for your feedback! |
2 participants
Check for nullptr in computeIK and print warnings when returning a nullptr