Proposer: Eitan Marder-Eppstein and Tully Foote
Present at review:
- Josh Faust
- Stu Glaser
Question / concerns / comments
Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.
- Right now pluginlib isn't finding some of my plugins. I need calls like getPackagesWithPluginsByPluginType and getPluginLibrariesInPackage and getPluginsInPackage to figure out why my plugins aren't visible. I want to know what's available to me.
[Eitan] I'm not sure that rospack plugins supports these kind of queries.... I'll have to ask Josh how reasonable they are. Right now, the PluginLoader only knows about plugins for a specific package and base class type which are the arguments that you pass to its constructor.
- [Tully] Are you talking about command line or programmatic?
- I really dislike having to put nearly the same information in the plugin_manifest.cpp and the plugin.xml file.
- [Eitan] Not sure if we can avoid this. The underlying Poco manifest (created in the cpp file under the hood) doesn't store all the information we want to track about a given plugin. Without hacking Poco code... I don't know. I'll discuss with Tully though.
- It would be great to have a program that will help determine what plugins there are in the system (by type, by package).
- [Eitan] Agreed, this might be a feature that we add to the pluginlib package after the first release, but its definitely something to keep in mind
- [Tully] I think this and Stu's first comment are currently being offloaded to "rospack plugin" This is great capability, but we're not trying to do this in the scope of this package.
- What happens when the package containing a plugin hasn't been built? Does pluginlib know enough about what's going on to report this exact error, "Plugin package containing XXX hasn't been built"?
- [Eitan] Right now.. pluginlib will report that it cannot find the library for the plugin. We could add additional information suggesting that this is probably because the package was not built.
- For createPluginInstance, the bool parameter should be named "auto_load_plugin_library"
- [Eitan] Fair enough.
There seems to be an overloading of the word "plugin". To me the plugin is the xml file/shared object you're loading, whereas then you're registering classes that can be created (not plugins). This caused me to be confused quite often when trying to figure out what the PluginLoader class functions actually did. (loadPlugin(), for example -- what does it do?).
- [Eitan] Ok... I see what you're saying here, we think of plugins as the classes you instantiate and libraries as the file/shared objects being loading which may contain one or more plugins. What would you suggest, as far as naming goes, to make this distinction clear?
- Why are loadPluginLibrary()/unloadPluginLibrary() accessible? What does calling loadPluginLibrary() manually do?
- [Eitan] The above functions operate on the shared object referenced by a given plugin. They could be useful for dealing with shared objects that have more than one plugin in them, but I agree that they're probably unnecessary as you reference a library by the name of the plugin. I'll have to talk to Tully about this one.
In general I'm confused as to the scope of PluginLoader itself -- you initialize it with a package and a plugin type, but then you can call getPluginType(<class name>). You can also load a "plugin" by name -- what does that mean?
- [Eitan] The call getPluginType should not take a name... that's just silly... I agree on removing that.
- What does creating a plugin by name mean? Shouldn't you be creating it by class, since those are guaranteed to be unique?
- [Eitan] We had some issues with special characters, namely "::" used with namespaces as far as using the class name for a plugin as its identifier. Specifically, the class names do not play nice with advertise and subscribe calls over ROS. This caused us to allow users to name their plugins something without "::" in it. Perhaps we need to figure out how to make things work without this?
To be filled out by proposer based on comments gathered during API review period
Package status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved