Cleanup wrong lifecycle transitions in tests and unnecessary checks #1534
Conversation
github-actions
bot
requested review from
bmagyar,
harderthan,
mamueluth,
Maverobot,
olivier-stasse,
saikishor and
VX792
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1534 +/- ##
==========================================
+ Coverage 84.27% 84.49% +0.21%
==========================================
Files 123 123
Lines 11359 11297 -62
Branches 961 950 -11
==========================================
- Hits 9573 9545 -28
+ Misses 1470 1446 -24
+ Partials 316 306 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
christophfroehlich
changed the title
| @@ -101,15 +101,6 @@ controller_interface::return_type DiffDriveController::update_reference_from_sub | |||
| const rclcpp::Time & time, const rclcpp::Duration & /*period*/) | |||
| { | |||
| auto logger = get_node()->get_logger(); | |||
| if (get_lifecycle_state().id() == State::PRIMARY_STATE_INACTIVE) | |||
| { | |||
| if (!is_halted) | |||
There was a problem hiding this comment.
Do we also need to remove the is_halted variable, if it is a member variable?
There was a problem hiding this comment.
we use it in on_deactivate() transition
There was a problem hiding this comment.
Can't we simply call halt in the deactivate method always, instead of using this is_halted variable?
What do you think?
I can recheck the code later and get back to you
There was a problem hiding this comment.
I think we could, sure.
| if (!is_halted) | ||
| { | ||
| halt(); | ||
| is_halted = true; | ||
| } |
There was a problem hiding this comment.
@christophfroehlich why do we need this here?
There was a problem hiding this comment.
to stop the vehicle in deactivate transition
There was a problem hiding this comment.
Same as above, can't we simply call halt without any check in the deactivate method?
Closes #417 and closes #405 and closes #1501