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.

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 automatically_add_executor_with_node option #1594

Merged

Conversation

BriceRenaudeau
Copy link
Contributor

This PR tries to implement #1589

It adds the automatically_add_executor_with_node option when creating a callback_group for a LifecycleNode.

@fujitatomoya
Copy link
Collaborator

@BriceRenaudeau

thanks for opening PR 👍

could you also take care of DCO failure https://github.com/ros2/rclcpp/pull/1594/checks?check_run_id=2183190459?

@BriceRenaudeau BriceRenaudeau force-pushed the Add_lifecycle_callback_group_option branch from 55543bf to 5f62c9a Compare March 24, 2021 17:37
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
@BriceRenaudeau BriceRenaudeau force-pushed the Add_lifecycle_callback_group_option branch from 5f62c9a to ff3102a Compare March 24, 2021 17:41
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since this API is not covered by https://github.com/ros2/rclcpp/blob/master/rclcpp/test/rclcpp/test_add_callback_groups_to_executor.cpp, can you maybe add some code here to just exercise the new API?

TEST_F(TestDefaultStateMachine, test_callback_groups) {
auto test_node = std::make_shared<EmptyLifecycleNode>("testnode");
auto groups = test_node->get_callback_groups();
EXPECT_EQ(groups.size(), 1u);
auto group = test_node->create_callback_group(
rclcpp::CallbackGroupType::MutuallyExclusive);
EXPECT_NE(nullptr, group);
groups = test_node->get_callback_groups();
EXPECT_EQ(groups.size(), 2u);
EXPECT_EQ(groups[1].lock().get(), group.get());
}

Brice added 2 commits March 25, 2021 09:40
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
@wjwwood
Copy link
Member

wjwwood commented Mar 25, 2021

@ros-pull-request-builder retest this please

@wjwwood wjwwood self-assigned this Mar 25, 2021
@wjwwood
Copy link
Member

wjwwood commented Mar 25, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

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.

3 participants