Waitset timestamp fields

[iluetkeb]

Both the standard wait_set and internal structures got the timestamps.
Currently, the fields are not used.

Also see ros2/rmw#200

[iluetkeb]

@dirk-thomas @wjwwood are you generally okay with the field additions to the existing structs? if yes, I could proceed with further work that's actually using them.

[iluetkeb]

Hey @JaimeMartin @MiguelCompany this is the start of timestamp information in rmw/rcl. It is intended that these fields are filled by the middleware with the arrival timestamp of messages. I would like to start a discussion with you guys on how to best provide this information.

[ralph-lange]

Connects to ros2/design#259.

[wjwwood]

I had a few style nitpicks, but otherwise lgtm.

We should consider any necessary updates to the tests and documentation as well.

[MiguelCompany]

Hey @JaimeMartin @MiguelCompany this is the start of timestamp information in rmw/rcl. It is intended that these fields are filled by the middleware with the arrival timestamp of messages. I would like to start a discussion with you guys on how to best provide this information.

Would it be enough to have a get_first_unread_sample_info on the subscribers that could be called from rmw_wait when necessary?

[iluetkeb]

Would it be enough to have a get_first_unread_sample_info on the subscribers that could be called from rmw_wait when necessary?

That should work and would probably provide the most accurate information, however we are a bit concerned about performance. Reading the sample info seems like it would require deserialization. Also, since one of the other things we have identified is that we might potentially need to call rmw_wait more often, we should keep this as performant as possible.

One other idea I had is that you could keep the listener attached to the objects across calls to wait, and then store the current time within the callback that gets notified upon changes. This would likely incur almost no overhead and still provide good-enough information.

[iluetkeb]

We should consider any necessary updates to the tests and documentation as well.

As said in ros2/rmw#200, I think we need to test everything in rcl, since it does the relevant work.

However, in test_wait.cpp, I do not see any tests which are checking the pre-existing fields. For example, in wait_set_is_valid, there is no looking at the actual contents of the wait_set. Similarly, there is a test test_resize_to_zero, which looks at the counts but not the arrays inside the wait_set.

@wjwwood is that intentionally? I mean, I'd be happy to add a test for the contents, but I'm wondering what the current lack of such tests should tell me.

[wjwwood]

@wjwwood is that intentionally? I mean, I'd be happy to add a test for the contents, but I'm wondering what the current lack of such tests should tell me.

I don't think it is intentional, but wait_set_is_valid was added recently. I think @jacobperron added it in this pr #538, but I can't tell just from the context if that's intentional or an oversight.

[jacobperron]

For the test I added, I'm fairly certain I just copied one of the existing tests and modified it minimally to exercise the API I added.

[iluetkeb]

Alright, @wjwwood @jacobperron thanks for the info. I'll add a test looking at all the fields, then.

[wjwwood]

lgtm other than nitpicks

[iluetkeb]

Alright folks, this now has a test for presence of timestamps, and in ros2/rmw_fastrtps#359 we have an implementation of timestamps for subscriptions, clients and services that passes the test.

[Karsten1987]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

@iluetkeb ironically, I don't have access to the fork of boschresearch, so I can't push a fix on your branch. I think what's missing here is the following:

diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt
index fb31b27..470337f 100644
--- a/rcl/test/CMakeLists.txt
+++ b/rcl/test/CMakeLists.txt
@@ -219,7 +219,7 @@ function(test_target_function)
     ENV ${rmw_implementation_env_var}
     APPEND_LIBRARY_DIRS ${extra_lib_dirs}
     LIBRARIES ${PROJECT_NAME}
-    AMENT_DEPENDENCIES ${rmw_implementation} "osrf_testing_tools_cpp"
+    AMENT_DEPENDENCIES ${rmw_implementation} "osrf_testing_tools_cpp" "test_msgs"
   )

   # This condition runs the timestmap test only when we know the RMW implementation

[iluetkeb]

That include is not actually necessary, since I moved the relevant test to test_wait_set_timestamp.c. I'm wondering why it doesn't complain on my box, but anyway, it's removed now.

I also added you to the correct team for our forks.

[Karsten1987]

• Linux
• Linux-aarch64
• macOS
• Windows

[rotu]

I'm sorry I'm coming to this late. It seems like a bad idea, and I think should be strongly reconsidered. ros2/design#259 (comment)

[iluetkeb]

@rotu I have commented in the design issue and I hope I could address your concerns.

It is a bit late now, in my case literally because I'm in Europe, where it's currently half past eight 😉 So I will log off after this comment, and tomorrow being Good Friday, I won't work. If you have further concerns, I can only contribute further on Saturday or (preferably) after Easter. Hope that's enough! Happy Holidays to you all.

[iluetkeb]

Closed in favor of #619