Change servo namespacing logic #2354
Conversation
| // joint topic parameter can be explicitly set to "/baz/my_joint_state_topic". | ||
| // **Note!** Make sure your planning scene monitor subscribes to correct joint states topic when using absolute | ||
| // joint state topic name *or* custom relative joint state topic name (for example, "my_joint_states"). | ||
| ros::NodeHandle nh_parent_ns = ros::NodeHandle(""); |
There was a problem hiding this comment.
Shouldn't this still be a private NodeHandle following the described logic?
There was a problem hiding this comment.
I may have used wrong terminology, so just to confirm that we are talking about the same thing: If the node would have name servo_node and it would exist within namespace foo. Then I would argue that the joint_states it listens to should be /foo/joint_states and not /foo/servo_node/joint_states. If the explanation is confusing, can you propose an alternative wording?
There was a problem hiding this comment.
I see, your solution totally makes sense
| cartesian_command_in_topic: delta_twist_cmds # Topic for incoming Cartesian twist commands | ||
| joint_command_in_topic: delta_joint_cmds # Topic for incoming joint angle commands | ||
| joint_topic: joint_states | ||
| status_topic: status # Publish status to this topic | ||
| command_out_topic: joint_group_position_controller/command # Publish outgoing commands here |
There was a problem hiding this comment.
Aren't the controller topics namespaced as well? A usual setup would have these at root level.
There was a problem hiding this comment.
Good point, I need to check the namespacing for this one.
There was a problem hiding this comment.
Thanks! Fixing the service namespacing is important. Please rebase.
Unfortunately, it looks like this tutorial does not work with the changes--
https://ros-planning.github.io/moveit_tutorials/doc/realtime_servo/realtime_servo_tutorial.html
I think the problem is, Servo is publishing on this topic: /servo_server/joint_group_position_controller/command
But the controller expects this topic: /joint_group_position_controller/command
So make sure topics in the yaml files have a leading /.
I'll need to test on our dual-arm projects after you rebase.
The service namespacing problem is solved by always advertising the services in the namespace of the node.
What if we're using the C++ interface, and two Servo objects are instantiated in the node? Might need to put the service topic in a yaml file, too :(
|
Overall I'm kind of lukewarm about this PR. It would be really nice to fix service namespacing but I think the other changes will lead to a lot of existing users who are missing |
…ace of the servo node.
… into node namespace.
…ple to use node namespace instead of the parent namespace.
|
The pose tracking PR brought in more changes than I realized. I have to go over those changes and see how they fit together with this PR. The pose tracking interface seems to utilize separate parameter namespace variable to its constructor while I would argue for using node handle. Sorry I didn't bring this up earlier, I simply hadn't considered namespacing back then. But I'll make the required changes here and then we can continue discussion when there is something more concrete to talk about. As for breaking changes, anyone who depends on And, if this PR grows much more, then maybe it is best to concentrate on just the services but I will at least try to get this right in one PR first. |
… more Servo instances are launched from a single node.
jliukkonen
force-pushed
the
change-servo-namespacing-logic
branch
from
e6d892f
to
6b63a8f
Compare
|
Well, this is a bit of a wall of text but I'm hoping this helps in the reviewing process. I listed below examples of how to use Servo with this PR and how the different parameters, topics and services end up looking like. As for the tutorials, I would rather concentrate on getting the namespacing right and once it is right, then it should be trivial to make the tiny fixes to the tutorials and examples that may still need updating. How to run two Servo instances in a single node:Example launch file: Parameters: One example per each instance. Topics: Note! Especially pay attention to Servo internal topics which are fixed by this PR to truly be internal to that specific Servo instance. Also, The Services: How to run single Servo instance in a single node:Parameters: Topics: Services: Multiple nodesRunning multiple nodes with multiple servos (or just one) in each of them is just a special case of the examples above. What about running the PoseTracker then? Well, it is exactly the same as the examples above except there is one more topic: More than one tracker? |
Codecov Report
@@ Coverage Diff @@
## master #2354 +/- ##
==========================================
- Coverage 56.29% 56.23% -0.06%
==========================================
Files 287 287
Lines 24433 24437 +4
==========================================
- Hits 13752 13739 -13
- Misses 10681 10698 +17
Continue to review full report at Codecov.
|
|
I'm waiting to test this on two more robot setups (one with dual arms). Looks good otherwise! |
| // joint topic parameter can be explicitly set to "/baz/my_joint_state_topic". | ||
| // **Note!** Make sure your planning scene monitor subscribes to correct joint states topic when using absolute | ||
| // joint state topic name *or* custom relative joint state topic name (for example, "my_joint_states"). | ||
| ros::NodeHandle nh_parent_ns = ros::NodeHandle(""); |
There was a problem hiding this comment.
I see, your solution totally makes sense
* Publish services correctly in the namespace of the node. * Set relative joint state topic names be relative to the parent namespace of the servo node. * Make parameter sub-namespace optional. By default load the parameters into node namespace. * Fix node handles for the servo server node and the cpp interface example to use node namespace instead of the parent namespace. * Fix formatting * Reading ee_frame_name parameter got lost somewhere along the way. Add it back. * Remove explicit parameter_ns variables and used namespaced node handles instead. * Fix internal namespace to truly be internal even in case where two or more Servo instances are launched from a single node. * Fix command_out_topic parameter in ur_simulated_config.yaml example config. * Apply namespacing fixes also to tests. * Shorten the comment about joint states topic names.
* Publish services correctly in the namespace of the node. * Set relative joint state topic names be relative to the parent namespace of the servo node. * Make parameter sub-namespace optional. By default load the parameters into node namespace. * Fix node handles for the servo server node and the cpp interface example to use node namespace instead of the parent namespace. * Fix formatting * Reading ee_frame_name parameter got lost somewhere along the way. Add it back. * Remove explicit parameter_ns variables and used namespaced node handles instead. * Fix internal namespace to truly be internal even in case where two or more Servo instances are launched from a single node. * Fix command_out_topic parameter in ur_simulated_config.yaml example config. * Apply namespacing fixes also to tests. * Shorten the comment about joint states topic names.
* Publish services correctly in the namespace of the node. * Set relative joint state topic names be relative to the parent namespace of the servo node. * Make parameter sub-namespace optional. By default load the parameters into node namespace. * Fix node handles for the servo server node and the cpp interface example to use node namespace instead of the parent namespace. * Fix formatting * Reading ee_frame_name parameter got lost somewhere along the way. Add it back. * Remove explicit parameter_ns variables and used namespaced node handles instead. * Fix internal namespace to truly be internal even in case where two or more Servo instances are launched from a single node. * Fix command_out_topic parameter in ur_simulated_config.yaml example config. * Apply namespacing fixes also to tests. * Shorten the comment about joint states topic names.
The namespacing logic seemed odd when running multiple servo servers within their own namespaces. This PR attempts to fix/change the namespacing logic within Servo to make it more intuitive. I listed before/after comparisons below to better illustrate the changes made in this PR.
Before
fooresulted to service topics like this:/foo/foo/<nodeName>/change_drift_dimensionswhen I would have expected simple/foo/<nodeName>/change_drift_dimensions.servo_server/delta_twist_cmds). And to be more precise, the topic names had to match the node name. I think it it better to keep relative topic names in config files without the requirement to use node name as part of them. This way user doesn't have to change config file topic names if the node name is changed in the launch file.ros::NodeHandle nh("~")the joint states subscriber would start listening to/<nodeName>/joint_statestopic which, I would argue, is rarely what one wants. And if more complex namespacing was used, there was no easy way (that I know of) to drop the<nodeName>part out of the full topic name just by modifying the topic name in the config file.After
/servo_server/<parameters>or use sub-namespace/servo_server/my_own_servo_param_namespace/<parameters>/foo/bar/servo/delta_twist_cmdsjust like before. Only the implicit requirement of specifying the calling node name as part of the topic names is removed, that is,delta_twist_cmdswill now suffice for as value forcartesian_command_in_topicparameter./joint_states, for more complex namespaces the subscribed topic might be, for example,/foo/bar/baz/joint_states. Note that user is still able to specify absolute topic names in config file just like before.In addition to the changes described in before/after examples, the servo server and cpp example files as well as the launch and config files have been modified to work with the changes in this PR.