Fix multiple calls of cleanup when system is unconfigured

[destogl]

Replaces #1175

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (71506d5) 47.87% compared to head (b183b0f) 47.90%.

❗ Current head b183b0f differs from pull request most recent head 2aa0d02. Consider uploading reports for the commit 2aa0d02 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
+ Coverage   47.87%   47.90%   +0.03%     
==========================================
  Files          41       41              
  Lines        3453     3463      +10     
  Branches     1878     1883       +5     
==========================================
+ Hits         1653     1659       +6     
- Misses        444      445       +1     
- Partials     1356     1359       +3     

Flag	Coverage Δ
unittests	47.90% <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 4 files with indirect coverage changes

[christophfroehlich]

Should we also return OK in PRIMARY_STATE_FINALIZED? This would maybe fix #1103, see the last point of the instructions in ros-controls/ros2_control_demos#417

[destogl]

Oh, yes, will do tomorrow morning. :)

[saikishor]

LGTM. Thank you @destogl.

One question, why don't we use return type OK by default, but when we enter the INACTIVE and ACTIVE state we set it the ERROR at the beginning after the condition, in such a way we don't have to code OK for every lifecycle state.

(or)

Something as simple as if and else and by default we return OK in else

  if (
    impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE ||
    impl_->get_state().id() == lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE)
  {
    return_type result = impl_->read(time, period);
    if (result == return_type::ERROR)
    {
      error();
    }  
    return result;
  }
  else
  {
    return return_type::OK;
  }
}

What do you think?

In any case, the current changes look fine to me. :)

[destogl]

@saikishor, I see your point here. This is definitely a simpler code, but I would like to have it rather explicit. Any unplanned thing should end up in error and OK state should be set explict.

[saikishor]

@saikishor, I see your point here. This is definitely a simpler code, but I would like to have it rather explicit. Any unplanned thing should end up in error and OK state should be set explict.

Make sense. I agree with you. Thanks for the explanation. 👍

[christophfroehlich]

Now it fixes #1103 too, thanks!