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 Action CLI #214

Merged
merged 20 commits into from
Apr 14, 2019
Merged

Add Action CLI #214

merged 20 commits into from
Apr 14, 2019

Conversation

jacobperron
Copy link
Member

Created a new package, ros2action, defining the command action. This PR adds implementation for the following verbs:

  info       Print information about an action
  list       Output a list of action names
  send_goal  Send an action goal
  show       Output the action definition

Connects to #202
Requires ros2/rcl#411
Requires ros2/rclpy#306

Contains ros2cli command 'action' with verbs: list and show.
The list verb lists action names for any running action servers and action clients.
The show verb prints the definition for a given action type.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Prints a list of node names that have an action client or server for a given action name.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
The tool shouldn't need to know details about the implementation of actions.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label Apr 3, 2019
@jacobperron
Copy link
Member Author

I still need to add auto-completion and do more testing.

Running CI to find any issues early:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@Karsten1987
Copy link
Contributor

Not sure how trivial it is, but somehow echoing the feedback after submitting a goal would be extremely handy I believe.
Maybe the sending the goal can return an ID which can then be used to reference the feedback or so.

@jacobperron
Copy link
Member Author

Currently, the send_goal verb is blocking until the goal completes and echos the sent goal, result, and optional feedback. For example:

$ ros2 action send_goal --feedback /fibonacci example_interfaces/Fibonacci "order: 5"
Waiting for an action server to become available...
Sending goal:
     order: 5

Feedback:
    sequence: [0, 1, 1]

Feedback:
    sequence: [0, 1, 1, 2]

Feedback:
    sequence: [0, 1, 1, 2, 3]

Feedback:
    sequence: [0, 1, 1, 2, 3, 5]

Result:
    sequence: [0, 1, 1, 2, 3, 5]

Goal finished with status: SUCCEEDED

I agree that also echoing the goal ID would be useful.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

This should be ready for review.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 4, 2019
@Karsten1987
Copy link
Contributor

The command line tool looks great to me. I can send a command, get the feedback and see the action definitions. 👍

Only nitpick I had was for the ros2 action info command. I can't really figure out why we would need to have -c as an option. The other thing is that ros2 action info <topic> is different from ros2 action /<topic>. I believe as with other ros2cli tools, the forward slash should be added automatically if suited.

Karsten1987
Karsten1987 previously approved these changes Apr 5, 2019
Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I'll give you a 👍 , but you might want to have somebody look over the code who has a more critical eye on Python code than me :)

ros2action/package.xml Show resolved Hide resolved
ros2action/ros2action/verb/__init__.py Outdated Show resolved Hide resolved
ros2action/setup.py Show resolved Hide resolved
@jacobperron
Copy link
Member Author

The other thing is that ros2 action info is different from ros2 action /. I believe as with other ros2cli tools, the forward slash should be added automatically if suited.

I was trying to mimic ros2 topic info, which also has a -c option. I'll look into adding the forward slash.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This also has the side-effect of making the forward slash optional for the action name.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

@Karsten1987

ros2 action info is different from ros2 action /. I believe as with other ros2cli tools, the forward slash should be added automatically if suited.

I think I've addressed this in 0d13d00.

Also, now printing the goal UUID in hex format (8ef62e0)

@Karsten1987
Copy link
Contributor

The forward slash situation was addressed. 👍

I have two more remarks though:

  • ros2 action show: Would be great to have a -t, --types option just like ros2 topic list to have an idea of the actions and their types. That also makes calling ros2 action show easier.
  • ros2 action send_cancel: Now that send_goal returns a ID for the action, do you think it's feasible to add a cancel command which takes this ID and thus sends out a cancel request. I think that could be helpful.

@jacobperron
Copy link
Member Author

Thanks @Karsten1987!

ros2 action show: Would be great to have a -t, --types option just like ros2 topic list to have an idea of the actions and their types. That also makes calling ros2 action show easier.

I plan to make this feature as part of a new interface command as described in #202 (comment). Expect a follow up PR with this.

ros2 action send_cancel: Now that send_goal returns a ID for the action, do you think it's feasible to add a cancel command which takes this ID and thus sends out a cancel request. I think that could be helpful.

This reminded me that the intention was to cancel the goal if the user terminates the send_goal command before an action completes (I'll look into adding that to this PR).

I can also see the value in a separate cancel verb for interacting with a goal not started by the CLI tool, but I think we can save it for a follow-up ticket. Maybe good to discuss as part of #202.

Wrapped send goal logic in try-finally clause.
This ensures that any active goal will be canceled before the CLI command terminates and also ensure that the ROS node is shutdown.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron dismissed Karsten1987’s stale review April 11, 2019 19:11

I've updated the send goal logic to account for a SIGINT or errors.

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.

LGTM with nitpicks fixed and green CI. It works fine, including tab completion, on my machine.

ros2action/test/test_pep257.py Outdated Show resolved Hide resolved
ros2action/test/test_flake8.py Outdated Show resolved Hide resolved
ros2action/test/test_copyright.py Outdated Show resolved Hide resolved
ros2action/ros2action/verb/show.py Outdated Show resolved Hide resolved
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

ros2action/package.xml Outdated Show resolved Hide resolved
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

I would rather only catch expected exceptions here - basically the two handle before within the function. That way in case of an unexpected exception you still get a valuable stackable.

Done 6a62cb8

@jacobperron
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit a463030 into master Apr 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the ros2action branch April 14, 2019 14:31
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Apr 14, 2019
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

4 participants