Conversation
|
Do I understand this correctly, that you present two ways of setting up the remapping rules? Either by passing a structure to the Node constructor or setting the rules within the |
|
@Karsten1987 yes you understand correctly. I agree about |
|
@Karsten1987 Remappings on the Node constructor may be useful depending how composition via a |
| { | ||
| public: | ||
| MinimalRemappingSubscriber(const rclcpp::ParsedArgs & node_args) | ||
| : Node("minimal_remapping_subscriber", node_args) |
There was a problem hiding this comment.
@wjwwood Ideas on how to make this work for dynamic composition? I suppose it could require a derived class to have a constructor that took rclcpp::ParsedArgs, but can class_loader call a constructor with arguments?
There was a problem hiding this comment.
Likely we'll require derived nodes to implement a common constructor, which takes a rclcpp::Context at least, and if the remapping arguments are not passed by the context, then something like the rclcpp::ParsedArgs too.
Question, what's in the rclcpp::ParsedArgs? Is it the remapping arguments or the arguments left over after they are removed? Something else?
I'd actually say the two things a node needs to take related to remapping are:
- remapping rules
- possibly left over command line arguments (sans "ROS arguments")
The remapping rules should be divested from any notion of command line arguments, because they may have been communicated via Service call or generated programmatically for testing. Instead, I think there should be a function that takes command line arguments and produces remapping rules.
Maybe that's already what you're aiming for and I'm just confused by the name.
There was a problem hiding this comment.
I would say, after thinking about it more, that component nodes should at least take:
- an
rclcpp::Context - a set of remapping rules
- a set of options for the node
- a set of default values for parameters
The last three could be combined into one structure to limit the number of arguments.
The user would most likely pass all of these, untouched, to the rclcpp::Node constructor along with a default name and namespace.
The reason being, if you had two instances of the same node (so with the same default name) there'd be no good way (that I can think of) to pass different remapping rules to each node via the context (since they'll have the same name and namespace by default). So I think it might be easiest to pass node specific things directly to the node and only place things in the context that might apply to more than one, but I'll leave that up to you to sort out.
There could be a set of remapping rules in the context which are "global", that would apply to all nodes which share the context, and you could have a cascading type pattern where node remappings would override context remappings would override hard coded defaults.
In the options for the node it could include things like whether or not to use intra process, etc. But it could also have an option to ignore the context remappings.
The node might also take other arguments (again either separately or as part of a large structure) like node specific command line arguments or "left over command line arguments" depending on how it was created/executed.
There was a problem hiding this comment.
Oh, to answer you more generic question about class loader. Yes, it can pass arguments when instantiating a class, but taking a note from the ignition counter part (maybe we can use that directly) we might want to actually load a factory class. We can automatically create and register this factory class for people.
I'll be spending more time on this soon. For now, it's fine I think for you to focus on what needs to get to the node and how the node uses it and I'll try to follow up and make it component load-able and stuff.
There was a problem hiding this comment.
Question, what's in the rclcpp::ParsedArgs? Is it the remapping arguments or the arguments left
over after they are removed? Something else?
Something else. The constructor would call on rcl to parse the arguments. It's members would include a list of remap rules (rcl_remap_rule_t ?), default values for parameters (haven't yet checked if this fits with @mikaelarguedas plans), special rules like __name, __ns (ROS1 also has __log, but I'm not sure if that's necessary), and left over non-ROS arguments that the user's code could do something with.
Maybe that's already what you're aiming for and I'm just confused by the name.
I'm starting with an assumption that anything passed to the node needs to also be parse-able via the command line for roslaunch to work with non-composable nodes. The name rclcpp::ParsedArgs is meant to indicate it reuses that machinery for composable nodes.
So I think it might be easiest to pass node specific things directly to the node and only place things
in the context that might apply to more than one, but I'll leave that up to you to sort out.
Agreed. I left the rules out Context because yesterday I heard you say IntraProcessManager is also part of it and should be shared among composed nodes.
There could be a set of remapping rules in the context which are "global", that would apply to all
nodes which share the context, and you could have a cascading type pattern where node
remappings would override context remappings would override hard coded defaults.
Is this like the NodeHandle specific remappings in roscpp where the NodeHandle uses it's own rules first, and then falls back to global rules?
but taking a note from the ignition counter part (maybe we can use that directly)
It would need to be pulled out of ignition-common first. ignition-common has a lot of dependencies that would be undesirable.
There was a problem hiding this comment.
Something else. The constructor would call on rcl to parse the arguments. It's members would include a list of remap rules (rcl_remap_rule_t ?), default values for parameters (haven't yet checked if this fits with @mikaelarguedas plans), special rules like __name, __ns (ROS1 also has __log, but I'm not sure if that's necessary), and left over non-ROS arguments that the user's code could do something with.
That makes sense to me.
Is this like the NodeHandle specific remappings in roscpp where the NodeHandle uses it's own rules first, and then falls back to global rules?
Sort of, but what I described have a different purpose (generic versus node specific remappings) and are fixed in depth (just hard coded -> node specific -> context specific). I think of NodeHandle's from ROS 1 more like subparsers from argparse. Basically it lets you add on to (or completely change) the namespace, so if you want to create a bunch of publishers under the same namespace, you could create a NodeHandle for that namespace and then create the publishers from it to avoid needing to specify the namespace for each publisher explictly. This is also useful when you have a library which creates things affect by namespace, but you want to control what namespace they are created into, you can just pass a NodeHandle in and the library can use that to build off of. You can "chain" NodeHandle's indefinitely, see for examples:
There was a problem hiding this comment.
It would need to be pulled out of ignition-common first. ignition-common has a lot of dependencies that would be undesirable.
Wasn't there a plan to split it into a few pieces? We were also looking at some of the generic C++ utilities in it.
|
This was an early PR that hasn't been updated to match what was implemented. I don't plan on updating this soon, so I'll close it for now. |
This PR proposes an API for static name remapping in python or C++. I expect most users would use command line arguments instead.
In review to signal feedback is welcome. The API is not implemented.