Have set_hold_position() actually stop the robot

[egordon]

Addresses #683

In almost all cases, that a trajectory doesn't return a valid point implies that set_hold_position() (which defines an empty trajectory with no valid points) has been called somewhere. We should stop the robot in this case.

Note the existing TODO that it is unclear how to stop an effort-controlled robot.

Note also that we should not reach this code under any other circumstance, since all incoming trajectories are validated before copied to the realtime pointer. Therefore, reaching this code without set_hold_position() implies a bug in our validation code, and I would argue stopping the robot is reasonable behavior.

To send us a pull request, please:

• Fork the repository.
• Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
• Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
• Commit to your fork using clear commit messages.
• Send a pull request, answering any default questions in the pull request interface.
• Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

[codecov-commenter]

Codecov Report

Merging #684 (ae81d4f) into master (e7f9962) will increase coverage by 0.90%.
The diff coverage is 35.20%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
+ Coverage   35.78%   36.68%   +0.90%     
==========================================
  Files         189        7     -182     
  Lines       17570      676   -16894     
  Branches    11592      357   -11235     
==========================================
- Hits         6287      248    -6039     
+ Misses        994      134     -860     
+ Partials    10289      294    -9995     

Flag	Coverage Δ
unittests	36.68% <35.20%> (+0.90%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp	11.11% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp	42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp	46.55% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp	17.62% <12.08%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp	47.44% <47.36%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp	100.00% <100.00%> (ø)
...troller/include/diff_drive_controller/odometry.hpp	100.00% <100.00%> (ø)

... and 189 files with indirect coverage changes

[christophfroehlich]

@egordon I haven't seen your issue before, please have a look if #558 already adresses it?!

[bmagyar]

Partial to #558 as it has tests...

[christophfroehlich]

I had a detailed look: I think this one here does not solve the problem for all interface combinations: it sets command-interfaces of velocity/effort to zero at the end of the update method -> a position hold is not guaranteed, because the PIDs' outputs will be overriden.

[destogl]

Closing in favor of #613