Skip to content

Refactor component containers#3129

Open
mini-1235 wants to merge 3 commits intoros2:rollingfrom
mini-1235:refactor/component_containers
Open

Refactor component containers#3129
mini-1235 wants to merge 3 commits intoros2:rollingfrom
mini-1235:refactor/component_containers

Conversation

@mini-1235
Copy link
Copy Markdown
Contributor

Description

Supersedes #3055

Is this user-facing behavior change?

Yes,

  • When users use component_container_mt/component_container_event/component_container_isolated, they will see a warning message
  • When using component_container without any arguments, it will default to non isolated + single threaded, so users will probably not notice this change

Did you use Generative AI?

Yes, Github Copilot

Additional Information

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@skyegalaxy
Copy link
Copy Markdown
Member

Would we be able to also include component containers for the new callback group events executor? I had a draft PR to do so but it was blocked by several cascading PRs waiting on green CI to get in

@mini-1235
Copy link
Copy Markdown
Contributor Author

Sure :) I didn't include it here since this PR is mainly focused on refactoring the existing component containers. Should I go ahead and include it in this one?

@mini-1235
Copy link
Copy Markdown
Contributor Author

I took a look through the core repositories, and I don't think anything is currently using component_container_mt/event/isolated, the only thing that seems to need updating is ros2/ros2_documentation#6401

@skyegalaxy
Copy link
Copy Markdown
Member

Sure :) I didn't include it here since this PR is mainly focused on refactoring the existing component containers. Should I go ahead and include it in this one?

Per today's Client Library WG meeting: yes, let's replace the experimental events executor in this PR with the EventsCBGExecutor. Could you also add options for single threaded vs multi threaded mode, similar to https://github.com/ros2/rclcpp/pull/3123/changes#diff-a10f35c54a2ef381f2f406fe9c2de5135e2b79125fc391d7eedd66b501b786fdR58 ?

@skyegalaxy
Copy link
Copy Markdown
Member

@mini-1235 - we're hoping to land this and #3097 before the feature freeze by EOD monday. since CI is pretty busted for rhel and windows, we decided at the client library WG meeting today to be pretty aggressive with our merges if linux CI is good, so we can meet the feature freeze.

We got some C++20 PRs we're hoping to get in first, then the CBG Executor PR, then this component container PR (with the new executor). I'm dedicating today to making sure PRs get approvals and CI, so I'll try to be quick once your new changes land here :)

@mini-1235
Copy link
Copy Markdown
Contributor Author

@skyegalaxy I think I see a potential issue when applying the changes: https://github.com/ros2/rclcpp/pull/3123/changes#r3102814441

It's also pretty late for me (I am in the Asia timezone), so I will be offline in about an hour and back in ~12 hours.

If you would like to push this forward today, feel free to edit or take over the PR 😄

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants