[JTC] Reject trajectories with nonzero terminal velocity

[christophfroehlich]

As suggested by @bmagyar, the new trajectory should be even ignored if the last velocity point is nonzero.

• This is valid for both interfaces (topic + actions).
• I think there might be use-cases, where this is intentionally (maybe receiving topics from a MPC with moving horizon, where the end of the trajectory is not a steady state). Therefore, I added a parameter to be able to deactivate this behavior. To avoid breaking changes, I deactivated this by default now but give a deprecation warning if allow_nonzero_velocity_stop==true.

I also added a test for that, please review #603 first.

[codecov-commenter]

Codecov Report

Merging #567 (ff3096c) into master (e7f9962) will decrease coverage by 3.31%.
The diff coverage is 26.60%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
- Coverage   35.78%   32.48%   -3.31%     
==========================================
  Files         189        7     -182     
  Lines       17570      665   -16905     
  Branches    11592      357   -11235     
==========================================
- Hits         6287      216    -6071     
+ Misses        994      157     -837     
+ Partials    10289      292    -9997     

Flag	Coverage Δ
unittests	32.48% <26.60%> (-3.31%)	⬇️

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	12.50% <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%> (ø)
...troller/include/diff_drive_controller/odometry.hpp	20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp	39.22% <35.50%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp	100.00% <100.00%> (ø)

... and 189 files with indirect coverage changes

[destogl]

As suggested by @bmagyar, the new trajectory should be even ignored if the last velocity point is nonzero.

I don't agree with this at all. If we have velocity only input, we want to continue moving between trajectories. This is not so unusual scenario, e.g., when we have joystick input. I would rather like to see a timeout if we have velocity-only input to stop the input.

[christophfroehlich]

Ok, I did not think of using JTC with Joystick, but rejecting it always did not seem to make sense for me too.
So you mean we should even not hold the position automatically but only after some timeout reached?

[destogl]

So you mean we should even not hold the position automatically but only after some timeout reached?

Exactly, I think we want to avoid run away if only velocity is used.

[christophfroehlich]

now I'm confused :D maybe we should discuss this in the next WG meeting
I think there are different use-cases, and you have others in mind than I do..

[BarisYazici]

any update merging this PR?

[christophfroehlich]

We recently decided to merge several smaller changes (#567 #558 and some optional time-out for topic interface) for JTC into a feature branch, test them thoroughly together, and merge them then together into master.
I fear that this will take some more weeks to happen.

[christophfroehlich]

@BarisYazici you could help out by reviewing #558 and this PR. I think everyone can do this without special permissions (goto Files changed -> Review Changes on top right corner)

[christophfroehlich]

I've split this PR into two, the more important one is #608

[destogl]

I've split this PR into two, the more important one is #608

@christophfroehlich in which order should this be merged?

[christophfroehlich]

the order listed in #607 would be fine

[mergify]

This pull request is in conflict. Could you fix it @christophfroehlich?

[mergify]

This pull request is in conflict. Could you fix it @christophfroehlich?

[christophfroehlich]

The failing test is not related to this PR, but shows the flakiness of the JTC tests 😕