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.

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

Adding base class for chained controllers: ChainedControllersInterface #663

Merged
merged 5 commits into from
May 17, 2022

Conversation

destogl
Copy link
Member

@destogl destogl commented Mar 7, 2022

This PR introduces new interfaces for chained controllers.
To simplify logic inside controller manager, ControllerInterface is extended with methods needed for chainable controllers, but with default or without implementation.
ChainableControllerInteface then forces user to implement those methods when adding a chainable controller.

The PR depends on #662 to be merged before. (the commit a3424c4 is duplicated for this PR to be able to cherry-pick the rest of the development.)

Copy link
Member

@progtologist progtologist left a comment

Choose a reason for hiding this comment

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

Looks good overall, only suggestion to fix the uninitialized variable.

@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2022

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

Copy link
Contributor

@livanov93 livanov93 left a comment

Choose a reason for hiding this comment

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

@destogl Where do we plan to put tests where two controllers are actually chained and the interfaces are claimed, so we can prove value propagation between interfaces/controllers?
In #664 or #667?

@destogl
Copy link
Member Author

destogl commented Mar 29, 2022

@destogl Where do we plan to put tests where two controllers are actually chained and the interfaces are claimed, so we can prove value propagation between interfaces/controllers? In #664 or #667?

The tests are placed in #667 where management of chained controllers is added into controller manager.

@destogl
Copy link
Member Author

destogl commented Mar 29, 2022

@livanov93 this PR depends on functionality from #662, but I didn't add it here to make review cleaner. So either you can try to merge them when testing or wait until #662 is merged.

Copy link
Contributor

@livanov93 livanov93 left a comment

Choose a reason for hiding this comment

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

Looks good! Great job!

@mergify
Copy link
Contributor

mergify bot commented Apr 6, 2022

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

@destogl destogl force-pushed the chained-controllers-interface-pr branch from 3fd805f to ae6f57e Compare April 6, 2022 10:55
@destogl
Copy link
Member Author

destogl commented Apr 6, 2022

CI needs update so we can ignore its failing.

@destogl destogl force-pushed the chained-controllers-interface-pr branch from ae6f57e to c605a76 Compare April 14, 2022 15:56
@destogl destogl force-pushed the chained-controllers-interface-pr branch from c605a76 to 3e29adc Compare April 25, 2022 19:13
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.

I'd really prefer if we could avoid the chainable API spilling into ControllerInterface.

@livanov93
Copy link
Contributor

I'd really prefer if we could avoid the chainable API spilling into ControllerInterface.

I fully understand your preferences, however, in the current organisation of ros2_control, all the checks regarding claiming interfaces and switching controllers are done at the higher level (e.g. controller manager, resource manager). This is done by calling ControllerInterface methods and makes the idea of not spilling some of the chainable context much harder...

@mergify
Copy link
Contributor

mergify bot commented May 15, 2022

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

Update structure of chainable controller to allow mode chaning only in 'UNCONFIGURED' state.

Switching to chained_mode is only forbidden if controller is active.

Add default implementation for 'on_set_chained_mode' method.

Avoid compile warning about unused variables.

Added check if all reference interface storage is initialized and that interface prefix is equal to controller's name.

Optimize tests and 'usings'.

Use two internal methods instead of 'update' directly on chained controllers.

Reorder constructor.
@destogl destogl force-pushed the chained-controllers-interface-pr branch from 757f1e6 to 862987e Compare May 16, 2022 11:55
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.

Just a few minor fixups otherwise looks good

@destogl destogl merged commit c01427c into ros-controls:master May 17, 2022
@destogl destogl deleted the chained-controllers-interface-pr branch May 17, 2022 15:31
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.

4 participants