Skip to content
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

Support for nodelet managers #31

Merged
merged 1 commit into from
Nov 10, 2013
Merged

Support for nodelet managers #31

merged 1 commit into from
Nov 10, 2013

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Nov 9, 2013

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
Copy link
Member

wjwwood commented Oct 19, 2013

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
Copy link
Contributor Author

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
Copy link
Member

wjwwood commented Nov 5, 2013

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:

  • If my app uses nodelets
    • ask each of the providers for each of the capabilities I require if they have a nodelet manager
    • If none, start my own
    • If one, use it
    • If more, dunno you decide?

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

@wjwwood
Copy link
Member

wjwwood commented Nov 5, 2013

Thoughts?

@bit-pirate
Copy link
Contributor Author

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
Copy link
Member

wjwwood commented Nov 5, 2013

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
Copy link
Contributor Author

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 turtlebot/turtlebot_apps#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
Copy link
Contributor Author

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
Copy link
Member

wjwwood commented Nov 9, 2013

Upgraded to a pull request.

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

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1b26d10 on issue_31 into da4d263 on master.

This attribute can be used as an implementation
hint to other capabilities and apps
@wjwwood
Copy link
Member

wjwwood commented Nov 9, 2013

Ok, I rebased it since #36 is merged.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ddd971b on issue_31 into 4764d73 on master.

wjwwood added a commit that referenced this pull request Nov 10, 2013
Support for nodelet managers
@wjwwood wjwwood merged commit 07e0c49 into master Nov 10, 2013
@wjwwood wjwwood deleted the issue_31 branch November 10, 2013 07:57
@bit-pirate
Copy link
Contributor Author

Regarding the implementation of the recent discussion about nodelet support over at robotics-in-concert/rocon_app_platform#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
Copy link
Contributor Author

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

@wjwwood
Copy link
Member

wjwwood commented Dec 19, 2013

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

@bit-pirate
Copy link
Contributor Author

Done.

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