-
Notifications
You must be signed in to change notification settings - Fork 417
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
Add option for the events executor to the isolated component container #2541
base: rolling
Are you sure you want to change the base?
Add option for the events executor to the isolated component container #2541
Conversation
std::vector<std::string> args = rclcpp::remove_ros_arguments(argc, argv); | ||
for (auto & arg : args) { | ||
if (arg == std::string("--use_multi_threaded_executor")) { | ||
if (arg == std::string("--use_multi_threaded_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.
Since using underscores in command flags is very uncommon. I added the new option with dashes and allowed for the same here.
Perhaps in the future we can deprecate the original flag based on discussion here.
5deb093
to
2c75845
Compare
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.
a couple of questions,
EventsExecutor
is still experimental, do we want to bring experimental executor intoComponentManager
? I am not sure about this, any thoughts?- Before adding
EventsExecutor
toComponentManagerIsolated
, probably we would want to supportComponentManager
?
IMO, this feature is really nice to have. but i think EventsExecutor
should support simulation time before this support, see #2480
Before we move forward with this, I think it'd be good to do @fujitatomoya's comment of adding this to the |
we should have a discussion on what would be the steps necessary to bring it out of experimental, but IMO this shouldn't prevent from using it here (especially because it's not the default).
I agree. |
Your last arguments ( |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
9bcecb7
to
b790207
Compare
@Timple the PR looks good. |
b790207
to
b2c64b0
Compare
Rebased |
Signed-off-by: Tim Clephas <[email protected]>
Signed-off-by: Tim Clephas <[email protected]>
Signed-off-by: Tim Clephas <[email protected]>
No description provided.