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

[Servo] Use a WallRate so the clock is monotonically increasing #1543

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Aug 29, 2022

Description

The default rclcpp::Rate uses a std::chrono::system_clock. Better to use a rclcpp::WallRate here since we only care about the change in time.

Hat tip for @ChrisThrasher for bringing this type of issue to my attention.

If we're really lucky, this will fix the flaky Servo tests.

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #1543 (f871086) into main (43a22ec) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1543      +/-   ##
==========================================
+ Coverage   51.06%   51.11%   +0.05%     
==========================================
  Files         380      380              
  Lines       31802    31802              
==========================================
+ Hits        16238    16253      +15     
+ Misses      15564    15549      -15     
Impacted Files Coverage Δ
moveit_ros/moveit_servo/src/pose_tracking.cpp 76.78% <100.00%> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 68.25% <100.00%> (+1.68%) ⬆️
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.73% <0.00%> (+0.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AndyZe
Copy link
Member Author

AndyZe commented Aug 29, 2022

CI has passed 3X in a row now. I'm going to see if I can re-enable some of the tests that were previously commented.

@vatanaksoytezer
Copy link
Contributor

@AndyZe do you want to try to re-enable other tests in this PR or is this good to merge?

@AndyZe
Copy link
Member Author

AndyZe commented Aug 30, 2022

@AndyZe do you want to try to re-enable other tests in this PR or is this good to merge?

I tried re-enabling them but saw some compiler errors because the API has changed, I guess. I'd be happy to push that to later.

@AndyZe AndyZe merged commit c134d89 into moveit:main Aug 30, 2022
@AndyZe AndyZe deleted the andyz/steady_clock branch August 30, 2022 12:51
@AndyZe AndyZe mentioned this pull request Sep 6, 2022
@tylerjw tylerjw added the backport-humble Mergify label that triggers a PR backport to Humble label Oct 28, 2022
mergify bot pushed a commit that referenced this pull request Oct 28, 2022
* [Servo] Use a WallRate so the clock is monotonically increasing

* Re-enable a commented integration test

(cherry picked from commit c134d89)
tylerjw pushed a commit that referenced this pull request Oct 28, 2022
… (#1658)

(cherry picked from commit c134d89)

Co-authored-by: AndyZe <zelenak@picknik.ai>
@AndyZe AndyZe mentioned this pull request Jul 7, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants