Skip to content
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

Update controller_manager_plugin.cpp #3179

Merged
merged 3 commits into from
Jan 10, 2025
Merged

Update controller_manager_plugin.cpp #3179

merged 3 commits into from
Jan 10, 2025

Conversation

wizard-coder
Copy link
Contributor

Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager

Description

When using two ROS2 controls, the namespace is automatically detected, but the namespace is not properly applied when registering the action client.

For more details, please refer to #3150

Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager
@mikeferguson
Copy link
Contributor

@wizard-coder - I reviewed the ticket as well - can you post the contents of ros2_controllers.left.yaml either here or on the original ticket? (I see you have the full launch file posted, but not the config).

@mikeferguson
Copy link
Contributor

As a note - this logic has been touched quite a bit in the past:

@wizard-coder
Copy link
Contributor Author

The contents of the config file are as follows:

ros2_contollers.left.yaml

/**/controller_manager:
  ros__parameters:
    update_rate: 100 # Hz

    arm_controller:
      type: joint_trajectory_controller/JointTrajectoryController

    joint_state_broadcaster:
      type: joint_state_broadcaster/JointStateBroadcaster

/**/arm_controller:
  ros__parameters:
    joints:
      - left_joint0
      - left_joint1
      - left_joint2
      - left_joint3
      - left_joint4
      - left_joint5
      - left_joint6
    command_interfaces:
      - position
    state_interfaces:
      - position
    allow_nonzero_velocity_at_trajectory_end: true

/**/joint_state_broadcaster:
  ros__parameters:
    joints:
      - left_joint0
      - left_joint1
      - left_joint2
      - left_joint3
      - left_joint4
      - left_joint5
      - left_joint6

    interfaces:
      - position

ros2_controllers.right.yaml

/**/controller_manager:
  ros__parameters:
    update_rate: 100 # Hz

    arm_controller:
      type: joint_trajectory_controller/JointTrajectoryController

    joint_state_broadcaster:
      type: joint_state_broadcaster/JointStateBroadcaster

/**/arm_controller:
  ros__parameters:
    joints:
      - right_joint0
      - right_joint1
      - right_joint2
      - right_joint3
      - right_joint4
      - right_joint5
      - right_joint6
    command_interfaces:
      - position
    state_interfaces:
      - position
    allow_nonzero_velocity_at_trajectory_end: true

/**/joint_state_broadcaster:
  ros__parameters:
    joints:
      - right_joint0
      - right_joint1
      - right_joint2
      - right_joint3
      - right_joint4
      - right_joint5
      - right_joint6

    interfaces:
      - position

@wizard-coder
Copy link
Contributor Author

As a note - this logic has been touched quite a bit in the past:

In the case of Ros2ControlManager, since ns_ is initialized as an empty string, I believe it should be initialized from the ros_control_namespace parameter. However, in the case of Ros2ControlMultiManager, it first finds the namespaces and then initializes ns_ through the Ros2ControlManager(const std::string& ns) initialization function. Therefore, since ns_ is already set at this point, I believe the initialization via ros_control_namespace should be ignored.

@mikeferguson
Copy link
Contributor

However, in the case of Ros2ControlMultiManager, it first finds the namespaces and then initializes ns_ through the Ros2ControlManager(const std::string& ns) initialization function

There we go - that's the part I was missing as to how you were getting the ns_ set initially - thanks.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 45.62%. Comparing base (75286c7) to head (ef6a7c7).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...ontrol_interface/src/controller_manager_plugin.cpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3179      +/-   ##
==========================================
- Coverage   46.01%   45.62%   -0.39%     
==========================================
  Files         714      714              
  Lines       62304    62279      -25     
  Branches     7532     7528       -4     
==========================================
- Hits        28666    28406     -260     
- Misses      33471    33706     +235     
  Partials      167      167              

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

Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I had a similar patch a while back, but I was waiting until I get time to add tests 🫠

It would be really good to add integration tests for this package, I think this's the 3 PR that try to fix the namespacing for Ros2ControlManager 😿

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks!

@sjahr sjahr enabled auto-merge January 6, 2025 10:27
@sjahr sjahr added this pull request to the merge queue Jan 10, 2025
Merged via the queue into moveit:main with commit 2566467 Jan 10, 2025
9 checks passed
@sea-bass
Copy link
Contributor

sea-bass commented Jan 10, 2025

Does this need backports? I didn't look at this PR, so would appreciate @sjahr and @JafarAbdi if you guys could make that call here.

I kicked off the backport PRs and you guys can review/merge/close them

@sea-bass sea-bass added backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy labels Jan 12, 2025
mergify bot pushed a commit that referenced this pull request Jan 12, 2025
Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager

Co-authored-by: Sebastian Jahr <[email protected]>
(cherry picked from commit 2566467)

# Conflicts:
#	moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp
mergify bot pushed a commit that referenced this pull request Jan 12, 2025
Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager

Co-authored-by: Sebastian Jahr <[email protected]>
(cherry picked from commit 2566467)
JafarAbdi pushed a commit that referenced this pull request Jan 12, 2025
Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager

Co-authored-by: Sebastian Jahr <[email protected]>
(cherry picked from commit 2566467)

Co-authored-by: Seohyeon Ryu <[email protected]>
sjahr pushed a commit that referenced this pull request Jan 13, 2025
* Update controller_manager_plugin.cpp (#3179)

Fixing the bug where the namespace is not properly applied when using Ros2ControlMultiManager

Co-authored-by: Sebastian Jahr <[email protected]>
(cherry picked from commit 2566467)

# Conflicts:
#	moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

* Address conflict

---------

Co-authored-by: Seohyeon Ryu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants