-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Deprecate StaticSingleThreaded Executor. #4812
Deprecate StaticSingleThreaded Executor. #4812
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@clalancette @alsora could you review this? |
HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/11670308253/artifacts/2142625163. To view the resulting site:
|
|
||
The *Static Single-Threaded Executor* has been deprecated, and *Single-Threaded Executor* is recommended instead. | ||
The *Static Single-Threaded Executor* was the only *Executor* optimized the runtime costs for rebuilding the entities of a node in terms of subscriptions, timers, service servers, action servers, etc. | ||
But this runtime improvements are now available in all the other *Executor*, so that all *Executor* can now take advantage of this improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the two sentences here can be written in a more clear way.
The Static Single-Threaded Executor was the only Executor optimized the runtime costs for rebuilding the entities of a node in terms of subscriptions, timers, service servers, action servers, etc.
But this runtime improvements are now available in all the other Executor, so that all Executor can now take advantage of this improvement.
could become
The Static Single-Threaded Executor was developed to reduce the the runtime costs for scanning the entities of a node in terms of subscriptions, timers, service servers, action servers, etc.
These runtime improvements are now available also in all the other Executor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also mention that there are known bugs affecting only the static executor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alsora do you mean ros2/rclcpp#2589, right? i am not sure about that, because that looks like regression and we are going to fix that? if it seems to take much longer time to address it, probably we can consider that with another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alsora friendly ping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that ticket refers to a bug affecting all waitset-executors, but there are also some bugs that are specific to the static executor.
You can see them looking at the unit-tests that are run on StandardExecutors
(i.e. they are not run on the static executor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I'd like to get @alsora 's approval before we merge.
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
we should also deprecate
StaticSingleThreadedExecutor
from the documentation.aligns with ros2/rclcpp#2598
Important
Do not backport this, because
StaticSingleThreadedExecutor
is deprecated only inrolling
.