Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add option for the events executor to the isolated component container #2541

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Add option for the events executor to the isolated component container #2541

wants to merge 3 commits into from

Conversation

Timple
Copy link
Contributor

@Timple Timple commented May 27, 2024

No description provided.

Timple
Timple commented May 27, 2024
View reviewed changes
rclcpp_components/src/component_container_isolated.cpp Outdated
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") ||
Copy link
Contributor Author

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.

@Timple Timple force-pushed the component-container-isolated-events-executor branch from 5deb093 to 2c75845 Compare May 27, 2024 09:32
fujitatomoya
fujitatomoya reviewed May 28, 2024
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a 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 into ComponentManager? I am not sure about this, any thoughts?
  • Before adding EventsExecutor to ComponentManagerIsolated, probably we would want to support ComponentManager?

IMO, this feature is really nice to have. but i think EventsExecutor should support simulation time before this support, see #2480

@audrow
Copy link
Member

audrow commented Jun 6, 2024

Before we move forward with this, I think it'd be good to do @fujitatomoya's comment of adding this to the ComponentManager as well.

@audrow audrow added the more-information-needed Further information is required label Jun 6, 2024
@alsora
Copy link
Collaborator

alsora commented Jun 7, 2024

EventsExecutor is still experimental

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).

Before adding EventsExecutor to ComponentManagerIsolated, probably we would want to support ComponentManager?

I agree.
Moreover, I wonder if we really need all these separate executables (component_container_mt.cpp, component_container.cpp, component_container_isolated.cpp). IMO we could just have 1 executable with a bunch of arguments: --isolated (true if present) and --executor-type (second argument is the name of the executor)

@Timple
Copy link
Contributor Author

Timple commented Jun 7, 2024

Your last arguments (--isolated & --executor-type) did cross my mind. But I was afraid about acceptance of larger PR's and backportability to Jazzy. But they can actually easily be extended without bracking api. So I'll happy refactor to that syntax if you agree 👍

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/the-ros-2-c-executors/38296/5

@Timple Timple force-pushed the component-container-isolated-events-executor branch 3 times, most recently from 9bcecb7 to b790207 Compare June 25, 2024 07:55
@alsora
Copy link
Collaborator

alsora commented Aug 27, 2024

@Timple the PR looks good.
Would you mind merging / rebasing your branch with rolling so that I can start the CI?

@Timple Timple force-pushed the component-container-isolated-events-executor branch from b790207 to b2c64b0 Compare August 27, 2024 05:40
@Timple
Copy link
Contributor Author

Timple commented Aug 27, 2024

Rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@alsora alsora alsora approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees
No one assigned
Labels
more-information-needed Further information is required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Timple @audrow @alsora @ros-discourse @fujitatomoya