Improved conversion of time values between ROS and DDS formats

[asorbini]

This PR applies changes similar to ros2/rmw_connext#491 to properly handle value RMW_DURATION_INFINITE and RMW_DURATION_UNSPECIFIED when converting time values between ROS and DDS formats.

The conversion to ROS format uses helper function rmw_dds_common::clamp_rmw_time_to_dds_time() to make sure that the ROS time can be represented by DDS (note that the current implementation does not actually guarantee it, but it will after ros2/rmw_dds_common#48 is merged).

I also took the chance to delete some commented out code, and made a small change to make sure users are informed with a warning that they are trying to configure lifespan qos on an RMW that doesn't support it (i.e. rmw_connextddsmicro).

The need to open this PR for the galactic branch comes from ros2/rosbag2#756 and the fact that ros2 bag play is currently broken with rmw_connextdds in Galactic/Rolling.

I will open another PR afterwards to merge this feature to master.

[hidmic]

LGTM pending green Galactic CI. CC'ing @cottsay for awareness.

[hidmic]

On a related note, @emersonknapp the fact that we missed this suggests that when RMW_DURATION_INFINITE was introduced, no tests were added to test_rmw_implementation.

[emersonknapp]

no tests were added to test_rmw_implementation.

Yes, you're right - I didn't know about these tests. I'll take an action item to add a test there before the distro freeze.

[emersonknapp]

LGTM - thanks for taking this on

[hidmic]

Full Galactic CI (minus Fast-DDS, to speed things up a bit):

• Linux
• macOS
• Windows

[asorbini]

@hidmic Thank you for starting CI jobs, I was just about to do the same. FYI I think Linux-aarch64 can be omitted, since it doesn't test rmw_connextdds

[hidmic]

FYI I think Linux-aarch64 can be omitted

Good point. Aborted.

[asorbini]

There were some suspicious failures, I'll take a look

-- run_test.py: return code -11

[asorbini]

I had introduced a regression because I forgot that lifespan is a reader-only qos. Should be fixed in 47224b2.

Running CI again:

• Linux:
• macOS:
• Windows:

EDIT: plans are running with 1323a01, which cleans up the code a little bit.

[clalancette]

@asorbini Can you retarget this fix to the master branch? Assuming we are happy with it there, then we can backport to galactic. Thanks.

[asorbini]

@clalancette switched the target to master. Here's some CI with just rmw_connextdds enabled:

• Linux
• macOs
• Windows

[asorbini]

2 failures on Linux:

• interactive_markers.TestInteractiveMarkerClient.states: should be unrelated, see TestInteractiveMarkerClient.states fails when run with rmw_connextdds #40
• test_tf2/test_static_publisher: I haven't seen this failure before, not sure if it could be related or not (it might be, since I don't see it failing in the latest nighty):

[test_static_publisher-3] [ RUN      ] StaticTransformPublisher.multiple_parent_test
...
[test_static_publisher-3] /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/geometry2/test_tf2/test/test_static_publisher.cpp:201: Failure
[test_static_publisher-3] Value of: mB.canTransform("a", "d", tf2::timeFromSec(0))
[test_static_publisher-3]   Actual: false
[test_static_publisher-3] Expected: true
[test_static_publisher-3] /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/geometry2/test_tf2/test/test_static_publisher.cpp:202: Failure
[test_static_publisher-3] Value of: mB.canTransform("a", "d", tf2::timeFromSec(100))
[test_static_publisher-3]   Actual: false
[test_static_publisher-3] Expected: true
[test_static_publisher-3] /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/geometry2/test_tf2/test/test_static_publisher.cpp:203: Failure
[test_static_publisher-3] Value of: mB.canTransform("a", "d", tf2::timeFromSec(1000))
[test_static_publisher-3]   Actual: false
[test_static_publisher-3] Expected: true
...
[test_static_publisher-3] [  FAILED  ] StaticTransformPublisher.multiple_parent_test (1809 ms)

[asorbini]

I have verified locally that the failure in test_tf2/test_static_publisher are not telated to the changes introduced by this PR.

As a test, I was able to reproduce the failure with branch master too. I'm not quite sure what causes it, but it seems to pass sporadically. One likely relevant tidbit of information (which also appears in the logs above) is this message which is repeated almost endlessly in the output:

4: [test_static_publisher-3] Warning: Invalid frame ID "a" passed to canTransform argument target_frame - frame does not exist
4: [test_static_publisher-3]          at line 156 in /home/asorbini/workspace-rmw/dev/tf2/geometry2/tf2/src/buffer_core.cpp

At this point I tried to "speed up" the reliability protocol (using an XML similar to the one I posted in this comment), but I haven't been able to get rid of the warnings, nor to get the test to pass reliably.

Regardless, I think this PR could be merged as is, and the test failure should be investigated separately.

@clalancette what do you think?

[clalancette]

4: [test_static_publisher-3] Warning: Invalid frame ID "a" passed to canTransform argument target_frame - frame does not exist
4: [test_static_publisher-3] at line 156 in /home/asorbini/workspace-rmw/dev/tf2/geometry2/tf2/src/buffer_core.cpp

This output means that there are no publications on the /tf topic with the 'a' frame.

That being said, we aren't seeing that on the nightlies. So there may be something going on with the interaction of rmw_connextdds and tf2.

At the end of the day, as long as this doesn't cause tests to go yellow with the default DDS vendor on the nightlies, and it has been approved, then I'm fine with this going in.

[asorbini]

At the end of the day, as long as this doesn't cause tests to go yellow with the default DDS vendor on the nightlies, and it has been approved, then I'm fine with this going in.

The error only happens when the test is run with RMW_IMPLEMENTATION=rmw_connextdds, and not with the default RMW, so I think we're good to go for now. Thank you for confirming!