Implement times for ros2 topic pub

[clalancette]

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

While doing some testing, it turned out to be useful to tell ros2 topic pub exactly how many times to publish a message. This is basically a generic version of the -1 flag that already exists, though that flag is left for backwards-compatibility.

I should point out that this is not the same as doing for i in seq 1 10 ; do ros2 topic pub ... ; done. The difference is that with the bash loop, a new node and publisher is created every time. This can be problematic for discovery, since it can happen that we bring up the node, publish, and destroy the node before connections are properly made. Doing it inside of ros2 topic pub itself neatly avoids this issue.

The one potentially flaky part of this is the test, which expects exactly 5 messages to be published. Depending on the platform and load, that may not be a reasonable assumption. Thoughts on how to improve that, while still testing that we are publishing a fixed number of times, are appreciated.

[mjcarroll]

The one potentially flaky part of this is the test, which expects exactly 5 messages to be published. Depending on the platform and load, that may not be a reasonable assumption. Thoughts on how to improve that, while still testing that we are publishing a fixed number of times, are appreciated.

Would it be sufficient to check the console output like:

publishing #1: std_msgs.msg.String(data='hello')
publishing #2: std_msgs.msg.String(data='hello')
publishing #3: std_msgs.msg.String(data='hello')
publishing #4: std_msgs.msg.String(data='hello')
publishing #5: std_msgs.msg.String(data='hello')

While that doesn't guarantee that the messages have been published, it would be a bit more robust.

[clalancette]

While that doesn't guarantee that the messages have been published, it would be a bit more robust.

Good idea! I'll go with that instead. Thanks.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[mjcarroll]

LGTM with Happy CI.

[dirk-thomas]

This check should happen at parse_args time as part of a custom type function.

[clalancette]

I ended up moving this check into main, alongside the other check for args.rate <= 0.

There is actually further work that could be done here to check for errors earlier. For instance, we could look up the message type/module and fill out the message all within main, before passing it down to the publish function. That would do less setup/teardown work in the case of an error. But I'll leave that for a future cleanup.

[dirk-thomas]

I would still suggest to move both checks into custom type functions to determine valid input at argument parsing time.

[clalancette]

As far as I can tell, this isn't possible. You can define a custom type callable in argparse, but that callable does not have access to the overall args structure. Even if it did, there is no way of knowing that you parsed all of the arguments by the time you got there. You can really only do this particular check at the end.

That being said, I did implement a custom type check for both rate and times that ensures that that they are positive float and non-negative integer, respectively.

[dirk-thomas]

I was referring to both individual checks which have been moved into type functions 👍

If both arguments can't be passed at the same time the arguments should be added to an exclusive group.

[clalancette]

Good call about the function names; fixed now. Also, I didn't know about argparse's mutually exclusive groups, thanks for that. I've now changed it to use them. I'll run another CI here, and this is ready for another review.

[clalancette]

New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

The macOS failure is a flake in topic_hz test, unrelated to this PR. So I think this is ready to go now.