Set multiple parameters

[ihasdapie]

Support setting multiple parameters at once using ros2 param set

See: #727 (comment)

This PR uses features from #716 (Diff: brianc/parameter_client...brianc/multiple_param_set)

$ ros2 param set -h

usage: ros2 param set [-h] [--spin-time SPIN_TIME] [-s] [--no-daemon] [--include-hidden-nodes] node_name parameter_name value

Set parameter

positional arguments:
  node_name             Name of the ROS node
  parameters            List of parameter name and value pairs i.e. "int_param 1 str_param hello_world"

options:
  -h, --help            show this help message and exit
  --spin-time SPIN_TIME
                        Spin time in seconds to wait for discovery (only applies when not using an already running daemon)
  -s, --use-sim-time    Enable ROS simulation time
  --no-daemon           Do not spawn nor use an already running daemon
  --include-hidden-nodes
                        Consider hidden nodes as well

Example usage:

ros2 param set /test_node int_param 12 str_param hello_world

[clalancette]

I feel like we shouldn't raise an exception here; previously, this would result in printing out a usage message and returning with shell result 2. I think we should try to emulate that.

[ihasdapie]

That's true -- printing out the whole backtrace and all is rather ugly. I've added an action to the new parameter argument list to maintain the same error behavior as before

ros2cli/ros2param/ros2param/verb/set.py

Lines 32 to 41 in 2e90009

	class RequireParameterPairAction(argparse.Action):
	"""Argparse action to validate parameter argument pairs."""
	def __call__(self, parser, args, values, option_string=None):
	if len(values) == 0:
	parser.error('No parameters specified')
	SystemExit(2)
	if len(values) % 2:
	parser.error('Must provide parameter name and value pairs')
	setattr(args, self.dest, values)

λ ros2 param set /add_two_ints_server
usage: ros2 param set [-h] [--spin-time SPIN_TIME] [-s]
                      [--no-daemon] [--include-hidden-nodes]
                      node_name [parameters ...]
ros2 param set: error: No parameter pairs specified
λ echo $?
2
λ ros2 param set /add_two_ints_server true
usage: ros2 param set [-h] [--spin-time SPIN_TIME] [-s]
                      [--no-daemon] [--include-hidden-nodes]
                      node_name [parameters ...]
ros2 param set: error: Must provide parameter name and value pairs

[clalancette]

These two blocks are very similar, with the exception of successful vs failed in the output string, and also using stderr or stdout for printing. It feels like they can be combined, though I'm not sure of that so just take this as a suggestion.

[ihasdapie]

I've combined them here but I feel like it's a little unnecessairly terse.

           for i, result in enumerate(results):
                print('Set parameter ' + parameters[i].name + ' ' +
                      'successful' if result.successful else 'failed' +
                      str(result.reason) if result.reason else '',
                      file=sys.stderr if result.successful else sys.stdout)

[clalancette]

Yeah, agreed. This is kind of hard to read. I'd say let's go back to the previous version (sorry).

[ihasdapie]

@clalancette Friendly ping -- I've made the requested changes, can you take a look?

[fujitatomoya]

I think test needs to be added in ros2cli/ros2param/test to set and get the parameters w/o error.

[ihasdapie]

I think test needs to be added in ros2cli/ros2param/test to set and get the parameters w/o error.

Done in 5547f87. There weren't any tests for the set verb before.

[clalancette]

I just rebased this and made some small fixes to it. With this, it is looking good to me but I'd like another opinion before I merge it. I'll go ahead and run CI, though.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[scpeters]

For backwards compatibility with ros2 param set node param_name value, I think the sequence of ros2 param set node param_name1 value1 param_name2 value2 makes sense, but it gets tricky to follow visually. I think the launch syntax of param:=value is a little easier to read when there are multiple parameters being set, but it's not clear how to do that in backwards compatible way.

Just an observation, not blocking

[clalancette]

For backwards compatibility with ros2 param set node param_name value, I think the sequence of ros2 param set node param_name1 value1 param_name2 value2 makes sense, but it gets tricky to follow visually. I think the launch syntax of param:=value is a little easier to read when there are multiple parameters being set, but it's not clear how to do that in backwards compatible way.

Yeah, I agree completely. The thing is I think the backwards compatibility is important.

We could do a thing where the first argument is name val, and all subsequent ones are name:=val, but I think that inconsistency is worse. We could also do a thing where we accept name val for a single argument with a warning, and then accept name:=val for arbitrary numbers of values without a warning. That would be more work, but would be more like launch and ros2 run as well.

I'm honestly not sure. @ros2/team thoughts?

[nuclearsandwich]

@ros2/team thoughts?

Perhaps it's possible to maintain the old behavior or at least emit a warning if a sequence of arguments is given and none of the arguments contain the := delimiter, thus indicating that it's a param value param value sequence rather than a param:=value param:=value sequence.

The thing is I think the backwards compatibility is important.

Breaking compatibility to improve usability is worthwhile.
I'm not really sure that tick-tock cycles help for command-line tools so I would be in favor of:

• Target Iron for this change (i.e. don't backport it)
• Update the Iron release docs with this as a Notable Change since last release
• Make an extra announcement on Discourse about this when it lands in Rolling

[ihasdapie]

Another option could be to have param value for setting single values and then param1:=value1 param2:=value2... for multiple parameters. That way we keep backwards compatability for single parameters and introduce the more readable syntax for multiple parameters.

EDIT: I realized this is exactly what @clalancette suggested, sorry. I think it'd be better to emit a warning for param value syntax and not support param1 value1 param2 value2 in lieu of bringing users onto param1:=value1... sequences