Fix duplicate fallback controllers from the controller chain

[saikishor]

Fixes: #3038

[Juliaj]

Fixes: #3038

Hi Sai, is there another issue that this PR fixes ? #3038 appears to be different from having duplicating fallback controllers. The scenario involves putting two (or more) controllers in the same chain group, each with a different fallback.

[codecov]

Codecov Report

❌ Patch coverage is 98.36066% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.38%. Comparing base (152ed06) to head (90e84e8).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ontroller_manager/test/test_controller_manager.cpp	98.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3108      +/-   ##
==========================================
+ Coverage   89.35%   89.38%   +0.03%     
==========================================
  Files         158      158              
  Lines       19392    19452      +60     
  Branches     1573     1573              
==========================================
+ Hits        17327    17388      +61     
  Misses       1419     1419              
+ Partials      646      645       -1     

Flag	Coverage Δ
unittests	89.38% <98.36%> (+0.03%)	⬆️

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

Files with missing lines	Coverage Δ
controller_manager/src/controller_manager.cpp	77.16% <100.00%> (-0.14%)	⬇️
...ontroller_manager/test/test_controller_manager.cpp	95.75% <98.33%> (+0.10%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[bmagyar]

thank you!

[shlok-mehndiratta]

Hi @saikishor, I was studying the update loop logic and had a small thought regarding the efficiency of the RT thread.

While add_item avoids duplicate names in the fallback_controllers_list, the get_active_controllers_using_command_interfaces_of_controller(...) search still executes for every duplicate in a failed chain. Since that function performs a nested iteration over all controllers and their command interfaces, repeating it multiple times for the same fallback adds redundant overhead to the 1 kHz RT cycle.

Would it be a nice minor optimization to wrap both the add_item call and the dependency search in a single if (!ros2_control::has_item(...)) check? It would help prune those redundant searches in the critical path.

Also, just a suggestion—would it be helpful to add a test case where members of the same chain have different fallback controllers?

[bmagyar]

@shlok-mehndiratta @saikishor I'll let you guys fight it out ;) and merge this in the mean time...