grpc : Dynamic plugin loading #143

Rjasuja · 2017-10-27T09:36:15Z

Split proto files(one for each plugin), load plugins based on plugins.conf file

Rjasuja · 2017-10-27T09:37:30Z

For now conf file is kept simple with plugin name in each new line, other option is to have xml file and have xml parser included.

coveralls · 2017-10-27T09:58:47Z

Coverage remained the same at 48.634% when pulling 7f63bed on grpc into 97d98b9 on develop.

julianoes · 2017-10-27T15:22:22Z

For now conf file is kept simple with plugin name in each new line, other option is to have xml file and have xml parser included.

We will have a xml parser included once the camera definition stuff is added that you could use.

julianoes

Cool, thanks @Rjasuja. I think this is going in the right direction. I left some comments about next steps that I see.

julianoes · 2017-10-27T15:24:09Z

grpc/python_client/CMakeLists.txt

+            --python_out=.
+            --grpc_python_out=.
+            ${proto_dir}/telemetry.proto
+        DEPENDS ${proto_dir}/telemetry.proto


With cmake you can list all files in a directory and loop over them.

julianoes · 2017-10-27T15:25:07Z

grpc/server/CMakeLists.txt

+        --plugin=protoc-gen-grpc=${GRPC_CPP_PLUGIN}
+        --cpp_out=. ${proto_dir}/action.proto
+    DEPENDS ${proto_dir}/action.proto
+)


Also, here I suggest to loop over the files automatically.

julianoes · 2017-10-27T15:26:16Z

grpc/server/dronecore_server.cpp

+
+};
+
+class ActionRPCImpl final : public ActionRPC::Service


I think these classes should be in separate files and then inside the respective plugin folder.

julianoes · 2017-10-27T15:26:46Z

grpc/proto/mission.proto

@@ -0,0 +1,51 @@
+syntax = "proto3";


I think these proto files should be moved to the respective plugin folders.

julianoes · 2017-10-27T15:28:01Z

grpc/server/dronecore_server.cpp

+    map_type map;
+    map["action"] = &createInstances<ActionRPCImpl>;
+    map["mission"] = &createInstances<MissionRPCImpl>;
+    map["telemetry"] = &createInstances<TelemetryRPCImpl>;


This will need to be auto-generated in the same way as the plugins are currently added to device_impl.

Do you agree?

@julianoes createInstance takes class name as parameter, if not here somewhere there should be mapping between string to class name. Any other way we could do without hardcoded mapping.

Thanks for the review, we will address other comments in next submission.

coveralls · 2017-10-30T10:15:52Z

Coverage remained the same at 48.634% when pulling 8b20aa7 on grpc into 97d98b9 on develop.

avinash-palleti · 2017-10-31T15:19:55Z

@julianoes Can you review the changes. If this is fine, we can go ahead for multiple device handling implementation through gRPC.

julianoes

I left a some comments. If you can address them or at least add todos, I think it's fine to merge this and move ahead.

julianoes · 2017-11-03T19:34:16Z

grpc/server/CMakeLists.txt

+    mission.pb.cc # generated
+    mission.grpc.pb.cc # generated
+    telemetry.pb.cc # generated
+    telemetry.grpc.pb.cc # generated


You could also generate that list using the same foreach. You just add it to all to a variable.

julianoes · 2017-11-03T19:34:58Z

grpc/server/dronecore_server.cpp

+    map_type map;
+    map["action"] = &createInstances<ActionRPCImpl>;
+    map["mission"] = &createInstances<MissionRPCImpl>;
+    map["telemetry"] = &createInstances<TelemetryRPCImpl>;


Do you agree?

julianoes · 2017-11-03T19:37:18Z

grpc/server/plugins/action/action.proto

+    rpc Land(ActionEmpty) returns(ActionResult) {}
+}
+
+// FIXME these different Empty types are ugly


I think there is a protobuf empty that we should reuse: import "google/protobuf/empty.proto";.

julianoes · 2017-11-03T19:37:39Z

grpc/server/plugins/telemetry/telemetry.proto

+    rpc TelemetryPositionSubscription(TelemetryEmpty) returns(stream TelemetryPosition) {}
+}
+
+message TelemetryEmpty {}


Also re-use empty here.

mrpollo · 2017-11-06T19:40:17Z

Same comments as @julianoes , thanks for the PR @Rjasuja

coveralls · 2017-11-07T15:05:48Z

Coverage decreased (-31.06%) to 17.571% when pulling 7a3b194 on grpc into 97d98b9 on develop.

julianoes

Only one small comment.

julianoes · 2017-11-07T21:07:30Z

grpc/server/plugins/action/actionrpc_impl.h


 using grpc::Status;
 using grpc::ServerContext;
 using dronecorerpc::ActionRPC;
-using dronecorerpc::ActionEmpty;
+using dronecorerpc::Empty;


Is this line still needed?

Rjasuja · 2017-11-08T02:32:05Z

Removed.

coveralls · 2017-11-08T03:12:01Z

Coverage remained the same at 17.571% when pulling 110df51 on grpc into 762c327 on develop.

Rjasuja · 2017-11-08T09:37:38Z

@julianoes Added cmake file for automated generation of plugin objects. Please review.

coveralls · 2017-11-08T09:58:56Z

Coverage remained the same at 17.571% when pulling 010a7a0 on grpc into 762c327 on develop.

julianoes · 2017-11-08T18:50:08Z

@Rjasuja cool thanks, this looks better! Can you merge or rebase with develop and check the CI issues please?

coveralls · 2017-11-09T06:01:08Z

Coverage remained the same at 17.571% when pulling 3582bc9 on grpc into 670b15f on develop.

Split proto files(one for each plugin), load plugins based on plugins.conf file

Modify Cmake to automatically generate plugins Move respective plugins in specific directories

Create a list of generated source files for all the plugins

Add plugin name in plugins.conf to enable the plugin Mapping table will generate object for the respective plugin and register the service

coveralls · 2017-11-13T08:43:09Z

Coverage remained the same at 17.571% when pulling 876d989 on grpc into 54890a2 on develop.

coveralls · 2017-11-13T09:00:43Z

Coverage remained the same at 17.571% when pulling 761e301 on grpc into 54890a2 on develop.

julianoes · 2017-11-13T14:02:48Z

Thanks @Rjasuja!

julianoes requested changes Oct 27, 2017

View reviewed changes

julianoes requested changes Nov 3, 2017

View reviewed changes

julianoes reviewed Nov 7, 2017

View reviewed changes

Rjasuja force-pushed the grpc branch from 7a3b194 to 110df51 Compare November 8, 2017 02:29

julianoes approved these changes Nov 8, 2017

View reviewed changes

Rjasuja force-pushed the grpc branch from 010a7a0 to 3582bc9 Compare November 9, 2017 05:48

Rjasuja added 3 commits November 13, 2017 13:46

grpc : Dynamic plugin loading

a538337

Split proto files(one for each plugin), load plugins based on plugins.conf file

Improvide Dynamic plugin structure and generation

8d01414

Modify Cmake to automatically generate plugins Move respective plugins in specific directories

Automate generation of dronecore_server executable

8d6d9fb

Create a list of generated source files for all the plugins

Rjasuja force-pushed the grpc branch from 3582bc9 to 876d989 Compare November 13, 2017 08:32

Automate mapping table generation for plugins

761e301

Add plugin name in plugins.conf to enable the plugin Mapping table will generate object for the respective plugin and register the service

Rjasuja force-pushed the grpc branch from 876d989 to 761e301 Compare November 13, 2017 08:47

julianoes merged commit 3f51147 into develop Nov 13, 2017

julianoes deleted the grpc branch November 13, 2017 14:02

julianoes mentioned this pull request Nov 27, 2017

gRPC follow up fixes #179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc : Dynamic plugin loading #143

grpc : Dynamic plugin loading #143

Rjasuja commented Oct 27, 2017

Rjasuja commented Oct 27, 2017

coveralls commented Oct 27, 2017

julianoes commented Oct 27, 2017

julianoes left a comment

julianoes Oct 27, 2017

julianoes Oct 27, 2017

julianoes Oct 27, 2017

julianoes Oct 27, 2017

julianoes Oct 27, 2017

julianoes Nov 3, 2017

avinash-palleti Nov 6, 2017

coveralls commented Oct 30, 2017

avinash-palleti commented Oct 31, 2017

julianoes left a comment

julianoes Nov 3, 2017

julianoes Nov 3, 2017

julianoes Nov 3, 2017

julianoes Nov 3, 2017

mrpollo commented Nov 6, 2017

coveralls commented Nov 7, 2017

julianoes left a comment

julianoes Nov 7, 2017

Rjasuja commented Nov 8, 2017

coveralls commented Nov 8, 2017

Rjasuja commented Nov 8, 2017

coveralls commented Nov 8, 2017

julianoes commented Nov 8, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 13, 2017

coveralls commented Nov 13, 2017

julianoes commented Nov 13, 2017

grpc : Dynamic plugin loading #143

grpc : Dynamic plugin loading #143

Conversation

Rjasuja commented Oct 27, 2017

Rjasuja commented Oct 27, 2017

coveralls commented Oct 27, 2017

julianoes commented Oct 27, 2017

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 30, 2017

avinash-palleti commented Oct 31, 2017

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrpollo commented Nov 6, 2017

coveralls commented Nov 7, 2017

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjasuja commented Nov 8, 2017

coveralls commented Nov 8, 2017

Rjasuja commented Nov 8, 2017

coveralls commented Nov 8, 2017

julianoes commented Nov 8, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 13, 2017

coveralls commented Nov 13, 2017

julianoes commented Nov 13, 2017