osrf / capabilities

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

Redesign nodelet support #42

Closed bit-pirate closed 10 years ago

bit-pirate commented 10 years ago

As discussed here: https://github.com/osrf/capabilities/pull/31#issuecomment-30382297

bit-pirate commented 10 years ago

@wjwwood has there been any progress on this? It's currently the major showstopper for a proper implementations on the TurtleBot.

wjwwood commented 10 years ago

Ok, so what did we decided we wanted to do?

Have the capability server provide a nodelet_manager and provide that nodelet manager's name somehow. Then all providers which use nodelet's share that one central nodelet manager.

Does that sum up the idea?

bit-pirate commented 10 years ago

Does that sum up the idea?

Yes.

wjwwood commented 10 years ago

So this is my plan for this:

Any problems with this?

wjwwood commented 10 years ago

I would say that adding a dependency on nodelet is undesirable, but since rospy currently depends on roscpp I'm not sure that matters...

bit-pirate commented 10 years ago

Any problems with this?

No. Sounds good to me.

I would say that adding a dependency on nodelet is undesirable

True. However, can't think of an easy way to split this up.

wjwwood commented 10 years ago

Ok, I opened this pull request to address this: https://github.com/osrf/capabilities/pull/47

@bit-pirate for review

bit-pirate commented 10 years ago

Any problems with this?

No. Sounds good to me.

Sorry for the delay. I finally reviewed this PR and realised that I was to quick with the statement above.

I'd like request two additions, which I don't see covered by the above PR

  1. I'd like to avoid using roslaunch params for letting capability providers know where the nodelet manager is. Otherwise we will end up with hard-coded names for the nodelet manager name in each provider launcher. This is currently the case with the turtlebot apps and it's annoying and hard to maintain.
  2. How can apps retrieve the information about the capability server's nodelet manager? I can't find any kind of services or similar tool to get this information.

In order to solve the above two issues, I suggest we implement a process in which a capability can let the server know it is a nodelet and wants to be loaded into the server's nodelet manager.

In a similar manner I need be able to retrieve the fully resolved name of the server's nodelet manager, so that apps can load nodelets into the same manager.

Please re-open this issue until we have cleared out the above concerns.

wjwwood commented 10 years ago

I'd like to avoid using roslaunch params for letting capability providers know where the nodelet manager is. Otherwise we will end up with hard-coded names for the nodelet manager name in each provider launcher. This is currently the case with the turtlebot apps and it's annoying and hard to maintain.

I envisioned that everyone would use the parameter to avoid hard coding the name of the nodelet manager. I don't understand what you mean here, can you expand on why this leads to hardcoding and what you want/expect end users to do instead?

How can apps retrieve the information about the capability server's nodelet manager? I can't find any kind of services or similar tool to get this information.

There isn't one, but I can add one.

In order to solve the above two issues, I suggest we implement a process in which a capability can let the server know it is a nodelet and wants to be loaded into the server's nodelet manager.

What does the capability_server do with this information?

In a similar manner I need be able to retrieve the fully resolved name of the server's nodelet manager, so that apps can load nodelets into the same manager.

That information is passed as an argument to the launch file of capability providers, and once I add a service call, there will be a way to externally get the nodelet_manager node name. Is that not sufficient?

bit-pirate commented 10 years ago

I envisioned that everyone would use the parameter to avoid hard coding the name of the nodelet manager. I don't understand what you mean here, can you expand on why this leads to hardcoding and what you want/expect end users to do instead?

I think I have misunderstood your code - your parameter remapping of capability_server_nodelet_manager_name will do the trick.

That information is passed as an argument to the launch file of capability providers, and once I add a service call, there will be a way to externally get the nodelet_manager node name. Is that not sufficient?

No, that would suffice. I'll run a test with both capabilities and apps, once that service is implemented and then provide feedback.

wjwwood commented 10 years ago

I think I have misunderstood your code - your parameter remapping of capability_server_nodelet_manager_name will do the trick.

Excellent, I don't fault you for not getting the gist right away, there isn't exactly any documentation for this yet. :smile:

No, that would suffice. I'll run a test with both capabilities and apps, once that service is implemented and then provide feedback.

I'll try to get that implemented today.

wjwwood commented 10 years ago

@bit-pirate Ok, I got the service call implemented. Please give the master branch a try and see if it works for you.