Add additional checks for non existing and not available interfaces. #1218

destogl · 2023-12-12T17:48:27Z

Fixing issues where controllers "prepare" and "perform" switch might be called even if HW is not configured which could lead to crashes.
Now only calls with all existing and available interface are allowed and on HW that is in inactive of active state.

saikishor · 2023-12-13T15:47:32Z

@destogl can we also get this PR : #1098 into account, as it handles similar cases

destogl · 2023-12-13T15:55:18Z

Rethinking this in the context of #1098 - I would change #1098, but then we don't have nicely shown all three states... (if this is needed at all) If we go with #1098 then the check in the Resource Manage will become much more complex. I am not sure if I want to go that route because I think that we gain nothing having this complexity. The available interface in the context of controller doesn't make much sense. So I propose that we have the following case:

New behavior:

Configured chainable controller: Listed exported interfaces are available and unclaimed
Active chainable controller(not in chained mode): Listed exported interfaces are available but unclaimed
Active chainable controller(in chained mode): Listed exported interfaces are available and claimed

codecov · 2023-12-13T15:55:50Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (477c85d) 47.85% compared to head (1e9f8a6) 48.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
+ Coverage   47.85%   48.02%   +0.17%     
==========================================
  Files          41       41              
  Lines        3477     3525      +48     
  Branches     1889     1912      +23     
==========================================
+ Hits         1664     1693      +29     
+ Misses        446      442       -4     
- Partials     1367     1390      +23

Flag	Coverage Δ
unittests	`48.02% <55.38%> (+0.17%)`	⬆️

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

Files	Coverage Δ
hardware_interface/src/resource_manager.cpp	`48.90% <55.38%> (+1.18%)`	⬆️

... and 1 file with indirect coverage changes

saikishor · 2023-12-14T19:18:38Z

Rethinking this in the context of #1098 - I would change #1098, but then we don't have nicely shown all three states... (if this is needed at all) If we go with #1098 then the check in the Resource Manage will become much more complex. I am not sure if I want to go that route because I think that we gain nothing having this complexity. The available interface in the context of controller doesn't make much sense. So I propose that we have the following case:

New behavior:

Configured chainable controller: Listed exported interfaces are available and unclaimed

Active chainable controller(not in chained mode): Listed exported interfaces are available but unclaimed

Active chainable controller(in chained mode): Listed exported interfaces are available and claimed

@destogl I'm not sure about this convention. IMO the available interface does make sense in the context of the controllers as well. For instance, until the controller is active they are not exported to the Resource manager and also it doesn't make sense to export them on configure as the controller is not ready yet and you cannot start chaining them.

I believe they should be available only upon activation and unavailable upon deactivation.

mergify · 2023-12-17T16:34:20Z

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

…ive state.

…in inactive state." This reverts commit 485b25f.

destogl · 2023-12-18T09:35:54Z

@saikishor you are right. I will adjust this PR if needed. I would expect that I need adjustments, but the tests are passing. Let me today test in a real-world scenario if everything works, we can also start merging this :)

Thanks for your support and patience! (the feature is quite tricky and I am careful to not break anything - so writing many tests...)

…ong.

… about it.

saikishor

Looks fine to me. Test it and let us know

hardware_interface/src/resource_manager.cpp

hardware_interface/test/test_resource_manager_prepare_perform_switch.cpp

destogl · 2023-12-18T18:21:25Z

This works on the HW, so we can move one with it, if all are happy.

saikishor · 2024-01-02T18:51:30Z

@destogl can you rebase the PR?

…existing-and-available-interfaces

Co-authored-by: Sai Kishor Kothakota <[email protected]>

mergify · 2024-01-04T15:58:14Z

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

hardware_interface/include/hardware_interface/resource_manager.hpp

…existing-and-available-interfaces

hardware_interface/test/test_resource_manager.hpp

Co-authored-by: Bence Magyar <[email protected]>

hardware_interface/src/resource_manager.cpp

saikishor

The changes itself look good to me @destogl
Great job 👏

hardware_interface/src/resource_manager.cpp

Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]>

…existing-and-available-interfaces

…1218) (cherry picked from commit 8c34ab6) # Conflicts: # hardware_interface/src/resource_manager.cpp # hardware_interface/test/mock_components/test_generic_system.cpp # hardware_interface/test/test_resource_manager.cpp

…1218) (cherry picked from commit 8c34ab6) # Conflicts: # hardware_interface/test/mock_components/test_generic_system.cpp

…1218) (cherry picked from commit 8c34ab6) # Conflicts: # hardware_interface/src/resource_manager.cpp # hardware_interface/test/mock_components/test_generic_system.cpp # hardware_interface/test/test_resource_manager.cpp

…(backport #1218) (#1291) * Add additional checks for non-existing and not available interfaces. (#1218) (cherry picked from commit 8c34ab6) * Resolve conflicts for backport. * Make tests to use hardware description in tests to be able to activate controllers. * Fix variable name for clarity. --------- Co-authored-by: Dr. Denis <[email protected]>

…(backport #1218) (#1292) * Add additional checks for non-existing and not available interfaces. (#1218) (cherry picked from commit 8c34ab6) * Resolve conflicts for backport. * Make tests to use hardware description in tests to be able to activate controllers. --------- Co-authored-by: Dr. Denis <[email protected]> Co-authored-by: Bence Magyar <[email protected]>

destogl added bug enhancement labels Dec 12, 2023

destogl self-assigned this Dec 12, 2023

destogl added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron high-priority labels Dec 12, 2023

destogl force-pushed the allow-prepare-comand-mode-switch-only-for-existing-and-available-interfaces branch from 19199ac to 37e64a8 Compare December 13, 2023 15:48

destogl added 3 commits December 17, 2023 19:42

Add additional checks for non existing and not available interfaces.

c575b4e

Protect components from being called only in inactive and active states.

f9766e2

Make reference interfaces of chainable controllers available in inact…

485b25f

…ive state.

destogl force-pushed the allow-prepare-comand-mode-switch-only-for-existing-and-available-interfaces branch from 37e64a8 to 485b25f Compare December 17, 2023 18:48

Revert "Make reference interfaces of chainable controllers available …

d033cf8

…in inactive state." This reverts commit 485b25f.

destogl added 2 commits December 18, 2023 10:37

[RM] Separate tests for prepare and perform switch because they are l…

44e3b72

…ong.

Optimize prepre/perform switch in RM and add additional documentation…

0a6b107

… about it.

destogl mentioned this pull request Dec 18, 2023

[CM] Remove accepting robot description using parameter and harden the behavior when getting it from topic. #1237

Closed

saikishor reviewed Dec 18, 2023

View reviewed changes

github-actions bot requested review from bijoua29, bmagyar, livanov93, olivier-stasse, saikishor and VanshGehlot December 27, 2023 12:28

destogl and others added 2 commits January 3, 2024 19:14

Merge branch 'master' into allow-prepare-comand-mode-switch-only-for-…

3275d39

…existing-and-available-interfaces

Make if nicer and more efficient

3162b29

Co-authored-by: Sai Kishor Kothakota <[email protected]>

saikishor mentioned this pull request Jan 4, 2024

Loading robot_description from topic does not work as expected #1262

Closed

bmagyar reviewed Jan 8, 2024

View reviewed changes

hardware_interface/include/hardware_interface/resource_manager.hpp Outdated Show resolved Hide resolved

Merge branch 'master' into allow-prepare-comand-mode-switch-only-for-…

df49ff8

…existing-and-available-interfaces

destogl commented Jan 8, 2024

View reviewed changes

hardware_interface/test/test_resource_manager.hpp Show resolved Hide resolved

destogl and others added 2 commits January 8, 2024 19:59

Update hardware_interface/test/test_resource_manager.hpp

46559ce

Fix some typos

77884b9

Co-authored-by: Bence Magyar <[email protected]>

bmagyar requested changes Jan 8, 2024

View reviewed changes

hardware_interface/src/resource_manager.cpp Show resolved Hide resolved

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

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

saikishor reviewed Jan 9, 2024

View reviewed changes

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

saikishor approved these changes Jan 9, 2024

View reviewed changes

destogl commented Jan 9, 2024

View reviewed changes

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

destogl and others added 4 commits January 9, 2024 17:43

Rename lambda method for clarity.

80ef781

Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]>

Merge branch 'master' into allow-prepare-comand-mode-switch-only-for-…

25a1da2

…existing-and-available-interfaces

Merge branch 'master' into allow-prepare-comand-mode-switch-only-for-…

bcd6043

…existing-and-available-interfaces

Merge branch 'master' into allow-prepare-comand-mode-switch-only-for-…

1e9f8a6

…existing-and-available-interfaces

destogl requested a review from bmagyar January 12, 2024 17:19

bmagyar approved these changes Jan 12, 2024

View reviewed changes

bmagyar merged commit 8c34ab6 into master Jan 12, 2024
13 checks passed

mergify bot mentioned this pull request Jan 12, 2024

Add additional checks for non existing and not available interfaces. (backport #1218) #1291

Merged

mergify bot pushed a commit that referenced this pull request Jan 12, 2024

Add additional checks for non existing and not available interfaces. (#…

c48b117

…1218) (cherry picked from commit 8c34ab6) # Conflicts: # hardware_interface/test/mock_components/test_generic_system.cpp

mergify bot mentioned this pull request Jan 12, 2024

Add additional checks for non existing and not available interfaces. (backport #1218) #1292

Merged

destogl deleted the allow-prepare-comand-mode-switch-only-for-existing-and-available-interfaces branch January 12, 2024 18:20

saikishor mentioned this pull request Feb 16, 2024

Fix multiple chainable controller activation bug #1401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional checks for non existing and not available interfaces. #1218

Add additional checks for non existing and not available interfaces. #1218

destogl commented Dec 12, 2023

saikishor commented Dec 13, 2023

destogl commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

saikishor commented Dec 14, 2023

mergify bot commented Dec 17, 2023

destogl commented Dec 18, 2023

saikishor left a comment

destogl commented Dec 18, 2023

saikishor commented Jan 2, 2024

mergify bot commented Jan 4, 2024

saikishor left a comment

Add additional checks for non existing and not available interfaces. #1218

Add additional checks for non existing and not available interfaces. #1218

Conversation

destogl commented Dec 12, 2023

saikishor commented Dec 13, 2023

destogl commented Dec 13, 2023 • edited Loading

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

saikishor commented Dec 14, 2023

mergify bot commented Dec 17, 2023

destogl commented Dec 18, 2023

saikishor left a comment

Choose a reason for hiding this comment

destogl commented Dec 18, 2023

saikishor commented Jan 2, 2024

mergify bot commented Jan 4, 2024

saikishor left a comment

Choose a reason for hiding this comment

destogl commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading