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 actions tutorial page #169

Merged
merged 10 commits into from
May 21, 2019
Merged

Add actions tutorial page #169

merged 10 commits into from
May 21, 2019

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Apr 17, 2019

Contains action server tutorial in Python and placeholders for other tutorials.

Looking for early feedback before I continue with the other sections. Perhaps this is too verbose?
Also, I'm considering adding a separate tutorial package to go along with this tutorial (ideally this tutorial would live with that package).

Resolves #166

@dirk-thomas
Copy link
Member

I think the outline is good as is. While it gets more detailed at the bottom that is a good thing imo.

The GitHub preview might not give the right impression since it doesn't show the full code snippet. I assume that is going to be different via index.ros.org (?).

@jacobperron
Copy link
Member Author

The GitHub preview might not give the right impression since it doesn't show the full code snippet. I assume that is going to be different via index.ros.org (?).

When I build locally (make html) it looks okay to me. Although, CI is complaining about the :linenos: flag. Looks like doc8 doesn't support the flag.

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.

Added nitpicks, hopefully that's what you were looking for ;-)

Individual pages instead of sections on one page might make the tutorial less intimidating to a potential reader . A small scroll bar might give hope that they can complete something pretty quickly.

Let's walk through the following action server implementation (``fibonacci_action_server.py``):

.. code-block:: python
:linenos:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this whole block show on index.ros.org? It looks like it's missing from the github preview.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shows locally when I build. I'm assuming it will work on index.ros.org. It's a sphinx directive that GitHub doesn't seem to be able to handle. Is there a way I can confirm this will work on index before merging?

Contains action server tutorial in Python and placeholders for other tutorials.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Specifically, ignore an error related to the :linenos: directive.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

I've addressed the early review and separated the tutorials into different pages.
I also made an amendment to .travis.yml, adding an exception for the Action tutorials (f84391b). doc8 doesn't recognize the :linenos: directive.

I still need to add C++ tutorials and a Python action client tutorial.

I've included code snippets with highlighting for clarity.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

Both Python tutorials are ready for review. I've changed the style by trying to build up the code demo piece-by-piece. I think it it more instructive and clear this way.

The Python files build on each other (e.g. client_1.py builds on client_0.py) as the tutorials progress.
I think the best way to review this is the build the documentation locally and view as html (ie. make html).

I'll add the C++ examples in a follow-up so they can be reviewed separately.

You should see our logged message "Executing goal..." folowed by a warning that the goal state was not set.
By default, if the goal handle state is not set in the execute callback it assumes the *aborted* state.

We can use the method `succeed() <http://docs.ros2.org/latest/api/rclpy/api/actions.html#rclpy.action.server.ServerGoalHandle.succeeded>`_ on the goal handle to indicate that the goal was successful:
Copy link
Member Author

Choose a reason for hiding this comment

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

This link won't work until rclpy API docs are updated.

For more detailed information about ROS actions, please refer to the `design article <http://design.ros2.org/articles/actions.html>`__.

This document contains a list of tutorials related to actions.
For reference, after completing all of the tutorials you should expect to have a ROS package that looks like the package `action_tutorials <https://github.com/ros2/demos/tree/master/action_tutorials>`__.
Copy link
Member Author

Choose a reason for hiding this comment

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

This link won't work until the action_tutorials package is added.

@jacobperron
Copy link
Member Author

I've added a package containing the code used in these tutorials for reference: ros2/demos#339

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

It looks awesome @jacobperron !

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

@hidmic Thanks for the review and fixing my grammar :)

I believe I've addressed all of your comments.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM !

@jacobperron jacobperron merged commit e06f465 into master May 21, 2019
@delete-merged-branch delete-merged-branch bot deleted the action_tutorial branch May 21, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Actions Tutorials
4 participants