Fix update rate issues by working around MutliThreadedExecutor#275
Merged
bmagyar merged 2 commits intoros-controls:masterfrom Jan 6, 2021
ruvu:fix/update-rate-issues
Merged
Fix update rate issues by working around MutliThreadedExecutor#275bmagyar merged 2 commits intoros-controls:masterfrom ruvu:fix/update-rate-issues
bmagyar merged 2 commits intoros-controls:masterfrom
ruvu:fix/update-rate-issues
Conversation
Member
|
Could you please elaborate on the effects of this approach? |
destogl
requested changes
Dec 20, 2020
Member
destogl
left a comment
There was a problem hiding this comment.
This is related to #260. It seems there is an issue in the depths of rclcpp (ros2/rclcpp#1487). There is also a solution that needs some more time... (ros2/rclcpp#1168).
The solution is IMHO legit until this is solved in rclcpp.
Currently the MutliThreadedExecutor performance is very bad. This leads to controllers not meeting their update rate. This PR is a temporary workaround for these issues. The current approach uses a `rclcpp` timer to execute the control loop. When used in combination with the `MutliThreadedExecutor`, the timers are not execute at their target frequency. I've converted the control loop to a while loop on a separate thread that uses `nanosleep` to execute the correct update rate. This means that `rclcpp` is not involved in the execution and leads to much better performance.
Contributor
Author
|
@bmagyar I've added a simple benchmark to the description |
bmagyar
approved these changes
Jan 6, 2021
Karsten1987
approved these changes
Jan 6, 2021
Contributor
|
This PR actually broke the build on OSX. |
Contributor
Author
|
Sorry, I did not know OSX was a requirment. Should there be a CI for that? Good suggestion to replace the call with |
destogl
pushed a commit
to b-robotized-forks/ros2_control
that referenced
this pull request
Aug 11, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently the MutliThreadedExecutor performance is very bad. This leads to controllers not meeting their update rate. This PR is a temporary workaround for these issues.
The current approach uses a
rclcpptimer to execute the control loop. When used in combination with theMutliThreadedExecutor, the timers are not execute at their target frequency. I've converted the control loop to a while loop on a separate thread that usesnanosleepto execute the correct update rate. This means thatrclcppis not involved in the execution and leads to much better performance.I've done some simple performance benchmarking. Using https://github.com/ros-controls/ros2_control_demos
with
update_rateset to 500 Hz.fix/update-rate-issues branch
master branch
I'm curious what you guys think about this workaround. Without this patch, I'm unable to run a control loop at 50Hz.