fix multiple system unconfigured rise cleanup #1175

anscipione · 2023-11-21T21:22:56Z

destogl · 2023-12-04T18:52:03Z

Hi @anscipione, thanks for your PR and reporting the issue. Have you checked the test locally? This change breaks the tests.

I have a proposal to make the change more readable. Please add these lines at the very beginning of the methods.

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)
{
  return return_type::OK;
}

And we should update the tests too.

- fix system components - fix other components

…nents

anscipione · 2023-12-12T22:20:07Z

Hi @anscipione, thanks for your PR and reporting the issue. Have you checked the test locally? This change breaks the tests.

I have a proposal to make the change more readable. Please add these lines at the very beginning of the methods.
if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)
{
  return return_type::OK;
}
And we should update the tests too.

I applied the suggested change.
I also fixed the tests, but I would prefer you to check them.

…figured_components

codecov · 2023-12-14T18:41:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (64c9e2a) 47.89% compared to head (7f39020) 47.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   47.89%   47.93%   +0.03%     
==========================================
  Files          41       41              
  Lines        3451     3461      +10     
  Branches     1876     1881       +5     
==========================================
+ Hits         1653     1659       +6     
- Misses        444      446       +2     
- Partials     1354     1356       +2

Flag	Coverage Δ
unittests	`47.93% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
hardware_interface/src/actuator.cpp	`58.77% <100.00%> (+1.49%)`	⬆️
hardware_interface/src/sensor.cpp	`49.48% <100.00%> (-1.05%)`	⬇️
hardware_interface/src/system.cpp	`58.77% <100.00%> (+1.49%)`	⬆️

... and 1 file with indirect coverage changes

…nents

hardware_interface/src/actuator.cpp

destogl · 2023-12-18T17:48:44Z

hardware_interface/src/sensor.cpp

@@ -197,6 +197,11 @@ return_type Sensor::read(const rclcpp::Time & time, const rclcpp::Duration & per
 {
  // TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"


Suggested change

// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl · 2023-12-18T17:49:09Z

hardware_interface/src/sensor.cpp

+  if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)
+  {
+    return return_type::OK;
+  }
+


Suggested change

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl · 2023-12-18T17:49:18Z

hardware_interface/src/system.cpp

@@ -216,6 +216,11 @@ return_type System::read(const rclcpp::Time & time, const rclcpp::Duration & per
 {
  // TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"


Suggested change

// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl · 2023-12-18T17:49:29Z

hardware_interface/src/system.cpp

+  if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)
+  {
+    return return_type::OK;
+  }
+


Suggested change

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl · 2023-12-18T17:49:37Z

hardware_interface/src/system.cpp

@@ -233,6 +238,11 @@ return_type System::write(const rclcpp::Time & time, const rclcpp::Duration & pe
 {
  // TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"


Suggested change

// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl · 2023-12-18T17:49:59Z

hardware_interface/src/system.cpp

+  if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)
+  {
+    return return_type::OK;
+  }
+


Suggested change

if (impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED)

{

return return_type::OK;

}

destogl

We have to move these methods as in the proposals. I cannot merge those directly, unfortunately.

…nents

saikishor · 2024-01-06T15:24:27Z

Hello @anscipione!

Do you have time to address the comments of @destogl? or shall i take over and fix it in a different PR?. Right now, I ended up doing the same without knowing about this PR

Thank you,

anscipione · 2024-01-06T15:45:46Z

I'll fix asap.

Co-authored-by: Dr. Denis <[email protected]>

…nents

destogl · 2024-01-08T18:44:00Z

I couldn't introduce changes to this, so opened a follow-up #1279

github-actions bot requested review from bmagyar, destogl, erickisos, harderthan, peterdavidfagan, rosterloh and sachinkum0009 November 21, 2023 21:23

anscipione force-pushed the fix_multiple_cleanup_on_unconfigured_components branch from 636616e to 59b72d1 Compare December 1, 2023 08:57

anscipione force-pushed the fix_multiple_cleanup_on_unconfigured_components branch from 59b72d1 to deb4753 Compare December 12, 2023 22:05

fix multiple system rise cleanup

63b725d

- fix system components - fix other components

anscipione force-pushed the fix_multiple_cleanup_on_unconfigured_components branch from b47e71a to 63b725d Compare December 12, 2023 22:17

Merge branch 'master' into fix_multiple_cleanup_on_unconfigured_compo…

5ad0199

…nents

Merge branch 'ros-controls:master' into fix_multiple_cleanup_on_uncon…

653f2d0

…figured_components

destogl mentioned this pull request Dec 14, 2023

Add additional return value to the hardware_interface::return_type #815

Closed

destogl linked an issue Dec 14, 2023 that may be closed by this pull request

Add additional return value to the hardware_interface::return_type #815

Closed

destogl previously approved these changes Dec 14, 2023

View reviewed changes

destogl linked an issue Dec 14, 2023 that may be closed by this pull request

fix multiple system unconfigured rise cleanup #1171

Closed

Merge branch 'master' into fix_multiple_cleanup_on_unconfigured_compo…

5184402

…nents

anscipione dismissed destogl’s stale review via 5184402 December 14, 2023 20:40

anscipione force-pushed the fix_multiple_cleanup_on_unconfigured_components branch from 7f8af64 to 5184402 Compare December 14, 2023 20:40

saikishor approved these changes Dec 15, 2023

View reviewed changes

destogl reviewed Dec 18, 2023

View reviewed changes

destogl added a commit to SchillingRobotics/ros2_control that referenced this pull request Dec 18, 2023

Protect 'read' and 'write' calls accoring to lifecycle ros-controls#1175

91b6d9f

Merge branch 'master' into fix_multiple_cleanup_on_unconfigured_compo…

40a3d14

…nents

saikishor mentioned this pull request Jan 6, 2024

Ressource manager hangs up on lifecycle changes #1103

Closed

anscipione and others added 2 commits January 7, 2024 08:20

Apply suggestions from code review

1c238f0

Co-authored-by: Dr. Denis <[email protected]>

Merge branch 'master' into fix_multiple_cleanup_on_unconfigured_compo…

7f39020

…nents

destogl mentioned this pull request Jan 8, 2024

Fix multiple calls of cleanup when system is unconfigured #1279

Merged

destogl closed this Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix multiple system unconfigured rise cleanup #1175

fix multiple system unconfigured rise cleanup #1175

anscipione commented Nov 21, 2023

destogl commented Dec 4, 2023

anscipione commented Dec 12, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

destogl Dec 18, 2023

destogl Dec 18, 2023

destogl Dec 18, 2023

destogl Dec 18, 2023

destogl Dec 18, 2023

destogl Dec 18, 2023

destogl left a comment

saikishor commented Jan 6, 2024

anscipione commented Jan 6, 2024

destogl commented Jan 8, 2024

		@@ -197,6 +197,11 @@ return_type Sensor::read(const rclcpp::Time & time, const rclcpp::Duration & per
		{
		// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

		@@ -216,6 +216,11 @@ return_type System::read(const rclcpp::Time & time, const rclcpp::Duration & per
		{
		// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

		@@ -233,6 +238,11 @@ return_type System::write(const rclcpp::Time & time, const rclcpp::Duration & pe
		{
		// TODO(destogl): discuss what should be default return value, e.g., "NOT_EXECUTED"

fix multiple system unconfigured rise cleanup #1175

fix multiple system unconfigured rise cleanup #1175

Conversation

anscipione commented Nov 21, 2023

destogl commented Dec 4, 2023

anscipione commented Dec 12, 2023

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl Dec 18, 2023

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

saikishor commented Jan 6, 2024

anscipione commented Jan 6, 2024

destogl commented Jan 8, 2024

codecov bot commented Dec 14, 2023 •

edited

Loading