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

Redesign nodelet support #42

Closed
bit-pirate opened this issue Dec 19, 2013 · 12 comments · Fixed by #47 or #49
Closed

Redesign nodelet support #42

bit-pirate opened this issue Dec 19, 2013 · 12 comments · Fixed by #47 or #49
Assignees

Comments

@bit-pirate
Copy link
Contributor

As discussed here: #31 (comment)

@bit-pirate
Copy link
Contributor Author

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

@wjwwood
Copy link
Member

wjwwood commented Feb 21, 2014

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

Does that sum up the idea?

Yes.

@wjwwood wjwwood self-assigned this Feb 24, 2014
@wjwwood
Copy link
Member

wjwwood commented Feb 24, 2014

So this is my plan for this:

  • Start a nodelet manager with the capability server
  • provide a roslaunch argument to provider's launch files mapping capability_nodelet_manager_name to the capability server's nodelet manager name
  • the launch files for providers should make use of this roslaunch argument to have their nodelets join that nodelet manager.

Any problems with this?

@wjwwood
Copy link
Member

wjwwood commented Feb 24, 2014

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

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

wjwwood commented Mar 6, 2014

Ok, I opened this pull request to address this: #47

@bit-pirate for review

@bit-pirate
Copy link
Contributor Author

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 wjwwood reopened this Mar 10, 2014
@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2014

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

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

wjwwood commented Mar 10, 2014

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. 😄

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

wjwwood commented Mar 14, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants