Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set chained controller interfaces as available for activated controllers #1098

Merged
merged 15 commits into from
Dec 17, 2023
Merged

Set chained controller interfaces as available for activated controllers #1098

merged 15 commits into from
Dec 17, 2023

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Aug 10, 2023 •
edited
Loading

This PR addresses the issue with the current list_hardware_interfaces service which lists the interfaces exposed by the chainable controllers as unavailable instead of available until they are in chained mode

Current behavior:

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

New behavior:

  • Configured chainable controller: Listed exported interfaces are unavailable 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

@saikishor saikishor changed the title Set chained controller interfaces as available Set chained controller interfaces as available for activated controllers Aug 10, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2023 •
edited by codecov bot
Loading

Codecov Report

Merging #1098 (333e0bf) into master (3313796) will increase coverage by 0.14%.
Report is 3 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1098      +/-   ##
==========================================
+ Coverage   47.65%   47.79%   +0.14%     
==========================================
  Files          40       40              
  Lines        3448     3450       +2     
  Branches     1867     1869       +2     
==========================================
+ Hits         1643     1649       +6     
+ Misses        480      457      -23     
- Partials     1325     1344      +19
Flag Coverage Δ
unittests 47.79% <0.00%> (+0.14%) ⬆️

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

Files Coverage Δ
controller_manager/src/controller_manager.cpp 39.22% <0.00%> (+0.44%) ⬆️

christophfroehlich
christophfroehlich previously approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as far as I understand the chainable controllers.

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2023

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

@saikishor saikishor force-pushed the set_chained_controller_interfaces_as_available branch from 10bc21c to 00f29cc Compare August 11, 2023 12:43
destogl
destogl previously approved these changes Oct 15, 2023
View reviewed changes
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 3, 2023

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

@saikishor
added method to check if interface is a reference interface from a co…
d1339d6 
…ntroller in resource manager
@saikishor
set hardware interfaces to available if they belong to chainable cont…
1c7905e 
…rollers
@saikishor
added test for list_hardware_interfaces service
2824112
@saikishor
for a configured controller the exported interfaces should be unavail…
9984f92 
…able
@saikishor
update the logic to check if the controller is active or not
17d3b10
@saikishor
Revert "added method to check if interface is a reference interface f…
0f8bf2f 
…rom a controller in resource manager"

This reverts commit 6301d64.
@saikishor
remove new lines at the end of the file
6bcfaad
@saikishor
undo the logic of checking if the controller is active
4cb248c 
This reverts commit d78caff.
@saikishor
update tests so the claimed interfaces are available upon deactivation
d06f8c9
@saikishor
control reference interface availability based on the controller state
4ed6bf8
@saikishor saikishor force-pushed the set_chained_controller_interfaces_as_available branch from 94361ba to 4ed6bf8 Compare December 3, 2023 18:05
@saikishor
Copy link
Member Author

@bmagyar I think the rebase I've done after the conflict has invalidated the review of @destogl and @christophfroehlich. In this case, do they have to review again? or what's the exact process to follow?

Thank you

christophfroehlich
christophfroehlich previously approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LstillGTM, and please don't forget to update docs with details on that. E.g. here: https://control.ros.org/master/doc/ros2_control/controller_manager/doc/controller_chaining.html#debugging-outputs

@saikishor
Copy link
Member Author

still LGTM, and please don't forget to update docs with details on that. E.g. here: https://control.ros.org/master/doc/ros2_control/controller_manager/doc/controller_chaining.html#debugging-outputs

@christophfroehlich I've updated the docs now. Please review and let me know your opinion.

Thank you

christophfroehlich
christophfroehlich previously approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@saikishor saikishor mentioned this pull request Dec 13, 2023
Merged
destogl
destogl previously approved these changes Dec 17, 2023
View reviewed changes
@destogl destogl dismissed stale reviews from christophfroehlich and themself via 333e0bf December 17, 2023 14:09
@destogl destogl self-requested a review December 17, 2023 14:11
@destogl destogl merged commit b6f32c3 into ros-controls:master Dec 17, 2023
13 checks passed
@saikishor saikishor deleted the set_chained_controller_interfaces_as_available branch August 17, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@destogl destogl destogl approved these changes

@christophfroehlich christophfroehlich christophfroehlich left review comments

@bmagyar bmagyar Awaiting requested review from bmagyar

@homalozoa homalozoa Awaiting requested review from homalozoa

@malapatiravi malapatiravi Awaiting requested review from malapatiravi

@mhubii mhubii Awaiting requested review from mhubii

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saikishor @codecov-commenter @destogl @christophfroehlich