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 option to wait for two subscriptions #4674

Merged

Conversation

DaveCollinsJr
Copy link
Contributor

Since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

…rial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@DaveCollinsJr lgtm, can you address a nitpick?

@@ -249,14 +249,17 @@ Input the full command like so:

.. code-block:: console

ros2 topic pub --once /turtle1/cmd_vel geometry_msgs/msg/Twist "{linear: {x: 2.0, y: 0.0, z: 0.0}, angular: {x: 0.0, y: 0.0, z: 1.8}}"
ros2 topic pub --once -w 2 /turtle1/cmd_vel geometry_msgs/msg/Twist "{linear: {x: 2.0, y: 0.0, z: 0.0}, angular: {x: 0.0, y: 0.0, z: 1.8}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

so the prerequisites is that turtle nodes are up and running, but i think that is okay to have -w option here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in my experience (running all of this in Docker) the -w 2 option is mandatory. If you don't use it, the results are inconsistent: The topic pub begins running as soon as it sees the first subscriber. Therefore: Sometimes the turtle moves (and you see nothing in the topic echo window), sometimes the turtle does not move (and you see something in the topic echo window). The only way I was able to guarantee the behavior the tutorial is trying to demonstrate is to use -w 2


``--once`` is an optional argument meaning “publish one message then exit”.

``-w 2`` is an optional argument meaning “wait for two matching subscriptions”. This is needed because we have both turtlesim and the topic echo subscribed
Copy link
Collaborator

Choose a reason for hiding this comment

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

one sentence per line, missing last period.

Suggested change
``-w 2`` is an optional argument meaning “wait for two matching subscriptions”. This is needed because we have both turtlesim and the topic echo subscribed
``-w 2`` is an optional argument meaning “wait for two matching subscriptions”.
This is needed because we have both turtlesim and the topic echo subscribed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK will fix.

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.

So I kind of think that the use of --once here is a mistake to begin with, and adding in the -w 2 is going in the wrong direction.

This is supposed to be the beginner tutorial. For first-time users using ros2 topic pub, the fact that we immediately hit them with 2 command-line options, the topic name, the topic type, and the args, seems like quite a lot.

I think we should do something entirely different here. I think we should rewrite this whole section to start with:

  ros2 topic pub /turtle1/cmd_vel geometry_msgs/msg/Twist "{linear: {x: 2.0, y: 0.0, z: 0.0}, angular: {x: 0.0, y: 0.0, z: 1.8}}"

That will automatically start publishing at 1 Hz, and will publish no matter how many subscribers are available. Once we've shown the user the simplest invocation of the command, we can then introduce the --once command later. What do you think?

@fujitatomoya
Copy link
Collaborator

I am okay with #4674 (review), @DaveCollinsJr can you adjust the PR?

@DaveCollinsJr
Copy link
Contributor Author

@clalancette, @fujitatomoya : I have updated the PR to rearrange the tutorial as suggested. Now the --rate 1 option is first.

Then it moves into the --once option which, in my experience requires the -w 2 option to work consistently.

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.

Thanks! This is looking at lot more like I think it should. I've left a few things to improve.

@DaveCollinsJr
Copy link
Contributor Author

OK @clalancette - I believe I've hit your requested changes. Nice: I didn't realize that the "publish at 1 Hz" was the default.

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.

Other than one nit, I really like this revamp.

…Understanding-ROS2-Topics.rst

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette added the backport-all backport at reviewers discretion; from rolling to all versions label Oct 2, 2024
@clalancette clalancette merged commit 9d2907e into ros2:rolling Oct 2, 2024
4 checks passed
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)
clalancette pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)

Co-authored-by: Dave Collins <davecollinsjr@centurylink.net>
clalancette pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)

Co-authored-by: Dave Collins <davecollinsjr@centurylink.net>
clalancette pushed a commit that referenced this pull request Oct 2, 2024
* Add option to wait for tso subscriptions since, if following the tutorial to the letter, you will currently have both turtlesim and topic echo subscribed. If you do not do this, sometimes the turtle will move, sometimes it wont - depending on which subscriber the command finds first.

* wip. Requested to change the flow a bit

* As suggested, moved the continuous publishing version to be first, then go into the --once version.

* added some changes recommended by clalancette

* remove annoying blank line

* Update source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9d2907e)

Co-authored-by: Dave Collins <davecollinsjr@centurylink.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-all backport at reviewers discretion; from rolling to all versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants