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

[Node Arguments] Python nodes support parameters #50

Merged
merged 6 commits into from
Dec 13, 2018

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Dec 13, 2018

Python nodes in Crystal support parameters
ros2 param list requires fully qualified node name (ros2/ros2cli#178)

Python nodes in Crystal support parameters
ros2 param list requires fully qualified node name (will file bug on ros2cli)
@sloretz sloretz self-assigned this Dec 13, 2018
wjwwood
wjwwood previously approved these changes Dec 13, 2018
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Does the fully qualified name work in bouncy?

@sloretz
Copy link
Contributor Author

sloretz commented Dec 13, 2018

Does the fully qualified name work in bouncy?

Oops, it does not

Bouncy

$ ros2 run demo_nodes_cpp talker __params:=demo_params.yaml > /dev/null &
[1] 1308
$ ros2 param list talker
  a_string
  some_int
  some_lists.some_doubles
  some_lists.some_integers
$ ros2 param list /talker
Node not found

@sloretz sloretz dismissed stale reviews from nuclearsandwich and wjwwood December 13, 2018 01:20

command does not work in bouncy

@nuclearsandwich
Copy link
Member

In that case we probably want to split out and describe both somehow.

Perhaps something like

In bouncy, the name is not fully qualified.

ros2 param list talker

It's awkward, but I was trying to keep Crystal+ the default.

@nuclearsandwich
Copy link
Member

While I'm thinking about bouncy, should the C++ caveat line that was removed instead be changed to

Parameters support for Python nodes was added in Crystal. In Bouncy and earlier only C++ nodes are supported.

@sloretz
Copy link
Contributor Author

sloretz commented Dec 13, 2018

@nuclearsandwich how does it look now?

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. Apologies for the confusion of approving first and asking questions after.

@nuclearsandwich
Copy link
Member

We should revisit these changes now that ros2/ros2cli#179 has made the leading / optional across ros2cli verbs, including parameters ones.

@sloretz
Copy link
Contributor Author

sloretz commented Dec 13, 2018

We should revisit these changes now that ros2/ros2cli#179 has made the leading / optional across ros2cli verbs, including parameters ones.

@nuclearsandwich I confirmed ros2 param list talker works with latest crystal debs, and undid the changes to that part of the document.

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Thanks again for the continued iteration.

@sloretz sloretz merged commit 3d5e350 into master Dec 13, 2018
@sloretz sloretz deleted the sloretz/node-arguments-python-params branch December 13, 2018 19:04
@sloretz sloretz removed the in review label Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants