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

Actions proposal #193

Merged
merged 76 commits into from
Mar 14, 2019
Merged

Actions proposal #193

merged 76 commits into from
Mar 14, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Oct 12, 2018

This PR is contains proposed changes and additions to the actions design doc started by @gbiggs in PR #183. It is targeted at gh-pages instead of gbiggs:gh-pages so the PR appears on this repo and the waffle board.

gbiggs and others added 30 commits August 1, 2018 11:31
Includes a state machine diagram for reference.
Added references to specific packages.
Rename ACTIVE -> EXECUTING and intermediate states to 'active' states.
Also added a transition from EXECUTING to CANCELED.
@jacobperron
Copy link
Member

Thanks for the review, everyone!

Updates to the design doc:

  • Added ACCEPTED state to the goal state machine.
    • This allows users to queue/list goals and know which ones have begun executing.
  • Removed transition EXECUTING -> CANCELED. This lets the core handle transitions after an accepted cancel request and the implementer confirm with 'set_canceled'.
  • Minor typo, grammar, consistency corrections.
  • Updated the dishwasher example to alleviate any confusion with IDs.
  • Client is now the sole entity responsible for generating goal IDs as UUIDs.
  • Added section in "Alternatives" regarding using multiple topics for feedback/status (instead of the proposed single topic per action server).

TODO:

  • Add section summarizing differences between ROS 1 and ROS 2
    • Any differences in terminology
    • API differences side-by-side
    • Example from MoveIt!
  • More sequence diagram examples

articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Show resolved Hide resolved
articles/actions.md Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
@gbiggs
Copy link
Member

gbiggs commented Oct 30, 2018

Also, we could have rosbag be aware of actions as a communication primitive and record the right things depending on your preferences (feedback and status only, requests and replies, etc...).

I think this is a requirement of making actions a first-class citizen in ROS2. All the tools, not just ros2 action, should be action-aware.

@sloretz
Copy link
Contributor Author

sloretz commented Oct 31, 2018

Would this need an additional topic, or would the client side set up background thread that calls get_result() to wait for completion, then calls the callback as its final action?

@gbiggs If I understand correctly I don't think a topic or a background thread are needed. At the rcl layer the client would send the "get result" service request as soon as it learns the goal is accepted. When the "get result" service response is received the service becomes ready in the waitset. The executor in rclcpp or rclpy would call a callback in spin() to handle the result.

articles/actions.md Outdated Show resolved Hide resolved
ruffsl added a commit that referenced this pull request Nov 29, 2018
To better secure and segment access control of the different ROS subsystems, specifically to avoid the mapping of ROS2 actions to DDS topics from colliding with those of ROS2 topics and services, actions are to be allocated their own prefix to facilitate simpler policy profile permissions. Additionally, this and helps to prevent crossover of information flow in the case of more general permission prefix expressions.  
Contex: #193 (comment)
@jacobperron jacobperron mentioned this pull request Jan 9, 2019
2 tasks
@sloretz sloretz mentioned this pull request Jan 29, 2019
24 tasks
* Update authors
* Update description of Cancel Service response
* Minor wording change related to feedback and status topics
Add arrow depicting user execution method being notified of cancel event

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks generally good, and like what we implemented. There are a few typos that I pointed out, and there are some unresolved conversations. In order to not lose what is in those conversations while merging, I would suggest to either:

  1. Comment on the conversations and declare them resolved, or
  2. Open new issues to track the parts that we haven't addressed in the first implementation of actions

articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
articles/actions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I'm going to approve, but there is one long outstanding discussion that is not resolved: #193 (comment) . @jacobperron I'd appreciate a comment to close out that thread, a new issue if there is further to discuss, or a clarification to the document before merge. Thanks.

@jacobperron
Copy link
Member

I'm going to approve, but there is one long outstanding discussion that is not resolved: #193 (comment) . @jacobperron I'd appreciate a comment to close out that thread, a new issue if there is further to discuss, or a clarification to the document before merge. Thanks.

Thanks, @clalancette! I made brief comment pointing to the continuation of the discussion in #203. Although it has been closed, I think we can postpone opening up another ticket until the issue is raised again or we decide it should be addressed.

@jacobperron jacobperron merged commit bbfa861 into gh-pages Mar 14, 2019
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Mar 14, 2019
@jacobperron jacobperron deleted the actions_proposal branch March 14, 2019 19:37
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.