Start executing passthrough trajectories earlier than all points are transferred. #1313

urfeex · 2025-03-24T14:32:35Z

When transferring trajectories with many waypoints it can take quite a
while until all points are transferred from the controller to the
hardware interface, especially on lower hardware_interface update rates.

With this change, execution starts as soon as enough trajectory points
are transferred to fill 5 control cycles.

This PR effectively allows reducing the hardware_interface update_rate
when using the passthrough_trajectory_controller significantly. This
will reduce the networking requirements improving the overall user
experience.

codecov · 2025-03-24T14:40:07Z

Codecov Report

Attention: Patch coverage is 0% with 82 lines in your changes missing coverage. Please review.

Project coverage is 5.58%. Comparing base (1b121b7) to head (d4c475f).
Report is 415 commits behind head on main.

Files with missing lines	Patch %	Lines
ur_robot_driver/src/hardware_interface.cpp	0.00%	70 Missing ⚠️
...trollers/src/passthrough_trajectory_controller.cpp	0.00%	12 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1313      +/-   ##
========================================
+ Coverage   3.59%   5.58%   +1.99%     
========================================
  Files         13      31      +18     
  Lines        947    3113    +2166     
  Branches     152     379     +227     
========================================
+ Hits          34     174     +140     
- Misses       843    2937    +2094     
+ Partials      70       2      -68

Flag	Coverage Δ
unittests	`5.58% <0.00%> (+1.99%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

urmahp

Overall looks good, added some comments.

ur_robot_driver/src/hardware_interface.cpp

urmahp

Overall it looks good. Have small suggestions which is not really related to this PR, but is part of the touched code, so I stumbled across them.

urmahp · 2025-03-31T12:24:41Z

ur_robot_driver/src/hardware_interface.cpp

-      } else if (!has_velocities(trajectory_joint_velocities_) && has_accelerations(trajectory_joint_accelerations_)) {
-        ur_driver_->writeTrajectorySplinePoint(trajectory_joint_positions_[i], trajectory_joint_accelerations_[i],
-                                               trajectory_times_[i]);
      } else if (has_velocities(trajectory_joint_velocities_) && has_accelerations(trajectory_joint_accelerations_)) {


Are we sure that it is not possible to create a trajectory with points, that has no velocity specified, but has accelerations? As it is now, then we will just skip those points.
Would it make sense to do something along the lines of:
ur_driver_->writeTrajectorySplinePoint(trajectory_joint_positions_[i], urcl::vector6d_t{ 0, 0, 0, 0, 0, 0 }, trajectory_joint_accelerations_[i], trajectory_times_[i]);

urmahp · 2025-03-31T12:33:10Z

ur_robot_driver/src/hardware_interface.cpp

-    if (!has_velocities(trajectory_joint_velocities_) && !has_accelerations(trajectory_joint_accelerations_)) {
-      for (size_t i = 0; i < trajectory_joint_positions_.size(); i++) {
+    for (size_t i = point_index_sent; i < point_index_received; i++) {
+      if (!has_velocities(trajectory_joint_velocities_) && !has_accelerations(trajectory_joint_accelerations_)) {


Are we sure that positions is always going to be part of the points received from the forward controller?
If we can end up in a situation where positions is not part of the received points, then I think we should handle it more cleanly with a separate if statement and Logging an error.

In the current implementation that we have with the passthrough trajectory controller, that is given. But I do see the point that a better handling would be to catch that here. I would actually advocate for the same thing regarding the acceleration but no velocity. If the user wants to specify accelerations combined with 0 velocities that should be done explicitly.

I'll add catching and raising an error.

…ransferred. When transferring trajectories with many waypoints it can take quite a while until all points are transferred from the controller to the hardware interface, especially on lower hardware_interface update rates. With this change, execution starts as soon as enough trajectory points are transferred to fill 5 control cycles. This PR effectively allows reducing the hardware_interface update_rate when using the passthrough_trajectory_controller significantly. This will reduce the networking requirements improving the overall user experience.

If we miss the new trajectory, wen cannot resize the vectors appropriately.

Co-authored-by: Mads Holm Peters <[email protected]>

…transferred. (#1313) * Start executing passthrough trajectories earlier than all points are transferred. When transferring trajectories with many waypoints it can take quite a while until all points are transferred from the controller to the hardware interface, especially on lower hardware_interface update rates. With this change, execution starts as soon as enough trajectory points are transferred to fill 5 control cycles. This PR effectively allows reducing the hardware_interface update_rate when using the passthrough_trajectory_controller significantly. This will reduce the networking requirements improving the overall user experience. * Make sure we acknowledge a new trajectory before sending points If we miss the new trajectory, wen cannot resize the vectors appropriately. (cherry picked from commit 14f80ac) # Conflicts: # ur_controllers/src/passthrough_trajectory_controller.cpp # ur_robot_driver/urdf/ur.ros2_control.xacro

…transferred. (#1313) * Start executing passthrough trajectories earlier than all points are transferred. When transferring trajectories with many waypoints it can take quite a while until all points are transferred from the controller to the hardware interface, especially on lower hardware_interface update rates. With this change, execution starts as soon as enough trajectory points are transferred to fill 5 control cycles. This PR effectively allows reducing the hardware_interface update_rate when using the passthrough_trajectory_controller significantly. This will reduce the networking requirements improving the overall user experience. * Make sure we acknowledge a new trajectory before sending points If we miss the new trajectory, wen cannot resize the vectors appropriately.

…transferred. (backport of #1313) (#1335) * Start executing passthrough trajectories earlier than all points are transferred. When transferring trajectories with many waypoints it can take quite a while until all points are transferred from the controller to the hardware interface, especially on lower hardware_interface update rates. With this change, execution starts as soon as enough trajectory points are transferred to fill 5 control cycles. This PR effectively allows reducing the hardware_interface update_rate when using the passthrough_trajectory_controller significantly. This will reduce the networking requirements improving the overall user experience. * Make sure we acknowledge a new trajectory before sending points If we miss the new trajectory, wen cannot resize the vectors appropriately. Co-authored-by: Felix Exner <[email protected]>

urfeex force-pushed the buffering branch 2 times, most recently from 1f84b1d to 7ac040d Compare March 28, 2025 10:57

urfeex requested a review from a team March 28, 2025 11:28

urfeex marked this pull request as ready for review March 28, 2025 11:28

urmahp requested changes Mar 31, 2025

View reviewed changes

urfeex requested a review from urmahp March 31, 2025 12:02

urfeex changed the title ~~Start executing passthrogh trajectories earlier than all points are transferred.~~ Start executing passthrough trajectories earlier than all points are transferred. Mar 31, 2025

urmahp requested changes Mar 31, 2025

View reviewed changes

urfeex force-pushed the buffering branch from 9ad4fd0 to d4c475f Compare March 31, 2025 13:59

urfeex requested a review from urmahp March 31, 2025 13:59

urfeex and others added 6 commits April 2, 2025 13:14

Make sure we acknowledge a new trajectory before sending points

7695453

If we miss the new trajectory, wen cannot resize the vectors appropriately.

Apply suggestions from code review

984e934

Co-authored-by: Mads Holm Peters <[email protected]>

Remove comment that is no longer true

e1232c3

Remove non-used case from sending spline interpolation to the robot

a5f3e63

Catch two faulty cases and print an error instead

b48415b

urfeex force-pushed the buffering branch from d4c475f to b48415b Compare April 2, 2025 11:14

urmahp approved these changes Apr 4, 2025

View reviewed changes

urfeex added the backport-humble label Apr 7, 2025

urfeex merged commit 14f80ac into main Apr 7, 2025
7 of 10 checks passed

urfeex deleted the buffering branch April 7, 2025 05:05

mergify bot mentioned this pull request Apr 7, 2025

Start executing passthrough trajectories earlier than all points are transferred. (backport #1313) #1335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Start executing passthrough trajectories earlier than all points are transferred. #1313

Start executing passthrough trajectories earlier than all points are transferred. #1313

Uh oh!

urfeex commented Mar 24, 2025

Uh oh!

codecov bot commented Mar 24, 2025 •

edited

Loading

Uh oh!

urmahp left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

urmahp left a comment

Uh oh!

urmahp Mar 31, 2025

Uh oh!

urmahp Mar 31, 2025

Uh oh!

urfeex Mar 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Start executing passthrough trajectories earlier than all points are transferred. #1313

Start executing passthrough trajectories earlier than all points are transferred. #1313

Uh oh!

Conversation

urfeex commented Mar 24, 2025

Uh oh!

codecov bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

urmahp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

urmahp left a comment

Choose a reason for hiding this comment

Uh oh!

urmahp Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

urmahp Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

urfeex Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 24, 2025 •

edited

Loading