ros_control icon indicating copy to clipboard operation
ros_control copied to clipboard

Variadic Controller class + Variadic CompositeController class

Open mikepurvis opened this issue 6 years ago • 2 comments

This is a combination of changes previously proposed in #301, #302, and #329. The two additions, both made possible by ROS Kinetic+ being on C++11, are:

  • Instead of Controller taking a single Interface template parameter, it now takes an arbitrary number of them. This should be backward source compatible with previous Controller use, and because it's all headers, there's no ABI breakage. This also means that MultiInterfaceController no longer needs to exist, and can be deprecated at a future point (for now it's been made a pass-thru to Controller).
  • A new CompositeController template has been added, which permits combining controllers, and in particular modifying behaviour when the base controller(s) supply extension points in the form of virtual functions.

Clearpath have been using both of these features for 18+ months and found them useful.

mikepurvis avatar Aug 01 '19 17:08 mikepurvis

@ipa-mdl Thanks for looking. I agree on some kind of test; I think in the past when working on this, I got hung up on what exactly such a test should do. Given that the resource merging is already covered by a unit test, is it enough to just demonstrate something which compiles and maybe passes a trivial smoke test? Or should there be a full ControllerA and ControllerB, and then CompositeControllerAB, with a complete lifecycle and verification of the init/start/stop methods being properly called?

I'm totally down for that approach, I think my concern is just about keeping the test short and simple enough that it can be easily grokked.

mikepurvis avatar Aug 05 '19 12:08 mikepurvis

Given that the resource merging is already covered by a unit test, is it enough to just demonstrate something which compiles and maybe passes a trivial smoke test?

IMHO something like https://github.com/ros-controls/ros_control/commit/90f89e05eb87da69c9a48c31690b4ac954a23fce should do for now.

mathias-luedtke avatar Aug 05 '19 14:08 mathias-luedtke