-
Notifications
You must be signed in to change notification settings - Fork 429
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
Implement Unified Node Interface (NodeInterfaces class) #2041
Conversation
Only thing I'm unsure of if if I should leave the NodeT constructor for If it's not explicit we can rely on implicit conversion to allow passing in Node-like objects directly into methods that take in NodeHandle arguments without needing to explicitly instantiate the NodeHandle.. (I think? Maybe the templates won't let that happen, in which case, the point is moot...) |
Will build #11111 be the one??? |
I like the overall idea. It seems like there are multiple methods to retrieve each interface (e.g. Can we implement it in a way such that users can customize it? I.e. If I create a new node interface, I would like to add it there |
It's already implemented in such a way that you can attach a custom interface to the NodeHandle! Just use If you mean creating a new type of node interface, I'm not sure if it's possible (I'm already using quite a lot of template metaprogramming wizardry to achieve what we have at the moment...) And in that case, the nodes themselves need to be updated to support the interface, so at that point I'd just ask for a PR for those new interfaces to be added to the NodeHandle class. The multiple ways for retrieving the interface are just for convenience (or typing less...) I was basing it off the issue over this PR was inspired by. More precisely, the shorter name is for convenience, while the longer name is supposed to use the same names as the node getters for node interfaces. |
1a6b388
to
3ac067b
Compare
3ac067b
to
7e32191
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to talk about the name of the new class NodeHandle
. I think we should pick something else because really it's not a "handle" (we already have a rcl_node_handle_t
which is actually a handle, i.e. an opaque object that is a stand in for another actual thing) and because we had a NodeHandle
in ROS 1 which was something very different and that might cause confusion.
Ideally this whole class would be replaced with a constraint
but we don't have access to that atm with C++17.
Some ideas from me:
NodeWithInterfaces<...>
my_func(NodeWithInterface<...>)
would read as "my_func takes a Node-like thing with the interfaces ..."
NodeWithRequiredInterfaces<...>
NodeInterfaces<...>
- invokes the idea of a collection of interfaces, which may or may not be misleading depending on how we end up implementing this class
How about NodeInterfaceHandle? (Since it's a handler for node interfaces, no matter where they come from). Otherwise, I'm fine with NodeInterfaces also, since this class is just supposed to be an aggregation of interfaces. I'm less inclined towards the |
9be3912
to
6734ecf
Compare
9503d98
to
e6a67eb
Compare
@ros-pull-request-builder retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that something like this may be a good idea.
I have left one suggestion of how to implement it.
I think we have to thoroughly evaluate how this can be used in demos/examples, and double check if using the new "unified node interface" is beneficial.
+1 |
Could I double-check that this +1 is over NodeInterfaceHandle (so +1 to NodeInterfaces even though NodeInterfaceHandle exists?) The multiple inheritance templated example you gave above used NodeInterfaceHandle 😬. I'll change it to whichever you give the +1 to! Also, what do you think about putting getters for this node interface aggregate onto |
03aed20
to
9e675d9
Compare
I've updated the tests and moved everything to node_interfaces! RPr is green! |
9e675d9
to
3ef3c5d
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
0fd3242
to
dce078b
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
dce078b
to
cf31bb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, after checking on the compile-only test
469be6f
to
57335a0
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com> Co-authored-by: methylDragon <methylDragon@gmail.com>
57335a0
to
fa69bf1
Compare
Hey @methylDragon @wjwwood, this PR broke RHEL nightly builds when building rclcpp. Can you please take a look?:
Log output:
I notice:
|
@Crola1702 I'm looking at it! |
Noting that this was fixed in |
Finally a longstanding quibble about Nodes and LifecycleNodes and node interfaces has a potential resolution.
Description
This PR introduces the concept of a
NodeInterfaces
class (that is, a unified adapter for any disparate collection of unique kinds of NodeInterfaces) that allows you to package interfaces into a single object. This is inspired by #831.This allows you to write functions that take in a single object instead of a collection of node interfaces (or instead of a templated function...), and also supports both Nodes and LifecycleNodes, allowing you to quickly swap any place where you originally passed a Node with a NodeInterfaces. This also means that the issue where Nodes and LifecycleNodes exist on separate inheritance trees can be mitigated a little by using this as a bridging class.
What's more, the template parameters allow you to select which interfaces are made available, and also can serve as type hints to downstream users for which interfaces a function requires.
Additionally, you can aggregate interfaces from different nodes (or mocked interfaces) too, which isn't something you could trivially do if you made your function a template function that takes in a NodeT!
This PR also adds a new method for Nodes and LifecycleNodes to obtain their corresponding NodeInterfaces.Tests have been implemented for
Nodes, LifecycleNodes, andthe NodeInterfaces template class itself.Example
This is best explained with an example.
For more info, read the docstring.
Why is this useful?
(Adapted from #831)
Before you would have to...
But now you can:
And in fact, implicit conversions should work too!
And just like that you can support both Nodes and LifecycleNodes!
This also serves as an easy drop-in replacement for any method that takes in an
rclcpp::Node
or similar shared pointer!Why Not...
Why not
create_service(std::shared_ptr<rclcpp::Node> node);
?
Because
Additional Notes
This uses C++14 compatible template metaprogramming constructs, for backportability. I'm pretty sure there are some C++17/C++20 constructs that would make the implementation more concise though.. For a future time.
EDIT: I realized that fold expressions are C++17... We'll have to find some workaround when backporting. (Actually so is is_same_v 🤦♂️ )
Addendum
Because of #2075 , the NodeInterfaces object only accepts non-pointer node-like types. You must dereference any pointer arguments to the NodeInterfaces constructor.