Use correct namespace when evaluating parameter files for composable nodes #303
Conversation
…nodes * When resolving parameters from a file, use the full node namespace, combined with any base namespace provided by launch (e.g. from PushRosNamespace). * Handle edge case in 'to_parameters_list', when namespace equals '/'. * Fix bug in test fixture returning invalid node names that contain consecutive forward slashes. * Add tests to cover cases for loading parameters from file when namespace is provided by PushRosNamespace actions. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
Testing packages above launch_ros:
|
| @@ -293,16 +293,17 @@ def get_composable_node_load_request( | |||
| if parameters: | |||
| request.parameters = [ | |||
| param.to_parameter_msg() for param in to_parameters_list( | |||
| context, request.node_name, expanded_ns, | |||
| context, request.node_name, combined_ns, | |||
There was a problem hiding this comment.
This is the main bug.
| @@ -75,7 +75,7 @@ def to_parameters_list( | |||
| """ | |||
| parameters = [] # type: List[rclpy.parameter.Parameter] | |||
| node_name = node_name.lstrip('/') | |||
| if namespace: | |||
| if namespace and namespace != '/': | |||
There was a problem hiding this comment.
This handles an edge case where namespace == '/', which would result in a node name with a forward slash, e.g. /test_node_name. But, the rest of this function expects the slashes to be stripped.
| if request.node_namespace == '/': | ||
| response.full_node_name = f'/{request.node_name}' | ||
| else: | ||
| response.full_node_name = f'{request.node_namespace}/{request.node_name}' |
There was a problem hiding this comment.
This change is to avoid illegal node names (containing two forward slashes, e.g. //test_node_name).
Specifically, the change avoids the mock component container from reporting an illegal node name. If a real-world container does this, then I guess it will result in similar issues loading parameters (silently ignore parameters that don't match the node name). For example, if a container loads a node and reports an invalid name "//foo", then the following parameter will, silently, not be passed to the node: Maybe the following parameters file would work, but I didn't test it: IMO, we shouldn't go out of our way to support unexpected node naming conventions. We could go so far as to validate node names reported by containers and emit a warning that there's a problem with the container being launched. That probably deserves it's own PR though. |
delete-merged-branch
bot
deleted the
jacob/fix_composable_node_param_namespace
branch
namespace provided by launch (e.g. from PushRosNamespace).
PushRosNamespace actions.