-
Notifications
You must be signed in to change notification settings - Fork 531
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
Add support for namespaces and respawn of crashed nodes #977
Conversation
df76d60
to
a1c3ef9
Compare
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 indentation after line 30 is broken. Please revert it to the current state.
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.
You changed the indentation from 2 spaces to 4. The only lines changed in this PR should be related to your feature
61a2413
to
ca3330f
Compare
Yes, indeed; should be fixed now |
<arg unless="$(var bson_only_mode)" name="binary_encoder" default="default"/> | ||
|
||
<arg unless="$(var bson_only_mode)" name="binary_encoder" default="default"/> |
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.
This line is doubled now. @sea-bass Is this even needed? This argument does not seem to be used anywhere
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 understand how i haven't see this.
The argument seems to be used in the rosbridge_websocket node
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 bson_only_mode
is used but the binary_encoder
defined here doesn't seem to be used
ca3330f
to
c01de67
Compare
My proposed alternative for namespaces is to use:
before the nodes are defined. |
Fix indentation Fix indentation Update rosbridge_server/launch/rosbridge_websocket_launch.xml Co-authored-by: Błażej Sowa <bsowa123@gmail.com>
7fb615a
to
f4c60db
Compare
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.
Changes as they are look good and non disruptive.
Seems like everything was already resolved with @bjsowa 's review.
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.
Seems fine to me. The only issue is that rosapi node uses global namespace for all its services, so spawning multiple rosapi nodes in different namespaces creates a collision in service names.
roslibjs has a similar issue where it uses global namespace for service clients:
https://github.com/search?q=repo%3ARobotWebTools%2Froslibjs%20rosapi&type=code
Unless fixed in both projects, it won't work properly.
Added parameters in the launch file to set a namespace and node respawn.
ros2 launch rosbridge_server rosbridge_websocket_launch.xml namespace:="ROBOT" respawn:=true
This allow multiple robot to host their own bridge on the same network.
The respawn allows the Node to restart if it crashes. We had some issue with the ROSAPI node crashing when trying to access "dead" node information or node with empty names.