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

Fix namespacing for multiple instances of gazebo_ros2_control plugin #181

Merged
merged 13 commits into from
Dec 23, 2024

Conversation

bobbleballs
Copy link
Contributor

Fixes the issue described in #127
It appears that when adding __ns tags to ros args on nodes that have already been namespaced, something odd happens.
I'm not too sure what happens on the rclcpp node side of things but removing the additional namespace tags on the args seems to fix the issue for me at least.
I'm running Humble on 22.04 so I've only tested against the humble branch.
I also removed the additional RCL_ROS_ARGS_FLAG in the arguments because I don't think it was doing anything.
Cheers Ben

@tonynajjar
Copy link
Contributor

tonynajjar commented Feb 24, 2023

Hi @bobbleballs, usually PRs are opened targeting master and are afterwards backported to older branches

@bobbleballs bobbleballs changed the base branch from humble to master February 24, 2023 13:37
@bobbleballs bobbleballs changed the base branch from master to humble February 24, 2023 13:38
…ce issue described in ros-controls#127

Also remove superfluous ROS_ARGS_FLAG
@bobbleballs bobbleballs changed the base branch from humble to master February 24, 2023 13:45
@bobbleballs
Copy link
Contributor Author

Hi @tonynajjar sorry, I had some issues with rebasing. I think I've sorted it now!

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

can you provide a simple example of how to use the namespace with your changes? Maybe add some text in the README.md

@bobbleballs
Copy link
Contributor Author

Hi @ahcorde,
I haven't changed any of the ways that you would use Namespaces before with this plugin, but now it works with multiple instances of the same plugin/robot description.
I've added to the README.md here
Thanks!

@bobbleballs bobbleballs requested a review from ahcorde March 18, 2023 12:02
@mauriz
Copy link

mauriz commented May 22, 2023

Hi there, I think this patch needs to be merged. If you look at the bottom of this thread , there is an example with multiple robots whose controllers had problems with the namespace.

@splatter96
Copy link

@ahcorde Could you please take another look at this?
I was stuck on this issue for multiple days until i stumbled upon the previously mentioned thread. The patch in this MR fixed my problem with multi robot simulations in gazebo. It would be nice if this patch can be incorporated into the upcoming releases.

@DoppiaEffe94
Copy link

Hi All!
Has the mentioned fork already been merged into the master branch?
I'm having issue as well on creating multiple instances of the same robot in gazebo simulation.
Thanks.

@christophfroehlich
Copy link
Contributor

@mauriz @splatter96 @DoppiaEffe94 can you please add your review to this PR (In the "Files changed" tab in the upper right corner: "Review Changes"? Compile the plugin from source, and test it with different setups.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

The description sound reasonable to me.
I tested the existing examples (without any namespaces), maybe anyone can provide a working example with two namespaced robots?

doc/index.rst Show resolved Hide resolved
doc/index.rst Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
Copy link

@mauriz mauriz left a comment

Choose a reason for hiding this comment

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

Sorry that is a quick review, just minor comments. See if that is reasonable

@christophfroehlich
Copy link
Contributor

Sorry that is a quick review, just minor comments. See if that is reasonable

Thanks for proof-reading. There is a little button in the code comment UI to include the code inside your comment, where you can propose your changes. This is then a one-click action to apply your changes for us, could you please change your comments above?
Have you actually tested the new version, does it fix the mentioned problem?

@mauriz
Copy link

mauriz commented Mar 6, 2024

Ok sorry I am new to this review system. I will do that as well as code checking asap, no earlier than a couple of days though.. sorry

@DoppiaEffe94
Copy link

Hi @christophfroehlich !

I had the time to test the compilation and running of this PR.

Somehow the provided fork from does not compile in my ROS2 Humble version.
For sake of precision, I just forked the package from the "humble" branch and removed the mentioned lines, without touching the README.md for now.

With the above steps, now the package compiles without any issue and the namespacing problem seems to be solved, as you can see below:

Example for husky robot with namespace "husky_1":
gazebo_ros2_control_husky_1

and "husky_2":
gazebo_ros2_control_husky_2

You can see here my fork.

Do you want me to open a new PR for the merge request?

Thank you

@bobbleballs
Copy link
Contributor Author

Hi @christophfroehlich !

I had the time to test the compilation and running of this PR.

Somehow the provided fork from does not compile in my ROS2 Humble version. For sake of precision, I just forked the package from the "humble" branch and removed the mentioned lines, without touching the README.md for now.

Hi @DoppiaEffe94,
Could you give details on why this branch doesn't compile? It's currently merging against master so that could be why. This will be fixed when we backport.

Unfortunately I've now moved on from the position that I was at that was allowing me to test this regularly. But it was working when I left! I don't currently have a ROS install or the time to check things.

If possible we should test against master or rolling, but I understand that can be difficult.
Hopefully we can get this merged soon as it's a blocker for quite a few workflows!

@christophfroehlich
Copy link
Contributor

Do you want me to open a new PR for the merge request?

No you don't have to make a PR for humble. If it is merged to rolling we'll backport it to humble.

Ideally, someone can provide an example for gazebo_ros2_control_demos with two namespaced robots, where everyone can test this.

@machineIntelligence
Copy link

machineIntelligence commented Mar 11, 2024

Hi there @christophfroehlich. I am using Humble and Gazebo 11 and I am trying to get a gazebo_ros2_control example running using the Plankton-uuv_simulator repo found here. I can run all of the examples in the repo, but none of them use ROS2 control. Here is a snippet of my URDF

 <xacro:zray_base
      namespace="$(arg namespace)"
      inertial_reference_frame="$(arg inertial_reference_frame)">
      <gazebo>
        <plugin name="$(arg namespace)_uuv_plugin" filename="libuuv_underwater_object_ros_plugin.so">
          <fluid_density>1028.0</fluid_density>
          <namespace>namespace</namespace>
          <flow_velocity_topic>hydrodynamics/current_velocity</flow_velocity_topic>
          <debug>0</debug>
          <xacro:zray_hydro_model namespace="$(arg namespace)"/>
        </plugin>  
        <plugin name="gazebo_ros2_control" filename="libgazebo_ros2_control.so">
          <parameters>$(find zray_description)/config/controller_configuration.yaml</parameters>
            <robot_param>$(arg namespace)/robot_description</robot_param>
            <robot_param_node>$(arg namespace)/robot_state_publisher</robot_param_node>   
          <ros>
            <namespace>$(arg namespace)</namespace>
            <remapping>/tf:=tf</remapping>
            <remapping>/tf_static:=tf_static</remapping>
          </ros>             
        </plugin>
     </gazebo>
  </xacro:zray_base> 

After launching, if I check the node list I see /zray/robot_state_publisher, but I am getting the error

[gzserver-2] [ERROR] [1710189413.862622929] [zray.gazebo_ros2_control]: zray/robot_state_publisher service not available

Additionally, checking ros2 service list I see

/zray/robot_state_publisher/describe_parameters
/zray/robot_state_publisher/get_parameter_types
/zray/robot_state_publisher/get_parameters
/zray/robot_state_publisher/list_parameters
/zray/robot_state_publisher/set_parameters
/zray/robot_state_publisher/set_parameters_atomically

@christophfroehlich
Copy link
Contributor

@machineIntelligence you haven't written if you have compiled the code from this PR or speaking of the released version? From your gzserver output, it seems that there is a leading / missing: zray/robot_state_publisher instead of /zray/robot_state_publisher.

@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jul 18, 2024

@DoppiaEffe94 how far did you get with it? could you provide a demo for gazebo_ros2_control_demos including two robot descriptions already provided with this package?

@DoppiaEffe94
Copy link

Hello @christophfroehlich.

Sorry for the delay, I was really busy and with a lot of projects going on.

I will set a demo in the next days and get back to you.

@ros-gy
Copy link

ros-gy commented Oct 9, 2024

Leaving a note here, because this is a fix that is needed for my multi-robot simulation on Humble and 22.04. @ahcorde @DoppiaEffe94 Any progress lately?

doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Tested it once again with the two documented approaches, and pushed my examples as well.

@christophfroehlich
Copy link
Contributor

@Mergifyio backport humble

Copy link
Contributor

mergify bot commented Dec 21, 2024

backport humble

✅ Backports have been created

@ahcorde ahcorde merged commit 828b5f2 into ros-controls:master Dec 23, 2024
5 checks passed
mergify bot pushed a commit that referenced this pull request Dec 23, 2024
…181)

Co-authored-by: ben.holden <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
(cherry picked from commit 828b5f2)
ahcorde added a commit that referenced this pull request Jan 7, 2025
…181) (#398)

Co-authored-by: ben.holden <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
(cherry picked from commit 828b5f2)

Co-authored-by: Ben Holden <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
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.

9 participants