Fix Servo interruption

[AndyZe]

Tweak the interruption logic

[codecov]

Codecov Report

Merging #2314 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2314      +/-   ##
==========================================
- Coverage   57.96%   57.91%   -0.04%     
==========================================
  Files         327      327              
  Lines       25638    25633       -5     
==========================================
- Hits        14859    14844      -15     
- Misses      10779    10789      +10     

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/collision_check.cpp	81.58% <ø> (+0.57%)	⬆️
...oveit_servo/include/moveit_servo/collision_check.h	100.00% <100.00%> (ø)
...os/moveit_servo/include/moveit_servo/servo_calcs.h	100.00% <100.00%> (ø)
moveit_ros/moveit_servo/src/servo.cpp	72.04% <100.00%> (-0.91%)	⬇️
moveit_ros/moveit_servo/src/servo_calcs.cpp	62.15% <100.00%> (-0.59%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	69.45% <0.00%> (-1.63%)	⬇️
.../move_group_interface/src/move_group_interface.cpp	47.87% <0.00%> (-0.21%)	⬇️
... and 4 more

---

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 964ed5c...c022e90. Read the comment docs.

[tylerjw]

These changes look ok. I'm trying to clarify what they fix. Also, you need to run clang_format to get it to pass travis.

[AndyZe]

We could also discuss if we need both of these:

• stop()
• setPaused()

The difference seems to be, setPaused() allows the thread to keep spinning so the joints stay up to date. There is more overhead involved with stop() but it completely prevents spinning

Edit: I think I'll go ahead and delete stop(). The finer differences between stop() and setPaused() are probably confusing for users. Also, there is a lot of overhead involved in restarting after stop() and restarting seemed to be causing some issues in one of our projects (command vector length mismatch). Finally, stop() really doesn't reduce CPU load much compared to setPaused().

If reviewers disagree strongly, I'll just revert that commit

[AndyZe]

I'm working on making the tests more repeatable now. It seems like the issue had to do with initializing ROS in one executable for multiple tests. Separate executables for each test is 100% reliable from what I've seen so far. Will work on reducing duplicate code next.

[AndyZe]

OK, this is working well locally. Sorry to add so many lines but getting these tests to pass reliably has been really difficult.

I stuck a header file (common_test_setup.h) in the the /test folder. Not sure if there's any convention for where to put header files related to testing.

[tylerjw]

I really like what you are doing with the tests here. However, I feel that they don't belong in this PR. Would you mind splitting the changes to the tests into a separate PR from the one to change the logic for stopping?

[tylerjw]

It seems that there was some issue caused by the change of logic in the tests that prompted refactoring the tests. Could you help me understand what the reason was?

[AndyZe]

It seems that there was some issue caused by the change of logic in the tests that prompted refactoring the tests. Could you help me understand what the reason was?

Thanks for asking this question! It turns out the deletion of timer_.stop() is what was causing the flaky tests. I put that back in the destructor and tests are back to normal.

(originally I thought the tests on master branch were still flaky, but they aren't)

[tylerjw]

lgtm, tested with customer setup.