Skip to content

Add motion_primitives_forward_controller for interfacing motion primitive messages with hardware interfaces #1636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 79 commits into
base: master
Choose a base branch
from

Conversation

mathias31415
Copy link

This PR adds the motion_primitives_forward_controller, a controller for forwarding motion primitive commands to motion-primitive-capable hardware interfaces.
It was developed alongside the motion_primitive_ur_driver and is intended to support future hardware interfaces for additional robot manufacturers.
The corresponding PR for integrating the motion_primitive_ur_driver into the official Universal Robots ROS 2 driver can be found here: UniversalRobots/Universal_Robots_ROS2_Driver#1341.

The controller subscribes to MotionPrimitive.msg from the industrial_robot_motion_interfaces package. The MotionPrimitive.msg has been extended with additional helper types:

  • STOP_MOTION: Immediately interrupts execution and clears all queued primitives.
  • MOTION_SEQUENCE_START and MOTION_SEQUENCE_END: Define a sequence of primitives to be executed as a blended motion block.

The controller uses the following interfaces:

Command Interfaces

  • motion_type: Type of motion primitive (e.g., LINEAR_JOINT, LINEAR_CARTESIAN, CIRCULAR_CARTESIAN, etc.)
  • q1q6: Target joint positions for joint-based motion
  • pos_x, pos_y, pos_z: Target Cartesian position
  • pos_qx, pos_qy, pos_qz, pos_qw: Orientation quaternion of the target pose
  • pos_via_x, pos_via_y, pos_via_z: Intermediate via-point position for circular motion
  • pos_via_qx, pos_via_qy, pos_via_qz, pos_via_qw: Orientation quaternion of via-point
  • blend_radius: Blending radius for smooth transitions
  • velocity: Desired motion velocity
  • acceleration: Desired motion acceleration
  • move_time: Optional duration for time-based execution

State Interfaces

  • execution_status: Indicates the current execution state of the primitive.
  • ready_for_new_primitive: Boolean flag indicating whether the interface is ready to receive a new motion primitive

I'd appreciate any feedback or suggestions – thanks in advance!

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of stuff coming from templates in this PR and you either used some AI tool for coding or you are very good at writing AI-style comments.

Please try removing all commented code or fix them and remove all comments that add nothing to the code itself (which in this code is almost all comments...)

@destogl destogl marked this pull request as draft July 8, 2025 12:03
@destogl destogl marked this pull request as ready for review July 8, 2025 12:03
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

Attention: Patch coverage is 59.42408% with 155 lines in your changes missing coverage. Please review.

Project coverage is 85.62%. Comparing base (a10c564) to head (b2550d1).

Files with missing lines Patch % Lines
...oller/src/motion_primitives_forward_controller.cpp 39.25% 128 Missing and 19 partials ⚠️
...test/test_motion_primitives_forward_controller.hpp 89.06% 3 Missing and 4 partials ⚠️
...test/test_motion_primitives_forward_controller.cpp 97.82% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
- Coverage   86.44%   85.62%   -0.82%     
==========================================
  Files         123      129       +6     
  Lines       12259    12641     +382     
  Branches     1023     1070      +47     
==========================================
+ Hits        10597    10824     +227     
- Misses       1344     1475     +131     
- Partials      318      342      +24     
Flag Coverage Δ
unittests 85.62% <59.42%> (-0.82%) ⬇️

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

Files with missing lines Coverage Δ
...ontroller/motion_primitives_forward_controller.hpp 100.00% <100.00%> (ø)
...test_load_motion_primitives_forward_controller.cpp 100.00% <100.00%> (ø)
...otion_primitives_forward_controller_preceeding.cpp 100.00% <100.00%> (ø)
...test/test_motion_primitives_forward_controller.cpp 97.82% <97.82%> (ø)
...test/test_motion_primitives_forward_controller.hpp 89.06% <89.06%> (ø)
...oller/src/motion_primitives_forward_controller.cpp 39.25% <39.25%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

// Iterate over all command interfaces from the config yaml file
for (const auto & interface_name : params_.command_interfaces)
{
command_interfaces_config.names.push_back("motion_primitive/" + interface_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, all 25 command interfaces are given to the controller and then added with the motion_primitive prefix. The actual names of the interfaces don't matter as far as I can see from your implementation, they could also be i0 to i24. The prefix, however (the gpio group name), has to be exactly motion_primitive. That's not necessarily the case. As in your implementation for UR, the group name is ${tf_prefix}motion_primitive which would mean that this controller would not work if a tf_prefix is set. I would suggest to either

  • pass full interface names to the controller (such as $(var tf_prefix)motion_primitive/q1) or
  • pass both, the group name and each interface name as separate parameters or
  • Have the interface names fixed and only pass the group name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted it so that the full name is passed to the controller ($(var tf_prefix)motion_primitive/q1).

…atomic<bool> has_active_goal_ to ensure only one action goal is accepted at a time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants