-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update python publisher tutorial with links to relevant docs #6181
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
base: rolling
Are you sure you want to change the base?
Update python publisher tutorial with links to relevant docs #6181
Conversation
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with green CI.
|
@christophebedard can i have a second review on this? |
| ~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| The first lines of code after the comments import ``rclpy`` so its ``Node`` class can be used. | ||
| The first lines of code after the comments import `rclpy <{package_link(rclpy)}>`__ so its `Node <{package_link(rclpy)}api/node.html>`__ class can be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use {package(rclpy)} for rclpy here. It is equivalent and a bit shorter:
| The first lines of code after the comments import `rclpy <{package_link(rclpy)}>`__ so its `Node <{package_link(rclpy)}api/node.html>`__ class can be used. | |
| The first lines of code after the comments import {package(rclpy)} so its `Node <{package_link(rclpy)}api/node.html>`__ class can be used. |
| from rclpy.node import Node | ||
| The next statement imports the built-in string message type that the node uses to structure the data that it passes on the topic. | ||
| The next statement imports the built-in `String <{interface_link(std_msgs/msg/String)}>`__ message type that the node uses to structure the data that it passes on the topic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just using {interface(std_msgs/msg/String)} so that it writes the full message name? Same below
| The next statement imports the built-in `String <{interface_link(std_msgs/msg/String)}>`__ message type that the node uses to structure the data that it passes on the topic. | |
| The next statement imports the built-in {interface(std_msgs/msg/String)} message type that the node uses to structure the data that it passes on the topic. |
|
|
||
| ``create_publisher`` declares that the node publishes messages of type ``String`` (imported from the ``std_msgs.msg`` module), over a topic named ``topic``, and that the "queue size" is 10. | ||
| Queue size is a required QoS (quality of service) setting that limits the amount of queued messages if a subscriber is not receiving them fast enough. | ||
| `create_publisher <{package_link(rclpy)}api/node.html#rclpy.node.Node.create_publisher>`__ declares that the node publishes messages of type `String <{interface_link(std_msgs/msg/String)}>`__ (imported from the `std_msgs.msg <{package_link(std_msgs)}__message_definitions.html>`__ module), over a topic named ``topic``, and that the "queue size" is 10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __message_definitions.html page is more of an implementation detail of rosdoc2, so I wouldn't hardcode it in the docs here.
We could add a macro that lets us link to this page for a given package so that we only define __message_definitions.html in one single place (i.e., in the implementation of that macro). However, in this case, I think we can simply avoid linking to that page.
| `create_publisher <{package_link(rclpy)}api/node.html#rclpy.node.Node.create_publisher>`__ declares that the node publishes messages of type `String <{interface_link(std_msgs/msg/String)}>`__ (imported from the `std_msgs.msg <{package_link(std_msgs)}__message_definitions.html>`__ module), over a topic named ``topic``, and that the "queue size" is 10. | |
| `create_publisher <{package_link(rclpy)}api/node.html#rclpy.node.Node.create_publisher>`__ declares that the node publishes messages of type {interface(std_msgs/msg/String)} (imported from the ``std_msgs.msg`` module), over a topic named ``topic``, and that the "queue size" is 10. |
| First the ``rclpy`` library is initialized, then the node is created, and then it "spins" the node so its callbacks are called. | ||
| First the `rclpy <{package_link(rclpy)}>`__ library is initialized, then the node is created, and then it "spins" the node (using `spin() <{package_link(rclpy)}api/init_shutdown.html#rclpy.spin>`__) so its callbacks are called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| First the `rclpy <{package_link(rclpy)}>`__ library is initialized, then the node is created, and then it "spins" the node (using `spin() <{package_link(rclpy)}api/init_shutdown.html#rclpy.spin>`__) so its callbacks are called. | |
| First the {package(rclpy)} library is initialized, then the node is created, and then it "spins" the node (using `spin() <{package_link(rclpy)}api/init_shutdown.html#rclpy.spin>`__) so its callbacks are called. |
| <exec_depend>std_msgs</exec_depend> | ||
| This declares the package needs ``rclpy`` and ``std_msgs`` when its code is executed. | ||
| This declares the package needs `rclpy <{package_link(rclpy)}>`__ and `std_msgs <{package_link(std_msgs)}>`__ when its code is executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This declares the package needs `rclpy <{package_link(rclpy)}>`__ and `std_msgs <{package_link(std_msgs)}>`__ when its code is executed. | |
| This declares the package needs {package(rclpy)} and {package(std_msgs)} when its code is executed. |
| ^^^^^^^^^^^^^^^ | ||
| You likely already have the ``rclpy`` and ``std_msgs`` packages installed as part of your ROS 2 system. | ||
| It's good practice to run ``rosdep`` in the root of your workspace (``ros2_ws``) to check for missing dependencies before building: | ||
| You likely already have the `rclpy <{package_link(rclpy)}>`__ and `std_msgs <{package_link(std_msgs)}>`__ packages installed as part of your ROS 2 system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| You likely already have the `rclpy <{package_link(rclpy)}>`__ and `std_msgs <{package_link(std_msgs)}>`__ packages installed as part of your ROS 2 system. | |
| You likely already have the {package(rclpy)} and {package(std_msgs)} packages installed as part of your ROS 2 system. |
This PR is OK, but, similar to what was mentioned in #4985 (comment), I would not systematically do this for other articles. Instead of writing `Node <{package_link(rclpy)}api/node.html>`__ classeverywhere to link to the API docs for {api(rclpy, rclpy.Node)} classand expands it to the correct page. For now, I think we're still generally working on making sure API docs are properly generated, which does mean changes to rosdoc2, but it mostly means fixing a lot of the packages (e.g., ros2/geometry2#857). Then we can think about how to create/find tooling to easily link to it from here. Otherwise the page we link to may not exist or the actual URL may change shortly after. |
|
I can look up for developing/incorporating a proper system for links in a seperate issue. Or help with something that's WIP. Can this be taken care on the URL side? Maintaining a database of URL matched with the doc links? |
Description
Update python publisher and subscriber tutorial with links to relevant documentation.
Addresses #4985
Did you use Generative AI?
No.
Additional Information
This first PR is to test the idea. Also see that rosdep's link is explicitly set because the macro doesn't work, see #6180.