Deactivate controllers with command interfaces to hardware on DEACTIVATE #2334

MarqRazz · 2025-06-20T18:44:28Z

While testing with hardware I found it was easiest to only return DEACTIVATE from write() so that my hardware_interface didn't have to keep track of the number of times DEACTIVATE has been returned in read(). I also found that allowing write() while the hardware is INACTIVE does not make sense because ALL commanding controllers are stopped on deactivation and the hardware would not know which command_interfaces might be allowed returning DEACTIVATE again which would cause the controller_manager to get stuck in a loop attempting to stop any controllers with with command interfaces to the hardware.

I also added 2 new test to make sure the controller_manager correctly handles DEACTIVATE on read/write.

MarqRazz · 2025-06-20T18:52:26Z

controller_manager/src/controller_manager.cpp

+      for (const auto & controller : controllers)
+      {
+        auto controller_spec = std::find_if(
+          loaded_controllers.begin(), loaded_controllers.end(),
+          [&](const controller_manager::ControllerSpec & spec)
+          { return spec.c->get_name() == controller; });
+        if (controller_spec == loaded_controllers.end())
+        {
+          RCLCPP_WARN(
+            get_logger(),
+            "Deactivate failed to find controller [%s] in loaded controllers. "
+            "This can happen due to multiple returns of 'DEACTIVATE' from [%s] read()",
+            controller.c_str(), hardware_name.c_str());
+          continue;
+        }
+        std::vector<std::string> command_interface_names;
+        extract_command_interfaces_for_controller(
+          *controller_spec, resource_manager_, command_interface_names);
+        // if this controller has command interfaces add it to the deactivate_controllers_list
+        if (!command_interface_names.empty())
+        {
+          rt_buffer_.deactivate_controllers_list.push_back(controller);
+        }
+      }
+    }


If you have suggestion for a more efficient way of finding the active command interfaces to a controller I'd appreciate it!

is there anything here that could be refactored into a function? the idea being that you've got essentially the same code duplicated below

I've got some ideas to optimize this in a better way. I can open a follow-up PR for this

I don't think it makes sense to return DEACTIVATE from read() because that interface is expected to continue to work after the hardware goes inactive. What do you guys think about warning users that this is not supported in read() and to update their code to only return DEACTIVATE in write()? If that is okay I can remove all of this code here and update the tests.

Rethinking about this. When we receive DEACTIVATE from the read, it should work similar to the ERROR state, both read and write calls shouldn't work and for write, only write method shouldn't be called.

When DEACTIVATE is returned through read method, then it should behave exactly like error
state (No further read and write calls, unless activated) and all controllers that use any state and command interfaces should be deactivated)

When DEACTIVATE is returned through write method, then only read calls are allowed (only those use command interfaces should be deactivated)

What do you think? @MarqRazz

Yes that is how I was thinking it should work, but with the addition of warning the user when they return DEACTIVATE in read that it is treated the same as ERROR.

Well, but the component should go INACTIVE instead of ERROR right?

My takeaway from Marq's comments is that the component should transition to ERROR if the user needs to return DEACTIVATE from read(). In that case, we could provide an unsupported warning and recommend ERROR as the return value. This simplifies things conceptually.

That is what I implemented and added comments to the new section of code below. Let me know if we would prefer the hardware go INACTIVE vs ERROR.

codecov · 2025-06-23T15:01:32Z

Codecov Report

Attention: Patch coverage is 97.48428% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.01%. Comparing base (8c52010) to head (b3e6886).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	88.00%	2 Missing and 1 partial ⚠️
hardware_interface/src/resource_manager.cpp	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
+ Coverage   88.98%   89.01%   +0.03%     
==========================================
  Files         147      147              
  Lines       16708    16785      +77     
  Branches     1433     1438       +5     
==========================================
+ Hits        14867    14941      +74     
- Misses       1285     1286       +1     
- Partials      556      558       +2

Flag	Coverage Δ
unittests	`89.01% <97.48%> (+0.03%)`	⬆️

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

Files with missing lines	Coverage Δ
...est_controller_manager_hardware_error_handling.cpp	`100.00% <100.00%> (ø)`
hardware_interface/src/system.cpp	`81.02% <100.00%> (+0.41%)`	⬆️
...rface/test/mock_components/test_generic_system.cpp	`99.72% <ø> (ø)`
...dware_interface/test/test_component_interfaces.cpp	`97.65% <100.00%> (-0.03%)`	⬇️
...e_interface_testing/test/test_resource_manager.cpp	`99.45% <100.00%> (ø)`
hardware_interface/src/resource_manager.cpp	`78.26% <85.71%> (-0.03%)`	⬇️
controller_manager/src/controller_manager.cpp	`74.24% <88.00%> (+0.13%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bmagyar

the return type is a slight ABI break but it's internal API

saikishor

LGTM

saikishor · 2025-06-24T18:09:49Z

controller_manager/src/controller_manager.cpp

+      for (const auto & controller : controllers)
+      {
+        auto controller_spec = std::find_if(
+          loaded_controllers.begin(), loaded_controllers.end(),
+          [&](const controller_manager::ControllerSpec & spec)
+          { return spec.c->get_name() == controller; });
+        if (controller_spec == loaded_controllers.end())
+        {
+          RCLCPP_WARN(
+            get_logger(),
+            "Deactivate failed to find controller [%s] in loaded controllers. "
+            "This can happen due to multiple returns of 'DEACTIVATE' from [%s] read()",
+            controller.c_str(), hardware_name.c_str());
+          continue;
+        }
+        std::vector<std::string> command_interface_names;
+        extract_command_interfaces_for_controller(
+          *controller_spec, resource_manager_, command_interface_names);
+        // if this controller has command interfaces add it to the deactivate_controllers_list
+        if (!command_interface_names.empty())
+        {
+          rt_buffer_.deactivate_controllers_list.push_back(controller);
+        }
+      }
+    }


I've got some ideas to optimize this in a better way. I can open a follow-up PR for this

controller_manager/src/controller_manager.cpp

hardware_interface/src/resource_manager.cpp

MarqRazz · 2025-06-25T16:34:41Z

hardware_interface/src/resource_manager.cpp

+      if (ret_val == hardware_interface::return_type::DEACTIVATE)
+      {
+        RCLCPP_WARN(
+          get_logger(), "DEACTIVATE returned from read cycle is treated the same as ERROR.");
+        ret_val = hardware_interface::return_type::ERROR;
+      }


I implemented this as DEACTIVATE == ERROR in read(). If we want the hardware to go INACTIVE here it would be a simple change but can't think of a use case where a user would want to return DEACTIVATE vs ERROR in read. Would that mean they can't read from the device but they are still connected to the hardware and want to stop updating state??

…ATE (#2334) (cherry picked from commit 24d60ed)

…ATE (#2334) (#2341)

Deactivate controllers with command interfaces to hardware on DEACTIVATE

66edf54

github-actions bot requested review from ARK3r, arne48, christophfroehlich, DasRoteSkelett, destogl, olivier-stasse and progtologist June 20, 2025 18:44

remove extra logging

3e08ba9

MarqRazz commented Jun 20, 2025

View reviewed changes

finish cleaning up test_resource_manager

1512180

christophfroehlich mentioned this pull request Jun 24, 2025

Reset the PIDs states when controller is at rest ros-controls/ros2_controllers#1666

Open

6 tasks

bmagyar added the backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 jazzy. label Jun 24, 2025

bmagyar previously approved these changes Jun 24, 2025

View reviewed changes

saikishor previously approved these changes Jun 24, 2025

View reviewed changes

Juliaj reviewed Jun 25, 2025

View reviewed changes

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved

DEACTIVATE == ERROR in read

8622bca

MarqRazz dismissed stale reviews from saikishor and bmagyar via 8622bca June 25, 2025 16:30

Merge branch 'master' into deactivate_controllers

abb9d3c

saikishor reviewed Jun 25, 2025

View reviewed changes

hardware_interface/src/resource_manager.cpp Outdated Show resolved Hide resolved

hardware_interface/src/resource_manager.cpp Outdated Show resolved Hide resolved

MarqRazz commented Jun 25, 2025

View reviewed changes

address review

b3e6886

bmagyar approved these changes Jun 26, 2025

View reviewed changes

bmagyar merged commit 24d60ed into ros-controls:master Jun 26, 2025
20 of 26 checks passed

mergify bot pushed a commit that referenced this pull request Jun 26, 2025

Deactivate controllers with command interfaces to hardware on DEACTIV…

a3dc13d

…ATE (#2334) (cherry picked from commit 24d60ed)

mergify bot mentioned this pull request Jun 26, 2025

Deactivate controllers with command interfaces to hardware on DEACTIVATE (backport #2334) #2341

Merged

bmagyar pushed a commit that referenced this pull request Jun 26, 2025

Deactivate controllers with command interfaces to hardware on DEACTIV…

ca98a45

…ATE (#2334) (#2341)

MarqRazz deleted the deactivate_controllers branch June 27, 2025 15:33

MarqRazz mentioned this pull request Jul 1, 2025

Fix the prepare_command_mode_switch behaviour when HW is INACTIVE #2347

Open

KmakD mentioned this pull request Jul 8, 2025

Deactivate controllers when hardware component is inactive ros-controls/ros2_controllers#1811

Open

Deactivate controllers with command interfaces to hardware on DEACTIVATE #2334

Deactivate controllers with command interfaces to hardware on DEACTIVATE #2334

Uh oh!

Conversation

MarqRazz commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmagyar Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmagyar left a comment

Choose a reason for hiding this comment

Uh oh!

saikishor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bmagyar Jun 24, 2025 •

edited

Loading

codecov bot commented Jun 23, 2025 •

edited

Loading