Variadic Controller class + Variadic CompositeController class #387
Conversation
…ks for gcc and clang. Original error is: (#1) robothw_interfaces.h:60:68: error: typedef 'value_type' cannot be referenced with a class specifier.
… into melodic-devel
…ontroller-template-melodic2
| CompositeController& operator =(const CompositeController& c); | ||
| }; | ||
|
|
||
| } // namespace |
There was a problem hiding this comment.
please add the name of the namespace as well
| }; | ||
|
|
||
| } | ||
| } // namespace |
There was a problem hiding this comment.
please add the name of the namespace as well
| @@ -63,7 +63,7 @@ inline std::string enumerateElements(const T& val, | |||
| } | |||
|
|
|||
|
|
|||
| template <typename T> | |||
| template <class T> | |||
There was a problem hiding this comment.
I guess, you changed this for consistency reasons?
There was a problem hiding this comment.
To be honest, I don't remember the logic for this change, but I don't think it matters; I'm happy for us to consistently use typename or class, and can revert this part of the change if the decision is for typename. I think the only time it matters, typename is preferred (eg in ab14ea4).
There was a problem hiding this comment.
and can revert this part of the change if the decision is for typename
The other templates use class in their definitions as well, so it is fine for me.
class seems to be mandatory for nested templates (C++14 and older).
There was a problem hiding this comment.
No strong opinion on this, do it at your leasure.
|
@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. |
IMHO something like 90f89e0 should do for now. |
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:
Controllertaking a single Interface template parameter, it now takes an arbitrary number of them. This should be backward source compatible with previousControlleruse, and because it's all headers, there's no ABI breakage. This also means thatMultiInterfaceControllerno longer needs to exist, and can be deprecated at a future point (for now it's been made a pass-thru toController).CompositeControllertemplate 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.