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

Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution #843

Merged
merged 4 commits into from
Aug 13, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Aug 11, 2021

Fixes failing delay test on Windows (see https://ci.ros2.org/view/nightly/job/nightly_win_deb/2077/#showFailuresLink for example) - which was finishing a few milliseconds early. This is likely due to the resolution of float not carrying the expectations clearly.

Additionally, this is a more clear API given that rclcpp::Duration is strongly typed, rather than having to clarify the units in the PlayOptions declaration.

…osecond counts so that error messages are readable (move to EXPECT as well)

Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Aug 11, 2021

Windows: Build Status

Emerson Knapp added 2 commits August 11, 2021 15:04
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

After rclcpp::Duration update - may just be a resolution thing

Windows: Build Status

@emersonknapp emersonknapp changed the title [WIP] Convert all time comparisons in playing_respects_delay to use raw nanosecond counts so that error messages are readable (move to EXPECT as well) [WIP] Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution Aug 12, 2021
@emersonknapp emersonknapp changed the title [WIP] Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution [WIP] Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution Aug 12, 2021
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

Fixing compiler warnings - Windows run first: Build Status

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Aug 13, 2021

Rest of the builds

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

@emersonknapp emersonknapp marked this pull request as ready for review August 13, 2021 00:04
@emersonknapp emersonknapp requested a review from a team as a code owner August 13, 2021 00:04
@emersonknapp emersonknapp requested review from prajakta-gokhale and removed request for a team August 13, 2021 00:04
@emersonknapp emersonknapp changed the title [WIP] Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution Update PlayOptions::delay to rclcpp::Duration to get nanosecond resolution Aug 13, 2021
Copy link
Contributor

@lihui815 lihui815 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@emersonknapp emersonknapp merged commit cda207a into master Aug 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/delay_test_types branch August 13, 2021 16:30
WJaworskiRobotec pushed a commit to RobotecAI/rosbag2 that referenced this pull request Sep 12, 2021
…esolution (ros2#843)

* Change type of PlayOptions::delay to rclcpp::Duration

Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Wojciech Jaworski <[email protected]>
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.

4 participants