Skip to content
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.

Sign up for GitHub

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 ros_arguments option to Node action #249

Merged
merged 1 commit into from
Jun 29, 2021
Merged

Add ros_arguments option to Node action #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions launch_ros/launch_ros/actions/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ def __init__(
exec_name: Optional[SomeSubstitutionsType] = None,
parameters: Optional[SomeParameters] = None,
remappings: Optional[SomeRemapRules] = None,
ros_arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
**kwargs
) -> None:
Expand Down Expand Up @@ -180,6 +181,9 @@ def __init__(
wildcard namespace (`/**`) and other specific parameter declarations
may overwrite it.

Using `ros_arguments` is equivalent to using `arguments` with a
prepended '--ros-args' item.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christophebedard I see the convenience BUT is it necessary? Most ROS arguments can be given in an idiomatic way (e.g. via remappings) already. If we need --log-level, I'd argue we should add it. CC @wjwwood @ivanpauno.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found #60 while answering an answers.ros.org question: https://answers.ros.org/question/380883/specify-log-level-on-ros2-node-in-launch-file/#380965. In this case, it seemed to confuse the person. They weren't sure whether they needed to use -- beause launch_ros appends --ros-args ... too (even if in practice rcl handles it just fine). I think having ros_arguments would make it much simpler, but yeah it's up to you the maintainers 😝

If we need --log-level

you mean like a LogLevel substitution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found #60 while answering an answers.ros.org question: https://answers.ros.org/question/380883/specify-log-level-on-ros2-node-in-launch-file/#380965. In this case, it seemed to confuse the person.

I see. I was wondering about the use case. If not having to write out --ros-args helps people, I'm onboard.

you mean like a LogLevel substitution?

Like a log_level argument that can be a substitution. But that's a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we can handle things better if the user explicitly tells us when something is a ros argument, so +1 from me.


:param: executable the name of the executable to find if a package
is provided or otherwise a path to the executable to run.
:param: package the package in which the node executable can be found
Expand All @@ -191,13 +195,15 @@ def __init__(
or dictionaries of parameters.
:param: remappings ordered list of 'to' and 'from' string pairs to be
passed to the node as ROS remapping rules
:param: ros_arguments list of ROS arguments for the node
:param: arguments list of extra arguments for the node
"""
if package is not None:
cmd = [ExecutableInPackage(package=package, executable=executable)]
else:
cmd = [executable]
cmd += [] if arguments is None else arguments
cmd += [] if ros_arguments is None else ['--ros-args'] + ros_arguments
# Reserve space for ros specific arguments.
# The substitutions will get expanded when the action is executed.
cmd += ['--ros-args'] # Prepend ros specific arguments with --ros-args flag
Expand All @@ -218,6 +224,7 @@ def __init__(
self.__node_namespace = namespace
self.__parameters = [] if parameters is None else normalized_params
self.__remappings = [] if remappings is None else list(normalize_remap_rules(remappings))
self.__ros_arguments = ros_arguments
self.__arguments = arguments

self.__expanded_node_name = self.UNSPECIFIED_NODE_NAME
Expand Down
10 changes: 9 additions & 1 deletion test_launch_ros/test/test_launch_ros/actions/test_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,12 @@ def _assert_launch_no_errors(self, actions):
ls.include_launch_description(ld)
assert 0 == ls.run()

def _create_node(self, *, parameters=None, remappings=None):
def _create_node(self, *, parameters=None, remappings=None, ros_arguments=None):
return launch_ros.actions.Node(
package='demo_nodes_py', executable='talker_qos', output='screen',
name='my_node', namespace='my_ns',
exec_name='my_node_process',
ros_arguments=ros_arguments,
arguments=['--number_of_cycles', '1'],
parameters=parameters,
remappings=remappings,
Expand Down Expand Up @@ -92,6 +93,13 @@ def test_launch_node_with_remappings(self):
for i in range(2):
assert expanded_remappings[i] == ('chatter', 'new_chatter')

def test_launch_node_with_ros_arguments(self):
node_action = self._create_node(ros_arguments=['--log-level', 'debug'])
self._assert_launch_no_errors([node_action])

cmd_string = ' '.join(node_action.process_details['cmd'])
assert '--ros-args --log-level debug' in cmd_string

def test_launch_required_node(self):
# This node will never exit on its own, it'll keep publishing forever.
long_running_node = launch_ros.actions.Node(
Expand Down