Servo: Check frames are known before getting their TFs #612
Conversation
Codecov ReportBase: 50.95% // Head: 54.24% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #612 +/- ##
==========================================
+ Coverage 50.95% 54.24% +3.29%
==========================================
Files 378 191 -187
Lines 31643 20099 -11544
==========================================
- Hits 16122 10900 -5222
+ Misses 15521 9199 -6322
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| if (!current_state_->knowsFrameTransform(frame_name)) | ||
| { | ||
| std::string error_string = "Unknown frame: " + frame_name; | ||
| throw std::runtime_error(error_string); |
There was a problem hiding this comment.
Can be simplified: throw std::runtime_error{"Unknown frame: " + frame_name}; Also, I take it that changing the frames on the fly might be a possibility? Still, are there any alternatives to simply terminating a process if a frame cannot be found? Is it documented anywhere that start() can suddenly throw an exception? To prevent termination, someone has to catch that exception.
There was a problem hiding this comment.
There is a callback to change the robot command frame on the fly later.
I added a note about the exception in the header file. However, we might consider a change so start() returns a bool and we can return false instead of throwing?
| if (!current_state_->knowsFrameTransform(cmd.header.frame_id)) | ||
| { | ||
| rclcpp::Clock& clock = *node_->get_clock(); | ||
| RCLCPP_WARN_STREAM_THROTTLE(LOGGER, clock, ROS_LOG_THROTTLE_PERIOD, |
There was a problem hiding this comment.
Nit: this too can be simplified by rewriting: RCLCPP_WARN_STREAM_THROTTLE(LOGGER, *node_->get_clock(), ROS_LOG_THROTTLE_PERIOD, etc...
There was a problem hiding this comment.
This actually does not work. I don't understand exactly how ROS 2 clocks and timing works, but this has weird results:
It will compile, but some timestamps (seen in RCLCPP logging macros) turn into... not quite nullptr ? It is very strange
@tylerjw This change was indeed causing the tests to fail - because the original change skipped commands with an empty Fixed it, but I noticed just the servo tests ( |
henningkayser
added this to Review in progress
in Galactic/Rolling - 2.3.0 - September 10
henningkayser
removed this from Review in progress
in Galactic/Rolling - 2.3.0 - September 10
tylerjw
force-pushed
the
fix/check_servo_frames_before_using
branch
from
986c77c
to
e497736
Compare
|
Rebased this change to see if it will pass CI. If it does, I suggest we merge this change, as it fixes a known bug. @AndyZe do you have any opinion on this? |
tylerjw
force-pushed
the
fix/check_servo_frames_before_using
branch
from
e497736
to
ea39227
Compare
Description
Noticed this when I was looking at #611 originally. Servo should check the frames it needs to do TF math with exist in the robot state before trying to look them up.
Can test this before/after by: modifying this line to be an unknown frame, then running the servo teleop demo with keyboard (arrow keys) input. After this change, Servo will not crash if the command frame is invalid, but will instead print a warning and skip the command.
Note that this might break the test on
checkValidCommand()when it is re-enabled with #603. If that is merged first, I will rebase and make sure the test passes (and add a test case for this invalid frame)Checklist