-
Notifications
You must be signed in to change notification settings - Fork 123
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
Conversation
Hi @bobbleballs, usually PRs are opened targeting master and are afterwards backported to older branches |
…ce issue described in ros-controls#127 Also remove superfluous ROS_ARGS_FLAG
71f23ee
to
a18aeb6
Compare
Hi @tonynajjar sorry, I had some issues with rebasing. I think I've sorted it now! |
There was a problem hiding this 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
Hi @ahcorde, |
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. |
@ahcorde Could you please take another look at this? |
Hi All! |
@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. |
There was a problem hiding this 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?
There was a problem hiding this 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
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? |
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 |
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. 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": You can see here my fork. Do you want me to open a new PR for the merge request? Thank you |
Hi @DoppiaEffe94, 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. |
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. |
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
After launching, if I check the node list I see /zray/robot_state_publisher, but I am getting the error
Additionally, checking ros2 service list I see
|
@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 |
@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? |
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. |
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? |
There was a problem hiding this 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.
@Mergifyio backport humble |
✅ Backports have been created
|
…181) Co-authored-by: ben.holden <[email protected]> Co-authored-by: Christoph Froehlich <[email protected]> (cherry picked from commit 828b5f2)
…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]>
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