osrf / capabilities

Implements the concept of capabilities as part of the robots-in-concert system.
Other
8 stars 26 forks source link

Support for nodelet managers #31

Closed bit-pirate closed 10 years ago

bit-pirate commented 10 years ago

Kobuki's driver and controllers being implemented using nodelets quickly causes headaches, when starting to use namespaces. For now, we could only solve this by using absolute names for the nodelet's managers. Hence, you can't change the nodelet manager's namespace.

We need to find a way to handle this via capabilities. But how?

A first suggestion, support a nodelet_manager_name parameter in the interfaces.

Better ideas?

wjwwood commented 10 years ago

Maybe we need a set of parameters which are launch file parameters, because in this case ros parameters would not do use any good I think.

We could just add nodelet_manager_name as a one off special case, but it might be prudent to add a mechanism for other "special" parameters later.

bit-pirate commented 10 years ago

Maybe we need a set of parameters which are launch file parameters, because in this case ros parameters would not do use any good I think.

Agreed.

We could just add nodelet_manager_name as a one off special case, but it might be prudent to add a mechanism for other "special" parameters later.

It might be good to add another category to the CapabilityInterface called args and we handle it similar like the parameters. The general usage of args seems more volatile than the ros params. So, I'm not sure how many args will actually be used for capabilities in the end, but if it is not a big extra work, a general mechanism would be nice. On the other hand, I would be fine with just having a nodelet manager name parameter for the time being until we hit more practical "args use cases".

wjwwood commented 10 years ago

I am considering keeping this an implementation detail, such that you can call a service where you can specify a provider and if it defines a nodelet manager it will return it to you. The workflow for you would be:

So in this case the provider spec would define the nodelet manager name, not the interface.

wjwwood commented 10 years ago

Thoughts?

bit-pirate commented 10 years ago

If my app uses nodelets

Ha, I forgot about that! I was actually thinking about our use case with Kobuki, where a couple of capabilities will be implemented with nodelets. I.e. the nodelet of capability A needs to be loaded into capability B's nodelet manager.

Example use cases: Kobuki's safety controller to be loaded into the mobile base's nodelet manager, depthimage_to_laserscan into 3d sensor's nodelet manager, ...

However, in the end there wouldn't actually be a difference between a capability and an application using nodelets and wanting to load its nodelet(s) into the capability's nodelet manager they depend on.

I agree with this being an implementation detail of the capability or application and hence should concern the provider. On the other hand I was so far thinking of the interface being the only information needed in order to use a capability. Retrieving extra information from the provider would violate this design.

While thinking about this for a bit, I couldn't come up with a good use case for a nodelet manager parameter being part of the interface. So please go ahead with implementing your proposal.

wjwwood commented 10 years ago

I.e. the nodelet of capability A needs to be loaded into capability B's nodelet manager.

That should just be handled by the robot developer. The only reason to put it in any of the specs would be to convey that to something not written by the robot developer, like an app.

The thing I was thinking of, was like an app which finds features in an image and then shows them in an android app. So you would want the app code running on the robot to share a nodelet manager with the image producer, so the app server could look at the provider spec for the image producer, see that it uses a nodelet manager with name 'foo' (what I consider to be an implementation hint), and pass a know variable to the app launch file like nodelet_manager_name:=foo so that the app can use that. If the hint is ignored then it's just a little bit less efficient, but still works over topics and stuff.

While thinking about this for a bit, I couldn't come up with a good use case for a nodelet manager parameter being part of the interface. So please go ahead with implementing your proposal.

Thinking about my proposal more, I think we don't even need a service call (though I can have one anyways), because you'll just load the provider's spec and look to see if it has a nodelet manager name specified.

bit-pirate commented 10 years ago

That should just be handled by the robot developer. The only reason to put it in any of the specs would be to convey that to something not written by the robot developer, like an app.

True. On the other hand I want (need) to use the capabilities to allow proper namespacing without the need of absolute paths (see https://github.com/turtlebot/turtlebot_apps/issues/48).

Thinking about my proposal more, I think we don't even need a service call (though I can have one anyways), because you'll just load the provider's spec and look to see if it has a nodelet manager name specified.

That sounds like a good solution to me.

bit-pirate commented 10 years ago

We need to remember to update the docs (http://docs.ros.org/hydro/api/capabilities/html/capabilities.specs.html#module-capabilities.specs.provider) with this nodelet_manager param and probably the remappings parameter discussed in #22.

wjwwood commented 10 years ago

Upgraded to a pull request.

It includes a commit from #36, and so should be merged after it.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 1b26d10968269ec44894c669f2b0d97aa7223525 on issue_31 into da4d263751a86557e64c3a346f745aaac1b95b6a on master.

wjwwood commented 10 years ago

Ok, I rebased it since #36 is merged.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling ddd971b068b7f6900ce560b4e327f787ac726555 on issue_31 into 4764d737b6cb2921ea77d99b4e1af153423a2742 on master.

bit-pirate commented 10 years ago

Regarding the implementation of the recent discussion about nodelet support over at https://github.com/robotics-in-concert/rocon_app_platform/issues/115 :

Using one nodelet manager for alls cap and some app nodelets would require the capability server to support checking for an existing nodelet manager and loading nodelets into it on provider start-up. AFAIK this in turn would require roslaunch node.args replacement and hence a modification of all provider launchers.

Additionally some kind of handle to the nodelet manager, e.g. fully resolved name, needs to be accessible by other nodes, such as the app manager, to support other nodelets being loaded into that manager.

Anything more needing to be considered?

bit-pirate commented 10 years ago

Please re-open this issue, since it is not solved.

wjwwood commented 10 years ago

This issue cannot be reopened as it is a pull request. Can you open another?

bit-pirate commented 10 years ago

Done.