Skip to content

Make configure_controller lifecycle transition strict (no multi-hop) - #3196

Open
vedh1234 wants to merge 7 commits into
ros-controls:masterfrom
vedh1234:fix/strict-controller-transitions
Open

Make configure_controller lifecycle transition strict (no multi-hop)#3196
vedh1234 wants to merge 7 commits into
ros-controls:masterfrom
vedh1234:fix/strict-controller-transitions

Conversation

@vedh1234

@vedh1234 vedh1234 commented Apr 7, 2026

Copy link
Copy Markdown

Fix for issue #2916
As discussed in #2916 and #2414, configure_controller() previously performed a multi-hop transition: if a controller was in Inactive state, it would auto-cleanup before configuring.
For now cleanup sequence in unload_controller was kept as-is. Altering this would strictly break backward compatibility for service clients (e.g., spawner, orchestrator nodes).

Changes:

  • configure_controller(): now only accepts Unconfigured state; any other state returns ERROR
  • Removed the implicit auto-cleanup of Inactive controllers
  • Updated tests to reflect the strict behaviour

Comment thread controller_manager/test/test_load_controller.cpp Outdated
Comment thread controller_manager/src/controller_manager.cpp
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

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 github-actions Bot added the stale label Jun 5, 2026
@christophfroehlich

Copy link
Copy Markdown
Member

@vedh1234 could you follow-up on the comments please?

@github-actions github-actions Bot removed the stale label Jun 10, 2026
@vedh1234
vedh1234 force-pushed the fix/strict-controller-transitions branch from a580c7f to 0fc660d Compare July 9, 2026 15:27
@vedh1234

vedh1234 commented Jul 9, 2026

Copy link
Copy Markdown
Author

Sorry for the delay.
I have updated the required changes also while looking into code found that two tests (inactive_controller_cannot_be_cleaned_up and inactive_controller_cannot_be_configured) were verifying the same thing so I have merged them into single configure_controller_requires_unconfigured_state test. I have rebased it as well.

@saikishor

Copy link
Copy Markdown
Member

@vedh1234 Can you recheck the tests are failing

@vedh1234

Copy link
Copy Markdown
Author

Hi @saikishor!
I have pushed a fix for TestControllerManagerSrvs.configure_controller_srv, which wasn't updated when configure_controller became strict. Please approve the workflow run.
Thanks!

@christophfroehlich christophfroehlich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes LGTM, but the tests are failing

@vedh1234

vedh1234 commented Aug 1, 2026

Copy link
Copy Markdown
Author

I'm looking into it, on my local run, all the tests are passing (except the hardware related) but for some reason here they are failing. I'll check the CI logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants