Skip to content
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

Handle exception in RobotStateDisplay and print warning to not crash RViz #1267

Conversation

Projects
None yet
3 participants
@christian-rauch
Copy link
Member

commented Dec 14, 2018

Description

The RobotStateDisplay plugin crashes RVIz if a robot state with a non-existing joint is provided. See: #1266

This PR simply catches the exception and shows a warning instead of forwarding the exception an have RViz crash.

@welcome

This comment has been minimized.

Copy link

commented Dec 14, 2018

Thanks for helping in improving MoveIt!

@v4hn
Copy link
Member

left a comment

Thanks for the pull-request!
Having RViz crash just because some validity checks are not in place is frustrating indeed...

@christian-rauch

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

@v4hn Done.

@v4hn

v4hn approved these changes Dec 16, 2018

Copy link
Member

left a comment

Looks good now.

Waiting for second review.

@rhaschke
Copy link
Collaborator

left a comment

After being approved by @v4hn, this PR had several remaining issues:

  • the exception was printed twice on the console, from here and here
  • the robot_state_ was left in an inconsistent state on an exception
  • the error status message wasn't reset upon a successful retrieval of the RobotState
  • the same exception crash occured for wrong DisplayTrajectory msgs

I fixed these issues, pushing directly to @christian-rauch's repo. @v4hn, please approve again.

@christian-rauch

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@rhaschke Thanks for pointing to the additional ROS_ERROR_NAMED message. I saw it on the terminal but couldn't track it down in the source.
I am fine with the changes.

@v4hn
Copy link
Member

left a comment

Indeed, your changes look good @rhaschke .

I'm not a fan of the additional make_shared though, see below.

robot_state::robotStateMsgToRobotState(state_msg->state, *robot_state_);
auto robot_state = std::make_shared<robot_state::RobotState>(robot_model_);
robot_state::robotStateMsgToRobotState(state_msg->state, *robot_state);
robot_state_.swap(robot_state);

This comment has been minimized.

Copy link
@v4hn

v4hn Dec 18, 2018

Member

Yes, this makes sense. However, I preferred to skip the allocation because the failure case is very uncommon but the allocation is required on every update.
How about using setToDefaultValues to enforce a safe state in case of error?

@v4hn v4hn removed the awaits 2nd review label Dec 18, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2018

@v4hn Can you have a final look, please? If you approve, I would like to squash some of the commits for cleanup before finally merging.

@v4hn

v4hn approved these changes Dec 19, 2018

@v4hn

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Looks good now.

rhaschke added a commit that referenced this pull request Dec 19, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2018

Merged in 47ff403 after some cleanup squashing.

@rhaschke rhaschke closed this Dec 19, 2018

@christian-rauch christian-rauch deleted the christian-rauch:fix_robot_state_joints branch Dec 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.