Add controller dependency graph for ::AUTO switch mode by saikishor · Pull Request #2578 · ros-controls/ros2_control

saikishor · 2025-09-24T13:16:29Z

This PR aims to add the things needed for the ::AUTO switch mode and addresses few other things.

With the ::AUTO switch mode, you can activate and deactivate the whole chain

graph LR;
    A-->B;
    B-->C;
    C-->D;
    D-->E;

Here, you can activate the whole chain, activating controller A alone and deactivate the whole chain deactivating the controller E

graph LR;
    A-->B;
    B-->C;
    B-->D;
    D-->E;

Here, you can activate the whole chain, activating controller A alone and deactivate the whole chain deactivating the controller E and C

Activating controller D will only activate D and E
Activating controller B will activate B, C, D and E

…decessors

…rmation

codecov · 2025-09-24T13:34:56Z

Codecov Report

❌ Patch coverage is 91.12150% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.35%. Comparing base (56fc052) to head (cb75751).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ger/include/controller_manager/controller_spec.hpp	86.86%	13 Missing and 13 partials ⚠️
controller_manager/src/controller_manager.cpp	86.20%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2578      +/-   ##
==========================================
+ Coverage   89.31%   89.35%   +0.04%     
==========================================
  Files         145      145              
  Lines       16541    16931     +390     
  Branches     1396     1426      +30     
==========================================
+ Hits        14773    15129     +356     
- Misses       1231     1249      +18     
- Partials      537      553      +16

Flag	Coverage Δ
unittests	`89.35% <91.12%> (+0.04%)`	⬆️

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

Files with missing lines	Coverage Δ
...controller_interface/controller_interface_base.hpp	`91.66% <ø> (ø)`
.../include/controller_manager/controller_manager.hpp	`96.87% <ø> (ø)`
...ontroller_manager/test/test_controller_manager.cpp	`95.92% <100.00%> (+0.38%)`	⬆️
...ller_manager/test/test_controller_manager_srvs.cpp	`99.36% <100.00%> (+0.02%)`	⬆️
...e_interface/include/hardware_interface/helpers.hpp	`100.00% <100.00%> (+3.44%)`	⬆️
hardware_interface/test/test_helpers.cpp	`93.33% <100.00%> (+7.61%)`	⬆️
controller_manager/src/controller_manager.cpp	`75.33% <86.20%> (+0.16%)`	⬆️
...ger/include/controller_manager/controller_spec.hpp	`87.19% <86.86%> (-12.81%)`	⬇️

🚀 New features to boost your workflow:

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

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

github-actions · 2025-12-01T12:38:39Z

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

mergify · 2025-12-01T12:39:12Z

This pull request is in conflict. Could you fix it @saikishor?

github-actions · 2026-01-28T12:44:47Z

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

christophfroehlich

Oops, I found an unfinished review here. What is the current state of this feature?

christophfroehlich · 2025-09-24T19:24:52Z

  pal_statistics::RegistrationsRAII stats_registrations_;
+
+public:
+  using SharedPtr = std::shared_ptr<ControllerInterfaceBase>;


should we use RCLCPP_SHARED_PTR_DEFINITIONS and RCLCPP_WEAK_PTR_DEFINITIONS from "rclcpp/macros.hpp"?

PeterMitrano

Very excited about this feature and eventually fulling implementing FORCE_AUTO! I wouldn't say that I looked closely enough for this to be a "review", but I just wanted to point out a few small things, including one of the tests not making sense to me. Thanks for all the effort here!

PeterMitrano · 2025-12-13T13:56:31Z

+    graph.add_dependency(E, F);
+
+    EXPECT_THAT(
+      graph.get_dependencies_to_activate("A"),


kind of surprising that these include the node itself. in this case "A", and "B" in the case below 🤔

Yeah, it is not so intuitive I suppose, but yes, it gives the whole chain with dependencies

PeterMitrano · 2025-12-13T14:03:14Z

+
+    EXPECT_THAT(
+      graph.get_dependencies_to_activate("A"),
+      testing::UnorderedElementsAre("A", "B", "C", "D", "E", "F"));


why E and F? am I understanding this correctly?

graph LR; A-->B; B-->C; C-->D; E-->B; E-->F;

Loading

Yes,

Inorder to activate A, we need to activate B and B needs E and E needs F, that's why. It was quite complicated while implementing it

But according to the diagram, B does not need E. Rather, E needs B, but that's irrelevant for activating A.
Am I misunderstanding this?

I am imagining it like this: Let's say B is a low level joint level controller, and A offers Cartesian control and B offers trajectory control, both on top of E. If I activate both at the same time that would be a resource conflict. So if I activate A, then we can't activate E also.

Hello!

That's not how it works

Let's say, B exports 5 interfaces and then A takes 3 of those and E takes 2 of them. If you are activating A, then as it needs B to activate, but B cannot be left with half of the interfaces taken right?, so it'll have to also activate E

Did i explain myself?

If not, I can give a longer explanation when I get access to my PC

Sorry I got "B" and "E" reversed in the statement "B offers trajectory control, both on top of E" above.

But anyways, ok let's assume that A and E do not claim the same interfaces, but rather difference ones. Even then I'm not sure I agree about leaving unclaimed interfaces. In my experience it's perfectly fine to have unclaimed interfaces. From B's perspective, it just won't be receiving any valid commands on that interface, which seems fine to me.

Thanks for taking the time to explain! I am interested in contributing more to ros controls repos :)

Hello!

We cannot leave unclaimed interfaces because we cannot know what reference to take to do the control strategy. it depends on the situation though

True, but usually that's indicated with "inf". At least , in the hardware interfaces I've implemented that's how it works. But ok, I understand your point.

Maybe I just need to think of get_dependencies_to_activate as "here are all the controllers we might need to activate". Because there will need to be some filter for the case where A and E are in conflict, because they try to claim the same interfaces, right?

Sorry for coming late to this discussion.

Have you considered a case where there's two possible controllers in the place corresponding to E in the example above? If two different controllers can potentially claim the same interfaces from B how can you go about picking which to activate?

I think supporting unclaimed interfaces is a must here, since the only unequivocal dependencies are from A to B, B to C, etc., whereas B to E may be ambiguous as stated above.

Consider the parallelism with hardware interfaces themselves. There's usually no problem with running them with no controller claiming their interfaces, is there? Should this not be handled the same way?

saikishor · 2026-03-16T22:12:09Z

+    EXPECT_THAT(graph.get_dependencies_to_activate("D"), testing::UnorderedElementsAre("D"));
+    EXPECT_THAT(
+      graph.get_dependencies_to_activate("E"),
+      testing::UnorderedElementsAre("E", "F", "B", "C", "D", "A"));


PeterMitrano · 2026-03-02T21:41:58Z

+    // controllers can be activated together with the current controller.
+
+    mutually_exclusive_predecessor_groups.clear();
+    const auto are_all_reference_interfaces_found =


this and the are_all_command_interfaces_found lambdas seem to have no captures and do essentially the same thing. Maybe it could be pulled out into one function at file scope?

PeterMitrano · 2026-03-02T21:43:06Z

+  void get_controllers_to_activate(std::vector<std::string> & controllers_to_activate) const
+  {
+    // Check the predecessors of the controller and check if they belong to the controller's state
+    // interfaces If they do, add them to the list of controllers to activatestate_itf


possible typo at the end here?

PeterMitrano · 2026-03-02T21:46:12Z

+    size_t successors_reference_interfaces_count = 0;
+    for (const auto & successor : successors)
+    {
+      successors_reference_interfaces_count += successor->reference_interfaces.size();


maybe I missed it, but this variable doesn't seem to be used anywhere else?

I'll take a look

saikishor · 2026-03-16T22:12:09Z

+    EXPECT_THAT(graph.get_dependencies_to_activate("D"), testing::UnorderedElementsAre("D"));
+    EXPECT_THAT(
+      graph.get_dependencies_to_activate("E"),
+      testing::UnorderedElementsAre("E", "F", "B", "C", "D", "A"));


github-actions · 2026-05-29T14:58:18Z

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

github-actions Bot requested review from aprotyas, bijoua29, bmagyar, christophfroehlich, mamueluth, mcbed and mhubii September 24, 2025 13:16

saikishor added 23 commits September 24, 2025 15:17

Add controllers chain dependency graph to know the successors and pre…

69efebb

…decessors

Add some tests

37b69b9

WIP: Add dependencies finder class

1a5a1ad

Add Shared and WeakPtr attributed to controller interface base

7359136

first decent version

f03e5dc

Add a utility method to check if 2 containers have common items or not

a0ffcff

remove old code

75375de

Generate mutually exclusive predecessor groups

4ec10ce

Use the build_mutually_exclusive_groups method

5841232

Rename to build_mutually_exclusive_predecessor_groups

3515c3e

Add build_mutually_exclusive_successor_groups method

1335bdb

Fix the test expectations

1b216bc

add first complete working version

c935d03

Remove unused code

4b8ff0c

Remove more unused code

c55262a

Add new build_controllers_topology_info method and move common info…

f1d6104

…rmation

Create ControllerPeerInfo and add the dependency

b13dc1c

Add some debug prints

05cccf4

remove commented tests

a7c08ad

Add more conditions to protect the infinity builds

5edda85

Add the missing check

853a28b

Add logic to have a functional AUTO mode

b60f45d

Add some tests to test the logic of the AUTO mode

3a68481

saikishor added 5 commits September 24, 2025 15:17

Add tests to the newly added add_unique_items method

7151589

Added more tests for the chaining activation

f7255c6

move to RCLCPP_INFO in logging

19d06c0

Change to DEBUG logging

36bebe8

Update print list

9c9ef20

saikishor force-pushed the add/controllers/dependency_graph branch from aeea81a to 9c9ef20 Compare September 24, 2025 13:18

christophfroehlich reviewed Sep 24, 2025

View reviewed changes

Comment thread controller_manager/src/controller_manager.cpp

saikishor and others added 2 commits September 24, 2025 21:27

Update controller_manager/src/controller_manager.cpp

cb75751

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

Merge branch 'master' into add/controllers/dependency_graph

677b5ca

christophfroehlich added this to Review triage Nov 9, 2025

christophfroehlich moved this to WIP in Review triage Nov 9, 2025

github-actions Bot added the stale label Dec 1, 2025

github-actions Bot removed the stale label Dec 2, 2025

saikishor mentioned this pull request Dec 13, 2025

Implementation of AUTO and FORCE_AUTO #2900

Open

github-actions Bot added the stale label Jan 28, 2026

christophfroehlich reviewed Feb 3, 2026

View reviewed changes

github-actions Bot removed the stale label Feb 3, 2026

PeterMitrano reviewed Mar 2, 2026

View reviewed changes

saikishor commented Mar 16, 2026

View reviewed changes

github-actions Bot added the stale label May 29, 2026

Conversation

saikishor commented Sep 24, 2025

Uh oh!

codecov Bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

github-actions Bot commented Dec 1, 2025

Uh oh!

mergify Bot commented Dec 1, 2025

Uh oh!

github-actions Bot commented Jan 28, 2026

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterMitrano left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 24, 2025 •

edited

Loading