Comments

I might be wrong but based on my understanding we can only set one parameter currently using ros2 param set and hence setting params in an atomic fashion will not make sense using ros2 param set.

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service. Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

@deepanshubansal01 deepanshubansal01 requested a review from adityapande-1995

June 23, 2022 14:50

@clalancette

Copy link

Contributor

clalancette commented Jun 23, 2022

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service.

The problem with that, though, is that --atomic there only makes sense for setting parameters, not other service calls. So I don't think the flag belongs on ros2 service call.

Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

This we should definitely do. The other thing we could do is to fix up ros2 param set so that it can take multiple parameters; at that point, it makes sense to add the --atomic flag.

@deepanshubansal01

Copy link

Contributor

deepanshubansal01 commented Jun 23, 2022 •

edited

Loading

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service.

The problem with that, though, is that --atomic there only makes sense for setting parameters, not other service calls. So I don't think the flag belongs on ros2 service call.

Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

This we should definitely do. The other thing we could do is to fix up ros2 param set so that it can take multiple parameters; at that point, it makes sense to add the --atomic flag.

I see, makes sense!

@ihasdapie

Copy link

Member Author

ihasdapie commented Jun 23, 2022

Right, I definitely wasn't thinking when I put this PR together 😅. Will update this PR to support multiple parameters & param load --atomic

@clalancette

Copy link

Contributor

clalancette commented Jun 23, 2022

Will update this PR to support multiple parameters & param load --atomic

I'll suggest you do two separate PRs; the first one to add multiple param support to set, and then a second one (maybe this one) to add the --atomic flag. They are logically independent things, so they should have different PRs.

@ihasdapie ihasdapie force-pushed the brianc/atomic_parameter_set branch from 5bd9a70 to 13a8d16 Compare

June 23, 2022 21:57

@ihasdapie ihasdapie mentioned this pull request

Jun 23, 2022

Set multiple parameters #728

Open

@ihasdapie

Copy link

Member Author

ihasdapie commented Jun 23, 2022

ros2 param set with multiple parameters PR opened: #728

This PR is based off that PR (#728) and now implements --atomic for set and load verbs.

Diff: brianc/multiple_param_set...brianc/atomic_parameter_set

@audrow audrow changed the base branch from master to rolling

June 28, 2022 14:23

@clalancette clalancette assigned ihasdapie

Jul 14, 2022

ihasdapie added 2 commits

July 22, 2022 15:52

@ihasdapie


 multiple param set

dfb3b8b

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>

@ihasdapie


 --atomic flag for load & set

fb6efc1

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>

@ihasdapie ihasdapie force-pushed the brianc/atomic_parameter_set branch from 13a8d16 to fb6efc1 Compare

July 22, 2022 22:54

fujitatomoya

fujitatomoya reviewed

Jul 28, 2022

View reviewed changes

ros2param/ros2param/api/__init__.py

@@ -50,6 +50,10 @@ def load_parameter_file(*, node, node_name, parameter_file, use_wildcard):

parameters = list(parameter_dict_from_yaml_file(parameter_file, use_wildcard).values())

rclpy.spin_until_future_complete(node, future)

response = future.result()

Copy link

Collaborator

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

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

i think this call will raise exception if there is an actual exception.

ros2param/ros2param/api/__init__.py

future = client.load_parameter_file_atomically(parameter_file, use_wildcard)

parameters = list(parameter_dict_from_yaml_file(parameter_file, use_wildcard).values())

rclpy.spin_until_future_complete(node, future)

response = future.result()

Copy link

Collaborator

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

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

same here

ros2param/ros2param/api/__init__.py

raise RuntimeError('Exception while calling service of node '

f'{node_name}: {future.exception()}')

if response.result.successful:

Copy link

Collaborator

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

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

if response.result.successful is false, user can know that loading parameter operation has been failed?

ros2param/ros2param/api/__init__.py

f'{node_name}: {future.exception()}')

if response.result.successful:

msg = 'Set parameters {} successful'.format(' '.join([i.name for i in parameters]))

Copy link

Collaborator

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

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

this whole operation is atomic, and user knows parameters as input, so i am not sure if we need print all parameters again here. probably this command prints the result only?

ros2param/ros2param/api/__init__.py

client.wait_for_services(timeout_sec=5.0)

future = client.set_parameters_atomically(parameters)

rclpy.spin_until_future_complete(node, future)

response = future.result()

Copy link

Collaborator

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

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

same here.

Labels

None yet

4 participants

@ihasdapie @deepanshubansal01 @clalancette @fujitatomoya

Comments

Conversation

@ihasdapie ihasdapie commented Jun 23, 2022

Uh oh!

deepanshubansal01 commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clalancette commented Jun 23, 2022

Uh oh!

deepanshubansal01 commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihasdapie commented Jun 23, 2022

Uh oh!

clalancette commented Jun 23, 2022

Uh oh!

ihasdapie commented Jun 23, 2022

Uh oh!

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

@fujitatomoya fujitatomoya Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deepanshubansal01 commented Jun 23, 2022 •

edited

Loading

deepanshubansal01 commented Jun 23, 2022 •

edited

Loading