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.
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.
Timple
force-pushed
the
component-container-isolated-events-executor
branch
from
5deb093 to
2c75845
Compare
There was a problem hiding this comment.
a couple of questions,
EventsExecutoris still experimental, do we want to bring experimental executor intoComponentManager? I am not sure about this, any thoughts?- Before adding
EventsExecutortoComponentManagerIsolated, 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: |
Timple
force-pushed
the
component-container-isolated-events-executor
branch
3 times, most recently
from
9bcecb7 to
b790207
Compare
|
@Timple the PR looks good. |
Timple
force-pushed
the
component-container-isolated-events-executor
branch
from
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]>
|
Why is this blocked? |
There was a problem hiding this comment.
previous comments #2541 (review) and #2541 (comment), probably we can do this kind of refactoring in rolling without backporting. so i think those do not need to be addressed with this PR.
| node = std::make_shared<ComponentManagerIsolated>(exec); | ||
| } else { | ||
| std::cerr << "Invalid executor type: " << executor_type << std::endl; |
There was a problem hiding this comment.
this is actually behavior change compared to before. before, even if user specify the wrong option, that falls back to SingleThreadedExecutor, but now it fails and user does not really know what can be set. it would be probably nice to show what and how executor types are set? or command line helper function can be introduced?
| using executor = rclcpp::executors::MultiThreadedExecutor; | ||
| using ComponentManagerIsolated = rclcpp_components::ComponentManagerIsolated<executor>; | ||
| exec = std::make_shared<executor>(); | ||
| node = std::make_shared<ComponentManagerIsolated>(exec); |
There was a problem hiding this comment.
this is also behavior change compared to before. before, it creates ComponentManagerIsolated node with SingleThreadedExecutor always. but now creating ComponentManagerIsolated node with MultiThreadedExecutor, i do not think this is needed since component node is expected to do the simple task like load/unload the components. is this intentional change?
| break; | ||
| } | ||
| } else if (args[i] == "--use_multi_threaded_executor") { // backward compatibility | ||
| executor_type = "multi-threaded"; |
There was a problem hiding this comment.
it would be probably nice to add message for user, that this option is deprecated?
No description provided.