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.

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 ros2 param #95

Merged
merged 7 commits into from
Jun 4, 2018
Merged

add ros2 param #95

merged 7 commits into from
Jun 4, 2018

Conversation

dirk-thomas
Copy link
Member

  • list outputs the names of all parameters, either for all nodes or a node passed as an argument
  • get outputs the value of a specific parameter, the type information can be suppressed
  • set sets a parameter, the type is being determined by guessing the primitive types, no array types atm
  • delete removed a specific parameter (at least it should but it seems that the service doesn't work for that use case).

@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Jun 1, 2018
@dirk-thomas dirk-thomas self-assigned this Jun 1, 2018
@dirk-thomas dirk-thomas requested a review from sloretz June 1, 2018 22:19
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Partial review. I used get and list so far and they seem to work fine.

future = client.call_async(request)
futures[node_name] = future
else:
print(node_name, 'not ready')
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are a few debug print statements. Use the node's logger for debug messages instead?

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 removed the debug output in 55a5948.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Jun 4, 2018

Tested this along ros2/rclcpp#489 and it works great 👍
(I haven't tested with many requests queued up, only one a a time)

I pushed a few nitpicks, feel free to revert them if they're not appropriate.

A few questions that come to mind:

  • Could we add the option to provide the type explicitly ?
    • e.g. if I want to pass to a string parameter a value that can be converted to another type ("0", "True", "42.42"...)
  • Is there something in the API preventing to set the array types ? or was it just to keep the scope reasonable?

@dirk-thomas
Copy link
Member Author

  • Could we add the option to provide the type explicitly ?
  • Is there something in the API preventing to set the array types ? or was it just to keep the scope reasonable?

Additional feature can be added anytime. You could add it to this PR right now or keep it for a future PR. I just wanted to get a first version of this verb in to be able to use / test the parameters more easily.

@mikaelarguedas
Copy link
Member

Additional feature can be added anytime. You could add it to this PR right now or keep it for a future PR. I just wanted to get a first version of this verb in to be able to use / test the parameters more easily.

Follow up is fine by me 👍 it's great to have a first version of this verb out!
I just wanted to make sure that there was not a technical blocker for implementing all parameter types in the other verbs, I didn't mean to appear requesting additional features in this PR :).

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for pounding on this 👍

@dirk-thomas dirk-thomas merged commit 17bc821 into master Jun 4, 2018
@dirk-thomas dirk-thomas deleted the ros2param branch June 4, 2018 16:41
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 4, 2018
@wjwwood
Copy link
Member

wjwwood commented Jul 27, 2018

Retroactive summary for this feature:

add ros2 param

Purpose / Use Cases (Why implement this feature?)

This was implemented to provide a way to set and get parameters of ROS nodes which have parameters and a parameter service (allowing parameters to be set/checked via ROS).

Design (How does it work?)

It provides a new "command" to the ros2 command line tool called param which has a variety of "verbs" that do different things like list, set, get, and delete.
These verbs operate on remote nodes using the ROS parameter interface, see: https://design.ros2.org/articles/ros_parameters.html

Inputs / Outputs / API (How to use it?)

The usage of the tool is documented in the help text with ros2 param -h.

There is a "public" API which implements the actions the tool can take (used internally by the tool), see:

# Copyright 2018 Open Source Robotics Foundation, Inc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants