Clear registered handles of DiffDriveController on deactivate

[Noel215]

Hi,

With the current implementation, the wheel handles are not being reset when running on_deactivate method. Then, if the controller is deactivated and then activated again the handles are accumulated and that can lead to issues (See ros-controls/ros2_control_demos#239)

on_activate and on_deactivate are the opposite, so everything that is enabled in on_activate have to be disabled in on_deactivate (or reset at the beginning of the on_activate). Following that, I propose two possible solutions, let me know which one fits better:

Current Solution: Reset the handles with registered_handles.clear(); in the configure_side() method.

Alternative Solution: Clear each of the handles in on_deactivate method:

controller_interface::CallbackReturn DiffDriveController::on_deactivate(
  const rclcpp_lifecycle::State &)
{
  subscriber_is_active_ = false;
  registered_left_wheel_handles_.clear();
  registered_right_wheel_handles_.clear();
  return controller_interface::CallbackReturn::SUCCESS;
}

[codecov-commenter]

Codecov Report

Merging #596 (d57fcd3) into master (e7f9962) will increase coverage by 0.12%.
The diff coverage is 33.33%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
+ Coverage   35.78%   35.90%   +0.12%     
==========================================
  Files         189        7     -182     
  Lines       17570      674   -16896     
  Branches    11592      361   -11231     
==========================================
- Hits         6287      242    -6045     
+ Misses        994      132     -862     
+ Partials    10289      300    -9989     

Flag	Coverage Δ
unittests	35.90% <33.33%> (+0.12%)	⬆️

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

Impacted Files	Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp	11.11% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp	42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp	46.55% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp	17.62% <12.08%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp	45.70% <44.44%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp	100.00% <100.00%> (ø)
...troller/include/diff_drive_controller/odometry.hpp	100.00% <100.00%> (ø)

... and 189 files with indirect coverage changes

[bmagyar]

should this happen on deactivate?

[Noel215]

That is being executed on activate, before creating the handles.
I can change it to use the other solution if you think it's better.

[bmagyar]

Well those handles should be returned from loan on deactivate so I think it's only appropriate ;)

…

On Fri, 5 May 2023, 20:29 Noel Jiménez García, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In diff_drive_controller/src/diff_drive_controller.cpp <#596 (comment)> : > @@ -558,6 +558,7 @@ controller_interface::CallbackReturn DiffDriveController::configure_side( } // register handles + registered_handles.clear(); That is being executed on activate, before creating the handles. I can change it to use the other solution if you think it's better. — Reply to this email directly, view it on GitHub <#596 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA24PYOJXQZWXRIFEWMWBL3XEVIKDANCNFSM6AAAAAAXWLDTOI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[bmagyar]

Houston, we have a problem. Some tests are failing and they seem legit caused by this PR. Possible they are badly designed tests...

[christophfroehlich]

Thanks for pointing in this direction, I was not able to find this on my own ;)

The tests fail now, because it is assumed that the commands are zero after deactivation

ros2_controllers/diff_drive_controller/test/test_diff_drive_controller.cpp

Lines 424 to 425 in 546422d

	EXPECT_EQ(0.0, left_wheel_vel_cmd_.get_value()) << "Wheels are halted on deactivate()";
	EXPECT_EQ(0.0, right_wheel_vel_cmd_.get_value()) << "Wheels are halted on deactivate()";

This is now not the case, because DiffDriveController::halt() has no effect if no handles are registered.

ros2_controllers/diff_drive_controller/src/diff_drive_controller.cpp

Line 109 in 546422d

halt();

Let it explicitly halt before releasing the handles?

[bmagyar]

@Mergifyio backport humble

[mergify]

backport humble

✅ Backports have been created

• #606 Clear registered handles of DiffDriveController on deactivate (backport #596) has been created for branch humble