Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert "added ability to load a plugin using a operation on a task." #134

Conversation

meyerj
Copy link
Member

@meyerj meyerj commented Jan 29, 2016

This PR reverts commits 9011cf9 and 9c9e008 from #90, that add a loadPlugin() operation to the TaskContext interface.

Alternative solutions are:

  • use a remote proxy for the DeploymentComponent (the import() operation is doing exactly the same)
  • add the loadPlugin() operation to one application-specific subclass instead of to the base class TaskContext

It should be noted that there is a difference between loading a plugin (in the process) and loading a service (in a component or in the global service). Plugins can provide a service, but they can also provide typekits, transports or do something completely different. And of top of that there are packages, which are basically collections of component libraries and/or plugins. Some APIs in RTT do not clearly separate these two concepts (e.g. PluginLoader::loadPlugin(name, path_list), which actually loads all plugins provided by a package called name).

So generally loading a plugin is not related to a specific component (you are not passing the this pointer to anyone). What you probably wanted to do is to load a service plugin in the TaskContext based on the name of the plugin or package that provides this service (which may not be a 1:1 mapping if the package provides more than one service). This sounds more like an extension of the existing loadService() operation than a task for a separate operation.

@doudou
Copy link
Contributor

doudou commented Dec 16, 2016

Sounds reasonable to me.

@meyerj meyerj merged commit dac66cb into orocos-toolchain:master Dec 22, 2016
@meyerj meyerj deleted the revert-taskcontext-load-plugin-operation branch December 22, 2016 08:37
@jmachowinski
Copy link
Member

This merge destroyed the rtt-introspection functionality. What is the current alternative to
load a service or whatever, from outside the process if one does not use the OCL Deployer ?

@meyerj
Copy link
Member Author

meyerj commented Feb 17, 2017

Alternative solutions are:

  • use a remote proxy for the DeploymentComponent (the import() operation is doing exactly the same)
  • add the loadPlugin() operation to one application-specific subclass instead of to the base class TaskContext

I am not aware of any other quick and convenient solution, but adding this operation to each TaskContext interface is semantically wrong and might confuse users. On the long term, it would be an option to create a Service interface for the PluginLoader, add it as a global service and find a way to create proxies for global services remotely through CORBA.

An easier, but more hacky solution would be to add the loadPlugin() operation to every TaskContextProxy instance, and extend the IDL to forward the request to the server side and call into the PluginLoader there. In this case the operation would only be visible to remote proxies, but not for local components.

@jmachowinski
Copy link
Member

jmachowinski commented Feb 17, 2017

How would we register a global service ? The only reference I can get from the corba side is the TaskContext.
Or are you suggesting, to add a pluginLoader() to the TaskContext, that provides a set of operations ?

@meyerj
Copy link
Member Author

meyerj commented Feb 17, 2017

How would we register a global service ? The only reference I can get from the corba side is the TaskContext.

That's correct and the reason why the first proposed approach would require a completely independent CORBA IDL, beyond the current TaskContext servers and proxies as an entry point. Because the remote side does not necessarily know which process is providing the named TaskContext and loadPlugin() is a per-process operation.

The second approach is probably more feasible and similar to the operation you added to the TaskContext interface, but I would only expose the access to PluginLoader::loadPlugin() (or all global services) in TaskContextProxy instances and not for every local TaskContext. And it would not break existing code that already uses the loadPlugin(...) operation on the remote side.

@meyerj
Copy link
Member Author

meyerj commented Feb 17, 2017

I created a patch that implements the second solution:
master...meyerj:fix/load-plugin-for-introspection

As promised, it is a bit hacky, but would solve the problem. Feel free to create a new PR if the patch also works for you, but it is more a proof-of-concept than something I would really consider to merge. Probably it would be better to implement a PluginLoaderService, add it to the GlobalService and expose this global service to every TaskContextProxy as a fake service "Global" instead of adding only one fake operation.

@jmachowinski
Copy link
Member

Hi,
sorry for the delayed response, I was ill. And now I'm going to
take a vacation for two weeks. I'll try out the patch after my vacation.

@jmachowinski
Copy link
Member

Hi,
I just tested your patch. It works. As it is a bit intrusive, I was wondering, if we would
get the same effect by making loadPlugin a private / pImpl method on the taskContext and
register it as an operation.

meyerj added a commit to Intermodalics/rtt that referenced this pull request Jan 19, 2018
…trospection2 into rdt-toolchain-2.9

Second Introspection implementation

Only the commits related to introspection of channel elements have been picked. The
modifications to the TaskContext interface have later been reverted in
orocos-toolchain#134.
meyerj added a commit to Intermodalics/rtt that referenced this pull request Jan 26, 2018
…trospection2 into rdt-toolchain-2.9

Second Introspection implementation

Only the commits related to introspection of channel elements have been picked. The
modifications to the TaskContext interface have later been reverted in
orocos-toolchain#134.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants