Skip to content

Add new interface for time conversion#145

Merged
ivanpauno merged 7 commits intoros2:masterfrom
Barry-Xu-2018:topic-add-interface-time-convert
Nov 18, 2021
Merged

Add new interface for time conversion#145
ivanpauno merged 7 commits intoros2:masterfrom
Barry-Xu-2018:topic-add-interface-time-convert

Conversation

@Barry-Xu-2018
Copy link
Copy Markdown
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))

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-add-interface-time-convert branch from 27b4266 to f4f6ee9 Compare September 16, 2021 07:44
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-add-interface-time-convert branch from f4f6ee9 to 42fe666 Compare September 16, 2021 09:09
@fujitatomoya
Copy link
Copy Markdown
Collaborator

CI:

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

Copy link
Copy Markdown
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.

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

@fujitatomoya

Please help to re-run CI.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

CI:

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

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

@ivanpauno friendly ping

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

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

@clalancette
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

LGTM

Sorry, wrong place to reply.

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-add-interface-time-convert branch from 6164e5c to a9f9d67 Compare November 17, 2021 10:29
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-add-interface-time-convert branch from a9f9d67 to e9f73af Compare November 17, 2021 10:48
@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

Do rebase

Copy link
Copy Markdown
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
Copy Markdown
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