-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 rs_launch.py #2764
Update rs_launch.py #2764
Conversation
eb7643e
to
4c53382
Compare
I've edited a commit. command
topic list
command
topic list
|
I also recommend adding this parameter to the Node parameters, so when run "ros2 param list" you will see this parameter along with all the nodes parameter, like camera_name. |
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.
Left some comments, please fix and update the PR
@@ -22,6 +22,7 @@ | |||
|
|||
|
|||
configurable_parameters = [{'name': 'camera_name', 'default': 'camera', 'description': 'camera unique name'}, | |||
{'name': 'namespace', 'default': '', 'description': 'namespace for camera'}, |
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.
I don't want to surprise current users if they pull from this branch and build again.
Most of them used to see /camera/camera
when launching our node, so I would prefer to set the default value in the namespace to "camera"
, to keep this behavior.
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.
Also, I think it is better to name this parameter camera_namespace
to make it more clean for users that it defines a namespace for the camera.
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.
sure let's go with camera_namespace
4c53382
to
6b02bb3
Compare
6b02bb3
to
114350d
Compare
@SamerKhshiboun |
114350d
to
c6faad4
Compare
So final one is right one I think
`namespace does not suffix. The format what I expected shall be this.
|
Hi @hyunseok-yang ,
So, could you update the |
Like you mentioned, in case of multiple cameras are connected, multiple camera could be separated by only I understood intention of As I told above, what I expected on topic list is like below.
However if the suffix attached after namespace
How can I separate the multi-robot scenaro to distinguish cameras for each robot? You mentioned that _param_name_suffix Could you explain more specific scenario? |
1- We agree we need our suffix to distinguish between two nodes parameters when using rs_multi_camera_launch.py You will get this when listing the nodes in anther terminal:
And this is wrong. The solution for this is to do the following:
And ROS will append the namespace to the camera name, then we will get:
|
Hi @hyunseok-yang, Do you want both camera nodes to be under the same namespace ? |
That's correct. When I command this. os2 launch realsense2_camera rs_launch.py align_depth.enable:=true rgb_camera.profile:=640x480x6 depth_module.profile:=640x480x6 camera_namespace:=test camera_name:=cam1 I got this.
If I do this, only I got these topics without camera name.
|
No problem :)
Yes It's why I insisting this launch script.
|
do you run can you please verify your changes with |
Sure, after colcon build and setup.bash, I just run below command.
and I got this
this branch
|
Oh Now I got it. You did In case of me, I got this.
It's a /namespace/camera_name/camera_name But this is what this package expected since namespace is "namespace/camera_name" |
If you get a node that is all topics should be the node name and then a string can you please run |
Sure,
|
What did you get if run |
Just in case, I removed binary package And still got same result. |
Hi @hyunseok-yang , I need to clarify a bit more about The idea is to run the |
OK, Let me know if it is clarified to you.
My idea is like this. What do you think?
|
To conclude everything: A namespace can include nodes, topics, etc.. Today, when launching with: We get this output, and we want to keep like this, because many users have assumption on node names and topic names The first /camera is always the namespace so the node name is and this should be like this always. the topics are What I suggest is here: in rs_launch.py edit namespace in both places:
in rs_multi_camera_launch.py, edit local_parameters to be:
Then, you can run this example from your terminal: ( I have D455 and D435i cameras)
and then you will get two nodes (ros2 node list):
and you will get these topics:
If that good for you, please do the suggested changes and commit, and I will approve this PR and merge it. |
As you mentioned this will NOT include camera name on topics.
Is that your intention? So whatever camera_name is passed on launcher, it will not affect topic name right? It it so, we have to append a camera_name on for examples, I think it is little weired that the camera |
I understand that this is weird, but your proposal is doing the same weirdness under the hood (in the code itself) when you append the Anyway, I think the best solution should be enhancing other places in the code, where we define the topic names. What do you think ? |
I didn't intend it. I appended it because code are written in this packges.
If you can edit the code as what we desired, I would be happy :). So you mean, I will get below result, isn't it? with this launch command
THEN I will follow your way. After all of modifictaion, is it still required |
Lets start with your PR, and then I will continue in a separated PR (to add the camera name to the topic name) Please fix two things:: in
in
|
OK. just moment |
c6faad4
to
2ad2519
Compare
Please check, I modified the commit as what you require. |
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.
Approved
2ad2519
to
37cb7da
Compare
- Add namespace launch argument
37cb7da
to
16d49e0
Compare
@SamerKhshiboun Please check with latest commit 16d49e0 |
25a1191
into
IntelRealSense:ros2-development
add namespace launch argument