Fix crash if MoveIt Servo collision check is disabled

[schornakj]

Description

If the Servo collision check parameter is set to false, CollisionCheck's rclcpp::TimerBase::SharedPtr timer_ member is never initialized. Since ~CollisionCheck() calls timer_->cancel(), requesting that Servo start if collision checking was not enabled causes a crash.

This fixes the crash by checking if timer_ was actually set before trying to cancel it.

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]

Codecov Report

Merging #568 (755b321) into main (faee9a4) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   46.73%   46.71%   -0.01%     
==========================================
  Files         184      184              
  Lines       19685    19686       +1     
==========================================
- Hits         9198     9195       -3     
- Misses      10487    10491       +4     

Impacted Files	Coverage Δ
...oveit_servo/include/moveit_servo/collision_check.h	0.00% <0.00%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	74.72% <0.00%> (-1.13%)	⬇️

---

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 faee9a4...755b321. Read the comment docs.

[jliukkonen]

Why is the timer shared_ptr in the first place? In MoveIt1 it is just ros::Timer. This will fix the problem but I think the fix could also be unifying the timer initialization to match the MoveIt1.

[schornakj]

Why is the timer shared_ptr in the first place? In MoveIt1 it is just ros::Timer. This will fix the problem but I think the fix could also be unifying the timer initialization to match the MoveIt1.

In ROS1, the NodeHandle's createTimer function only returns a ros::Timer. The equivalent function in ROS2 is Node's create_wall_timer, which only returns a shared_ptr. ROS2 made a deliberate decision to interact with publishers, subscribers, timers, etc. through shared_ptrs, and while the rationale behind that is still debatable (unique_ptr would be preferable in many cases) that's the approach which is favored by the API.

[AndyZe]

Thanks! Good catch.