-
Notifications
You must be signed in to change notification settings - Fork 467
deprecate rclcpp::spin_some and rclcpp::spin_all #2848
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
base: rolling
Are you sure you want to change the base?
Conversation
e8e8faf
to
f1e7310
Compare
@Mergifyio rebase |
…match the other executors Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
…omments Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
✅ Branch has been successfully rebased |
f9d55bd
to
5cbdae6
Compare
Pulls: #2848 |
Description
Deprecate
rclcpp::spin_some
andrclcpp::spin_all
.These functions can lead to very poor performance for developers who are not aware that calling them will:
If called in a loop, you are wasting a lot of CPU cycles repeating those operations every time.
This instead is a lot more efficient because only the spin operation is repeated as expected.
Adding and removing nodes from executors are expensive operations that shouldn't be done at "steady state" of the system.
IMPORTANT
In this PR I'm also re-ordering the events-executor constructor arguments so that this becomes possible:
Now all executors take the executor options as first argument.
Is this user-facing behavior change?
Yes, deprecating some functions and modifying a constructor (although it's in the experimental namespace)
Did you use Generative AI?
No
closes #2675