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

revert call shutdown in LifecycleNode destructor (Iron) #2559

Merged
merged 2 commits into from
Jun 11, 2024
Merged

revert call shutdown in LifecycleNode destructor (Iron) #2559

merged 2 commits into from
Jun 11, 2024

Conversation

fujitatomoya
Copy link
Collaborator

revert #2543 and #2490

@fujitatomoya
Revert "lifecycle node dtor shutdown should be called only in primary…
ee2189c 
… state. (#2543)"

This reverts commit dceb612.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Revert "call shutdown in LifecycleNode dtor to avoid leaving the devi…
19f3cac 
…ce in un… (#2450) (#2490)"

This reverts commit f41a353.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
fujitatomoya
fujitatomoya commented Jun 10, 2024
View reviewed changes
rclcpp_lifecycle/src/lifecycle_node.cpp
@@ -184,7 +156,6 @@ LifecycleNode::~LifecycleNode()
node_timers_.reset();
node_logging_.reset();
node_graph_.reset();
node_base_.reset();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a bug fix, i wil resubmit this fix after reverting.

rclcpp_lifecycle/test/test_lifecycle_node.cpp
@@ -414,146 +414,6 @@ TEST_F(TestDefaultStateMachine, bad_mood) {
EXPECT_EQ(1u, test_node->number_of_callbacks);
}


TEST_F(TestDefaultStateMachine, shutdown_from_each_primary_state) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same with rolling, this is test enhancement. i will resubmit this later.

@fujitatomoya
Copy link
Collaborator Author

CI (--packages-above-and-dependencies rclcpp_lifecycle):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

This was referenced Jun 10, 2024
Merged
Closed
@fujitatomoya fujitatomoya self-assigned this Jun 10, 2024
@fujitatomoya fujitatomoya merged commit c1a01fc into iron Jun 11, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood 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

Assignees

@fujitatomoya fujitatomoya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fujitatomoya @wjwwood