Skip to content

add automatically_add_executor_with_node option#1594

Merged
wjwwood merged 4 commits intoros2:masterfrom
wyca-robotics:Add_lifecycle_callback_group_option
Mar 25, 2021
Merged

add automatically_add_executor_with_node option#1594
wjwwood merged 4 commits intoros2:masterfrom
wyca-robotics:Add_lifecycle_callback_group_option

Conversation

@BriceRenaudeau
Copy link
Copy Markdown
Contributor

@BriceRenaudeau BriceRenaudeau commented Mar 24, 2021

This PR tries to implement #1589

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

fujitatomoya
fujitatomoya reviewed Mar 24, 2021
View reviewed changes
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Mar 24, 2021

@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
add automatically_add_executor_with_node option
ff3102a 
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
Fix typo
0247a45 
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
wjwwood
wjwwood requested changes Mar 24, 2021
View reviewed changes
Copy link
Copy Markdown
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?

rclcpp/rclcpp_lifecycle/test/test_lifecycle_node.cpp

Lines 663 to 675 in 0088e35

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
add option usage in test
ec33bc6 
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
Document parameter
b442e26 
Signed-off-by: Brice <brice.renaudeau@wyca.fr>
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 25, 2021

@ros-pull-request-builder retest this please

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

wjwwood commented Mar 25, 2021

CI:

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

@wjwwood wjwwood merged commit 92ece94 into ros2:master Mar 25, 2021
@fujitatomoya fujitatomoya mentioned this pull request Mar 26, 2021
Closed
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

@wjwwood wjwwood wjwwood approved these changes

Assignees

@wjwwood wjwwood

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BriceRenaudeau @fujitatomoya @wjwwood