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

Check for nullptr on getGlobalLinkTransform #611

Merged

Conversation

AdamPettinger
Copy link
Contributor

@AdamPettinger AdamPettinger commented Aug 13, 2021

Description

Checks for nullptr when looking up a link transformation. The LinkModel* is set as nullptr here if the link name isn't known. This PR makes it throw in this case, instead of Seg Faulting. This should probably be backported to Moveit1 as well

I found this by modifying this line to be an unknown frame, then running the servo teleop demo with keyboard (arrow keys) input

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #611 (22b4b05) into main (bf6357e) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #611      +/-   ##
==========================================
+ Coverage   46.96%   46.96%   +0.01%     
==========================================
  Files         185      185              
  Lines       19655    19659       +4     
==========================================
+ Hits         9229     9231       +2     
- Misses      10426    10428       +2     
Impacted Files Coverage Δ
...bot_state/include/moveit/robot_state/robot_state.h 85.32% <50.00%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf6357e...22b4b05. Read the comment docs.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this. At some point we should add a test for this.

@AndyZe
Copy link
Member

AndyZe commented Aug 14, 2021

Good catch!

Are you sure you want to throw a std::runtime_error, though?

I don't see that done anywhere else in robot_state.h or robot_state.cpp. I think it should be like this, instead:

throw Exception("Invalid link");

To match what's done here.

@AdamPettinger
Copy link
Contributor Author

Good catch!

Are you sure you want to throw a std::runtime_error, though?

I don't see that done anywhere else in robot_state.h or robot_state.cpp. I think it should be like this, instead:

throw Exception("Invalid link");

To match what's done here.

Seems good to me, switched over to that

@tylerjw tylerjw merged commit e0699e4 into moveit:main Aug 16, 2021
@AdamPettinger AdamPettinger deleted the fix/getGlobalLinkTransform_on_nullptr branch August 16, 2021 15:52
tylerjw pushed a commit to tylerjw/moveit2 that referenced this pull request Aug 27, 2021
Co-authored-by: AndyZe <andyz@utexas.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants