Add support use_sim_time for ros2 topic hz/bw/pub.

[llapx]

Signed-off-by: Lei Liu Lei.Liu.AP@sony.com

[fujitatomoya]

address #752

[fujitatomoya]

@llapx thanks for taking care of this one. this PR is still W.I.P, right? adding node argument does not set simulation time, more implementation required.

[fujitatomoya]

CCed: @iuhilnehc-ynos @Barry-Xu-2018

[Barry-Xu-2018]

I think the current change is intermediate version.
@llapx please mark correct status of PR (e.g. Draft) to avoid reviewing incomplete change.

[llapx]

Hi All,

After some more tests, it confirmed that the code work well, in sim_time mode, there's need a sim clock to drive communication, I used ros2 bag record/play --clock to drive ros2 topic hz/bw/pub, thanks!

[fujitatomoya]

@llapx thanks for checking, can we add some test for each subcommand?

[llapx]

@llapx thanks for checking, can we add some test for each subcommand?

OK.

[fujitatomoya]

@Barry-Xu-2018 this is ready for review, can you help? i will do the same.

[Barry-Xu-2018]

this is ready for review, can you help? i will do the same.

Yes. I am reviewing this.

[iuhilnehc-ynos]

some nitpick and TODOs that we can improve.

[llapx]

Thanks for your feedbacks, I will update the code soon.

[fujitatomoya]

@llapx is this still draft?

[llapx]

@llapx is this still draft?

I have update the code, but some testcase will failed in jenkins, (my local test is OK), I'm finding the reason.

[llapx]

I found that most failed test cases not related to this PR, can anyone help to resolve this issue?
https://build.ros2.org/job/Rpr__ros2cli__ubuntu_jammy_amd64/128/testReport/

[fujitatomoya]

overall lgtm, minor comments.

[fujitatomoya]

@llapx i will start CI after my comments are resolved. thanks!

[iuhilnehc-ynos]

LGTM with my comments addressed.

[iuhilnehc-ynos]

I just run CI in linux:

The warning message reported by flake8, @llapx, could you check it?

  ./test/test_use_sim_time.py:16:1: I100 Import statements are in the wrong order. 'import functools' should be before 'import re'
  ./test/test_use_sim_time.py:37:1: I100 Import statements are in the wrong order. 'from rosgraph_msgs.msg import Clock' should be before 'from std_msgs.msg import String'
  ./ros2topic/verb/bw.py:33:1: I100 Import statements are in the wrong order. 'from argparse import ArgumentTypeError' should be before 'import sys'
  ./ros2topic/verb/bw.py:135:27: Q000 Double quotes found but single quotes preferred

[llapx]

I just run CI in linux:

The warning message reported by flake8, @llapx, could you check it?

  ./test/test_use_sim_time.py:16:1: I100 Import statements are in the wrong order. 'import functools' should be before 'import re'
  ./test/test_use_sim_time.py:37:1: I100 Import statements are in the wrong order. 'from rosgraph_msgs.msg import Clock' should be before 'from std_msgs.msg import String'
  ./ros2topic/verb/bw.py:33:1: I100 Import statements are in the wrong order. 'from argparse import ArgumentTypeError' should be before 'import sys'
  ./ros2topic/verb/bw.py:135:27: Q000 Double quotes found but single quotes preferred

Updated.

[iuhilnehc-ynos]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@wjwwood can you do final review?

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

Do we want backport this to humble? it extends sub-options. @clalancette

[clalancette]

Do we want backport this to humble? it extends sub-options. @clalancette

To put this in, we'll also have to put in #581 . I have a minor concern that doing the parameter_overrides there can introduce a small change in behavior, but I could be convinced it could be OK.

Regardless, I think we should let this sit in Rolling for a bit before we backport it, just to see if anyone runs into issues.

[fujitatomoya]

@clalancette can we add these feature to humble? dependent PR has been already in humble, i think this is useful (almost mandatory) for user development. what would you think?
if we have a go, we will manage the CI.

[fujitatomoya]

@clalancette friendly ping.

[fujitatomoya]

@Mergifyio backport humble

[fujitatomoya]

I am not sure why @Mergifyio does not work at this moment, create backport for humble #777

[wjwwood]

Yeah that's weird, it gave the 👍 like it noticed...

[Crola1702]

Hey @llapx @fujitatomoya.

This PR added test_use_sim_time.test_pub_times test. And it is failing flaky since it was added (Reported here: #772)

Log output:

FAIL: test_use_sim_time.TestROS2TopicUseSimTime.test_pub_times
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/ws/install_isolated/launch_testing/lib/python3.10/site-packages/launch_testing/markers.py", line 57, in _wrapper
    return func(self, *args, **kwargs)
  File "/tmp/ws/src/ros2/ros2cli/ros2topic/test/test_use_sim_time.py", line 124, in test_pub_times
    assert command.wait_for_shutdown(timeout=1)
AssertionError: assert False
 +  where False = <bound method ProcessProxy.wait_for_shutdown of <launch_testing.tools.process.ProcessProxy object at 0x7f0bc09f9330>>(timeout=1)
 +    where <bound method ProcessProxy.wait_for_shutdown of <launch_testing.tools.process.ProcessProxy object at 0x7f0bc09f9330>> = <launch_testing.tools.process.ProcessProxy object at 0x7f0bc09f9330>.wait_for_shutdown

It's failing on this line of code.

What do you think about this problem? Maybe we could increase the timeout in the test?

Reference build: https://build.ros2.org/view/Rci/job/Rci__nightly-debug_ubuntu_jammy_amd64/284/

[fujitatomoya]

What do you think about this problem? Maybe we could increase the timeout in the test?

yeah i do think so, comparing to other places, timeout could be too short.