Create a utility function to limit rmw_time_t to 32-bit values #37
Conversation
The RMW layer maintains the rmw_time_t's seconds and nanoseconds values as 64-bit unsigned integers. However, the DDS standard specifies the Time_t and Duration_t types with "long sec" and "unsigned long nanosec", and the IDL to C++11 mapping states that the C++ types for long and unsigned long are mapped to int32_t and uint32_t, respectively. This difference can result in the seconds (and/or nanoseconds) value being truncated and incorrect, causing other downstream failures. This function can be used in different RMW adaptation layers that interface to the DDS providers when converting from rmw_time_to to the DDS Duration_t (or Time_t), such as rmw_time_to_fast_rtps. Signed-off-by: Michael Jeronimo <[email protected]>
| */ | ||
| RMW_DDS_COMMON_PUBLIC | ||
| rmw_time_t | ||
| maintain_32bit_limits(const rmw_time_t & time); |
There was a problem hiding this comment.
To bikeshed the name of the function a bit: it doesn't say anything about dealing with time, so the calls are somewhat unclear:
auto t1 = rmw_dds_common::maintain_32bit_limits(zeros);
I'll suggest naming this method something like: rmw_time_to_dds_time or something along those lines.
There was a problem hiding this comment.
Separately, while this is an OK start, there are a few other questions that come up with regards to this:
- This works (to a degree) for passing an rmw_time down to the DDS layer. What happens when we get a time back from the DDS layer? I guess a 32-bit always fits into a 64-bit, so the answer may be "nothing", but it bears thinking about.
- Relatedly, what is going to happen in 2038 with 32-bit DDS times? I know @wjwwood mentioned that they were going to roll-over, do we have any documentation on how that is expected to work?
There was a problem hiding this comment.
Yes, much better name. Updated.
Regarding the first item, I can't think of any potential issues with expanding the 32-bit values to their 64-bit counterparts.
As for the second item, I don't have any answers, but I see you mentioned it here: ros2/rcl_interfaces#85. Which also references this: https://issues.omg.org/issues/DDS15-13. So it would be good to hear from the DDS vendors about their plans.
There was a problem hiding this comment.
You can try to @ some people here, or you could bring it up at the next Middleware Working Group meeting as a last resort, since several very knowledgeable DDS people are there sometimes.
I have been told how it was going to be resolved, but I can't find a link to any memorandum on the subject. It was likely in person.
There was a problem hiding this comment.
I'm wondering if it makes sense to introduce a dds_time_to_rmw_time here. For now, it would be a direct assignment (so essentially a no-op). But once we figure out what the DDS solution to this problem is, we may have to implement something here (like rebasing from the DDS new "epoch" to the Unix one).
Or maybe we should just wait to hear back from the DDS vendors here. @MiguelCompany @eboasson any feedback on 32-bit times in the DDS standard and the year 2038 problem?
There was a problem hiding this comment.
It is still a point of discussion in the OMG. The tracking system has issues that are being worked on for the various standards, the difficulty is in resolving the backwards compatibility considerations. The difficulty here is that all options have different consequences for the different specifications.
Currently, the RTPS protocol should be fine until after 2100 (switch to unsigned 32-bit seconds in DDSI-RTPS 2.3, at the price of breaking applications using time stamps prior to 19700101 00:00:00 UTC). For the APIs it looks likely that the seconds field will be extended to 64-bits because a change to unsigned affects application code in nasty ways, but as far as I know, this hasn't been decided yet.
(Cyclone avoids the problem altogether by using 64-bit nanoseconds since the Unix epoch, both internally and in its C API, and so I need not worry about surviving until the point where roll-over is within the 30 year horizon that aerospace tends to use. I guess no-one's planning another Voyager 2 ...)
There was a problem hiding this comment.
@eboasson Thanks so much for the feedback. That is very helpful.
What version of the RTPS spec does Cyclone currently implement?
@MiguelCompany @richiprosima Same question for eProsima; what version of the RTPS spec does Fast DDS currently implement?
There was a problem hiding this comment.
@clalancette in this respect Cyclone still follows version 2.2 (that is, the interpretation of the seconds field hasn't been updated yet).
Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Michael Jeronimo <[email protected]>
delete-merged-branch
bot
deleted the
mjeronimo/utility-function-for-32-bit-limits
branch
|
The RMW layer maintains the rmw_time_t's seconds and nanoseconds
values as 64-bit unsigned integers. However, the DDS standard
specifies the Time_t and Duration_t types with "long sec" and
"unsigned long nanosec", and the IDL to C++11 mapping states that
the C++ types for long and unsigned long are mapped to int32_t and
uint32_t, respectively. This difference can result in the seconds
(and/or nanoseconds) value being truncated and incorrect, causing
other downstream failures. This function can be used in different
RMW adaptation layers that interface to the DDS providers when
converting from rmw_time_to to the DDS Duration_t (or Time_t),
such as rmw_time_to_fast_rtps.
Signed-off-by: Michael Jeronimo [email protected]