Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new interface for time conversion #145

Merged
merged 7 commits into from
Nov 18, 2021

Conversation

Barry-Xu-2018
Copy link
Contributor

@Barry-Xu-2018 Barry-Xu-2018 commented Sep 16, 2021

Add a new interface rcpputils::rcutils_duration_cast.

About this new interface, it is mentioned at ros2/rclcpp#1662 (ros2/rclcpp#1662 (comment))

include/rcpputils/time.hpp Outdated Show resolved Hide resolved
include/rcpputils/time.hpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggesting some inline fixes that should help address the problems with CI.

include/rcpputils/time.hpp Outdated Show resolved Hide resolved
test/test_time.cpp Outdated Show resolved Hide resolved
@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya

Please help to re-run CI.

@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Barry-Xu-2018
Copy link
Contributor Author

@ivanpauno friendly ping

include/rcpputils/time.hpp Outdated Show resolved Hide resolved
test/test_time.cpp Outdated Show resolved Hide resolved
@Barry-Xu-2018
Copy link
Contributor Author

@ivanpauno Updated based on your comments.
Please help to review again

@clalancette
Copy link
Contributor

This looks like it needs a rebase, then CI and I think we can get it in.

// Casting to a double representation might lose precision and allow the check below to succeed
// but the actual cast to nanoseconds fail. Using 1 DurationT worth of nanoseconds less than max
constexpr auto maximum_safe_cast_ns =
std::chrono::nanoseconds::max() - std::chrono::duration<DurationRepT, DurationT>(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a bug.
std::chrono::duration<DurationRepT, DurationT>(1) depend on user input. This leads maximum_safe_cast_ns isn't fixed value. I will use std::chrono::nanoseconds(1) instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was not a bug, it is intentionally based on user template unit. if i am not mistaken...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If depend on user template unit, there is a below problem.
For the same time, std::chrono::minutes(2562047*60) doesn't throw exception, but std::chrono::hours(2562047) can show exception.

Copy link

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iuhilnehc-ynos
Copy link

LGTM

Sorry, wrong place to reply.

@Barry-Xu-2018
Copy link
Contributor Author

Do rebase

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@clalancette clalancette self-assigned this Nov 18, 2021
@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit a02ef91 into ros2:master Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants