Controllers should not be influenced by time jumps or slew

[mbuijs]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	-
Primary OS tested on	Ubuntu
Robotic platform tested on	simulation

---

Description of contribution in a few bullet points

• rclcpp::Rate is used for controlling the frequency in a few parts of the code.
• rclcpp::Rate uses std::chrono::system_clock which is meant to represent wall time. Due to unevitable clock drift on the local system in combination with time synchronisation mechnism's like NTP this clock can tick at a slower or faster rate to stay synchronized with the actual wall time or even jump both forwards and backwards in time in case of bigger offsets.
• rclcpp::Rate does not have a mechanism to deal with time jumps, so for example if the time jumps backwards 10 seconds, it will wait for 10 seconds plus the configured interval of the Rate.

This contribution changes the use of rclcpp::Rate to rclcpp::GenericRate<std::chrono::steady_clock> to use the monotonic steady_clock instead. This clock never jumps and is better suited for measuring intervals.

A possible point of discussion would be why I did not use rclcpp::WallRate, which is an alias for rclcpp::GenericRate<std::chrono::steady_clock>. The reason is that I find the name WallRate very misleading, since steady_clock specifically does not follow wall time. In my opinion it should be renamed to SteadyRate or something like that, but that's a different discussion.

Description of documentation updates required from your changes

N/A. (?)

[SteveMacenski]

Thank you for your detailed explanation. After a little bit of digging, I agree with your assessment.

[SteveMacenski]

I'd prefer WallRate since rclcpp gives that to us and makes this code more readable. When I google things, it seems that this is an accurate use of the Wall phrasing. I will merge when WallRate.

[mbuijs]

I'd prefer WallRate since rclcpp gives that to us and makes this code more readable. When I google things, it seems that this is an accurate use of the Wall phrasing. I will merge when WallRate.

I'm a bit surprised that you found this an accurate use of the term 'wall' then googling. Among the results I ended up at were man pages and cppreference, which both specifically mentioned that these wall clocks can jump. Then I checked how this is done in ROS1 and I found that there also ros::Rate and ros::WallRate are both linked to system_clock with the only difference being that ros::Rate is linked to ROS time in case use_sime_time is enabled. All findings combined, I decided to create a PR on rclcpp for renaming rclcpp::WallRate to rclcpp::SteadyRate: ros2/rclcpp#1373. This needs some more work though and likely some discussion on how to proceed.

I will change this PR to use rclcpp::WallRate if you insist, but I wanted to double check beforehand.

Side note: I'm wondering if in the long run it would be a better idea to not use Rate and it's specializations at all and switch to using timers instead. It's a bit more code, but in my experience ends up more aligned with how other asynchronous code is written.

[SteveMacenski]

Yes, please use the ROS2 provided APIs where they exist to make things more readable. I'd be happy to have it changed to another name if that change is made in rclcpp (I don't really have a strong feeling one way or another).

Timers have their roles, but not in the places where you made changes.

[mbuijs]

Done.

[SteveMacenski]

I'll merge after CI passes

[codecov]

Codecov Report

Merging #2012 into main will decrease coverage by 0.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2012      +/-   ##
==========================================
- Coverage   84.95%   84.33%   -0.62%     
==========================================
  Files         294      294              
  Lines       15056    15056              
==========================================
- Hits        12791    12698      -93     
- Misses       2265     2358      +93     

Flag	Coverage Δ
#project	84.33% <100.00%> (-0.62%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nav2_controller/src/nav2_controller.cpp	89.23% <100.00%> (-1.35%)	⬇️
nav2_costmap_2d/src/costmap_2d_ros.cpp	87.30% <100.00%> (ø)
...v2_recoveries/include/nav2_recoveries/recovery.hpp	83.52% <100.00%> (-5.89%)	⬇️
nav2_waypoint_follower/src/waypoint_follower.cpp	91.86% <100.00%> (ø)
nav2_util/include/nav2_util/costmap.hpp	0.00% <0.00%> (-100.00%)	⬇️
...vior_tree/test/plugins/action/test_spin_action.cpp	0.00% <0.00%> (-100.00%)	⬇️
...av2_dwb_controller/dwb_critics/src/oscillation.cpp	68.67% <0.00%> (-21.69%)	⬇️
...nclude/nav2_behavior_tree/behavior_tree_engine.hpp	88.88% <0.00%> (-11.12%)	⬇️
...v2_util/include/nav2_util/simple_action_server.hpp	91.87% <0.00%> (-3.75%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64871a7...e7594a2. Read the comment docs.