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

Duplicate controller name in start and stop controllers list fails silently #506

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Duplicate controller name in start and stop controllers list fails silently #506

merged 1 commit into from
Jan 31, 2023

Conversation

captain-yoshi
Copy link
Contributor

@captain-yoshi captain-yoshi commented Dec 6, 2022 •
edited
Loading

Fixes the case where the user would put the same controller name in the start and stop controllers request:

  switch_ctrl.request.start_controllers = { cartesian_ctrl_name };
  switch_ctrl.request.stop_controllers  = { cartesian_ctrl_name };
  switch_ctrl.request.timeout = 1.0;
  switch_ctrl.request.strictness = switch_ctrl.request.STRICT;

With latest version 7f3a406:

[DEBUG] [1670293480.214267297]: switching service called
[DEBUG] [1670293480.214310895]: switching service locked
[DEBUG] [1670293480.214325558]: switching controllers:
[DEBUG] [1670293480.214344125]:  - starting controller 'my_cartesian_force_controller'
[DEBUG] [1670293480.214358538]:  - stopping controller 'my_cartesian_force_controller'
[DEBUG] [1670293480.214390714]: Found controller 'my_cartesian_force_controller' that needs to be stopped in list of controllers
[DEBUG] [1670293480.214420813]: Stop request vector has size 1
[DEBUG] [1670293480.214453121]: Found controller 'my_cartesian_force_controller' that needs to be started in list of controllers
[DEBUG] [1670293480.214484711]: Start request vector has size 1
[DEBUG] [1670293480.214549437]: Request atomic controller switch from realtime loop
[DEBUG] [1670293480.222428004]: Successfully switched controllers
[DEBUG] [1670293480.222473856]: switching service finished

and with this patch:

[DEBUG] [1670294793.065341693]: switching service called
[DEBUG] [1670294793.065438787]: switching service locked
[DEBUG] [1670294793.065490294]: switching controllers:
[DEBUG] [1670294793.065548651]:  - starting controller 'my_cartesian_force_controller'
[DEBUG] [1670294793.065602438]:  - stopping controller 'my_cartesian_force_controller'
[DEBUG] [1670294793.065659779]: Found controller 'my_cartesian_force_controller' that needs to be stopped in list of controllers
[DEBUG] [1670294793.065718912]: Stop request vector has size 1
[DEBUG] [1670294793.065763460]: Found controller 'my_cartesian_force_controller' that needs to be started in list of controllers
[DEBUG] [1670294793.065807345]: Start request vector has size 1
[ERROR] [1670294793.065873152]: Could no start or stop controller 'my_cartesian_force_controller' because of conflicting switching command
[DEBUG] [1670294793.065921385]: switching service finished

Maybe you accept this behaviour for someone who wants to restart the controller ? If so, does the manager really stops and restart the controller ?

@captain-yoshi captain-yoshi mentioned this pull request Dec 6, 2022
Closed
@bmagyar
Copy link
Member

bmagyar commented Jan 31, 2023

I agree with you.

switch_controllers was never meant to be a tool to restart a single controller so having the same name in both start and stop list would be undefined behaviour in the best case.

bmagyar
bmagyar approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thank you

@bmagyar bmagyar merged commit a37f6be into ros-controls:noetic-devel Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@captain-yoshi @bmagyar