Skip to content

Examples with API for static remapping#200

Closed
sloretz wants to merge 3 commits intomasterfrom
impl_static_remapping
Closed

Examples with API for static remapping#200
sloretz wants to merge 3 commits intomasterfrom
impl_static_remapping

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Jan 19, 2018

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.

@sloretz sloretz added the in review Waiting for review (Kanban column) label Jan 19, 2018
@sloretz sloretz self-assigned this Jan 19, 2018
@sloretz sloretz changed the title Impl static remapping Examples with API for static remapping Jan 19, 2018
@Karsten1987
Copy link
Copy Markdown
Contributor

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 ::init call?
If so, I'd vote for the init function. My thinking here is that it potentially requires less code adjustments for the user, meaning they can just inherit from Node and that's it.
Further, when thinking about launching multiple nodes as components, we have to make sure that each Node within its component takes care of handling these arguments. Whereas, the user is most likely always in charge of the main function and thus can easily control the rules in the init function.
Does that make sense?

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jan 19, 2018

@Karsten1987 yes you understand correctly. I agree about ::init. It is required if it is the only place command line arguments are passed in. The rules via a constructor tries to match the NodeHandle specific rules feature in roscpp, but this isn't necessary in ros 2 since rules can have node name prefix (assuming node names are unique in a process).

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jan 19, 2018

@Karsten1987 Remappings on the Node constructor may be useful depending how composition via a roslaunch equivalent works. If it uses dynamic composition then by the time a new node is composed rclcpp::init has already been called, and remappings specific to the node need to be passed in another way.

{
public:
MinimalRemappingSubscriber(const rclcpp::ParsedArgs & node_args)
: Node("minimal_remapping_subscriber", node_args)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

http://wiki.ros.org/roscpp/Overview/NodeHandles#Namespaces

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Mar 12, 2018
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 2, 2018

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.

@sloretz sloretz closed this May 2, 2018
@sloretz sloretz deleted the impl_static_remapping branch May 2, 2018 23:16
@sloretz sloretz removed the in progress Actively being worked on (Kanban column) label May 2, 2018
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.

3 participants