Convert ROS2 time value to signed int64_t

[ros2]

Convert ROS2 time value to signed int64_t

[tfoote]

New CI with rebases and rclpy:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

I opened a follow up issue: ros2/ros2#458

[serge-nikulin]

@wjwwood:

Well, the idea was that we don't need to represent time before 1970 pretty much ever. If you need to represent time at that point in history, there are "date formats" which can capture the information in parts with the year, month, day, hour, minute, second, and so on separated. Remember that this timepoint representation is meant to measure time since an arbitrary point in the past, not all of time (which it cannot possibly do anyways), so I don't think "non-Kelvin" makes any sense. With the current setup we can only reference time after 1970, with signed, we can only measure time after (1970 - (263 - 1) nanoseconds), which is just another arbitrary "absolute zero".

If you accept that the timepoint's signed-ness just moves the arbitrary point in the past from which we measure, then the unsigned version gives us more time to represent since in both cases 0 is the unix epoch. It's common for solutions to use a signed 64-bit seconds field, but since we're representing with only nanoseconds we're more limited.

That being said, both of the roll over dates for signed versus unsigned 64-bit nanoseconds are far enough in the future to be academic for us.

I think (someone check my math):

• int64_t nanoseconds with unix epoch will approximately roll over sometime in the year 2262

  • i.e. 1970 + ((263 - 1) / 1e9 / 60 / 60 / 24 / 365.25)

• uint64_t nanoseconds with unix epoch will approximately roll over sometime in the year 2554

  • i.e. 1970 + ((264 - 1) / 1e9 / 60 / 60 / 24 / 365.25)

Which is a difference of 292 years, but for me 2262 is probably far enough away (245 years away) that it will work also.

For the argument that we shouldn't mix signed and unsigned types, I thinks that's a rather aggressive restriction, so all things being equal it's not the best, single reason to prefer a signed time point type.

However, given that it seems acceptable (at least to me) to use a signed 64-bit int for nanoseconds since unix epoch, it might be something we would do.

I'm currently in crunch time, so I don't have time to champion this particular change, but if you're interested in getting it done, I'd suggest making an issue on our rcutils repository about this change and let our team/community discuss it.

[serge-nikulin]

For the argument that we shouldn't mix signed and unsigned types, I thinks that's a rather aggressive restriction

Unfortunately not in MISRA C 2012 land (E.g. rule 10.4).

[serge-nikulin]

@wjwwood:

Unfortunately not in MISRA C 2012 land (E.g. rule 10.4).

But aren't exceptions allowed? If we could put all math using the storage behind functions and test those functions properly, I think an exception would be allowed.

There is almost certainly other code, e.g. serialization code, that will mix these types at some point, and will need a similar exception, e.g. there's no other way to store data as unsigned octets and then get a signed 8-bit or 32-bit or 64-bit value out of them without casting from unsigned to signed.

[tfoote]

If this can help us become more compliant this seems to make sense. It does make the range checking much simpler. We'll likely want to move those methods into C but for now the C++ templated versions are very compact.

[wjwwood]

Ok, here's CI:

• Linux
• Linux-aarch64
• macOS
• Windows

Thanks for the reviews @tfoote.

[wjwwood]

New CI with rebases:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

@serge-nikulin looks like this set of changes affects rclpy as well, see these new compiler warnings:

http://ci.ros2.org/job/ci_linux/3918/warnings22Result/new/package.-275512363/

[wjwwood]

GNU C Compiler Warnings: 1 warning.

• 1 new warning

[tfoote]

Above CI segment updated with ros2/rclcpp@9e26556 added

[tfoote]

Everything went green, but windows went unstable for what appears to be unrelated recent changes.

	Test Result (7 failures / +7)
projectroot.test_logging_macros
projectroot.test_api_srv_composition__rmw_fastrtps_cpp
projectroot.test_api_srv_composition_client_first__rmw_fastrtps_cpp
rcutils.TestLoggingMacros.test_logging_throttle
rcutils.TestLoggingMacros.test_logging_skipfirst_throttle
composition.test_api_srv_composition__rmw_fastrtps_cpp.xunit.missing_result
composition.test_api_srv_composition_client_first__rmw_fastrtps_cpp.xunit.missing_result

[mikaelarguedas]

composition.test_api_srv_composition__rmw_fastrtps_cpp.xunit.missing_result composition.test_api_srv_composition_client_first__rmw_fastrtps_cpp.xunit.missing_result are known flaky tests.

The rcutils ones are new and related to timing (only the tests doing throttling are failing) so I think they are caused from this PR and should be investigated before considering merging

[serge-nikulin]

test_logging_macros fails because of integer overflow in the current implementation of rcutils_steady_time_now function.

The overflow periodically happens in RCUTILS_S_TO_NS macro during PC runtime: this performance counter increments at CPU frequency, 3.6 GHz on my PC.

I am currently trying to find a way to fix this issue (fighting chain of logging macros).

[mikaelarguedas]

Thanks @serge-nikulin for the update. It appears that the variables used in

rcutils/src/time_win32.c

Line 39 in f37ce5b

ULARGE_INTEGER uli;

should be converted to signed as well, right ?

[serge-nikulin]

@mikaelarguedas, yes, it should but this particular bug should not reveal itself for ~200 years from now. :)

The immediate bug AFAIK is interaction between overflow in rcutils_steady_time_now and RCUTILS_LOG_CONDITION_THROTTLE_BEFORE macro.

[serge-nikulin]

@mikaelarguedas, please see my fix in ff6bf5e

Alternative (more correct?) solution would be to scale performance_count by a variable non-overflowing factor instead of RCUTILS_S_TO_NS which is always 1^9. This solution would employ this kind of bit hack.

[serge-nikulin]

@tfoote, FYI: I have pushed the fix for rcutils failed tests.

[tfoote]

Thanks @serge-nikulin new round of CI kicked off.

• Linux
• Linux-aarch64
• macOS
• Windows

@mikaelarguedas thanks for the heads up on the flakey tests. I confirmed they went away in a rebuild: http://ci.ros2.org/job/ci_windows/4024/#showFailuresLink

[serge-nikulin]

@tfoote, it looks like some Windows node environment problem (failed before the build started?).

[mikaelarguedas]

Yup the node disconnected. The Job restarted automatically, I updated the link in the previous comment

[mikaelarguedas]

Alternative (more correct?) solution would be to scale performance_count by a variable non-overflowing factor instead of RCUTILS_S_TO_NS which is always 1^9. This solution would employ this kind of bit hack.

Thanks for the suggested alternative. I'm fine to go ahead and merge this as is as even if the problem is not completely solved, I don't expect anyone to hit the overflow in a reasonable future

[serge-nikulin]

@mikaelarguedas , the alternative solution also might overflow. The only correct solution is to do math in 128-bit space. Please go ahead with the current solution.

[mikaelarguedas]

My only remark looking at the various PRs is that it may be worth forwarding the rcutils_time_point_* types to rcl and use that everywhere instead of the bare primitive type (although I don't expect us to change it again anytime soon).

Talking with @wjwwood we'll merge this as is and will open a set of follow-up PRs to forward and use the custom type everywhere (after auditing the codebase for every use of it)