238 refactor dronecore server #295

JonasVautherin · 2018-02-28T16:24:38Z

Refactor dronecore_server in order to have it build as a library (for iOS) and to have the connection unit tested (saved me from a few bugs like having the discover callback called multiple times or me calling register_on_discover multiple times.

Connects to #238.

JonasVautherin · 2018-02-28T16:48:38Z

grpc/server/test/connection_initiator_test.cpp

+    MockDroneCore dc;
+    event_callback_t discover_callback;
+    EXPECT_CALL(dc, register_on_discover(_))
+    .WillOnce(SaveCallback(&discover_callback));


This should be indented, but I can't seem to find how to do this with astyle -_-.

Yer that's weird about astyle. I guess at some point we should try to out the clang tooling to enforce style.

julianoes

Looks clean!

julianoes · 2018-03-01T16:00:13Z

grpc/server/src/backend.h

+
+    dronecore::DroneCore _dc;
+    dronecore::backend::ConnectionInitiator<dronecore::DroneCore> _connection_initiator;
+    std::unique_ptr<grpc::Server> server;


Should it be _server?

julianoes · 2018-03-01T16:00:29Z

grpc/server/src/backend.h

+    std::unique_ptr<grpc::Server> server;
+};
+
+} // namespace backend


I like the namespacing.

julianoes · 2018-03-01T16:03:58Z

grpc/server/test/connection_initiator_test.cpp

+    MockDroneCore dc;
+    event_callback_t discover_callback;
+    EXPECT_CALL(dc, register_on_discover(_))
+    .WillOnce(SaveCallback(&discover_callback));


Yer that's weird about astyle. I guess at some point we should try to out the clang tooling to enforce style.

julianoes · 2018-03-01T16:04:30Z

grpc/server/test/connection_initiator_test.cpp

+        initiator.wait();
+    });
+
+    EXPECT_TRUE(async_future.wait_for(std::chrono::milliseconds(100)) == std::future_status::timeout) <<


I usually use fake time for unit tests. Have you seen that?

Oh, I've seen a FAKE_TIME, but immediately forgot about it. I'll have a look!

I am not sure how I should use it here, actually. In call_every_handler_test.cpp, I see the benefit of the fake time because the object under test is using the fake time as well. But here, I don't know how I can make my initiator.wait() use the fake time?

I am a bit confused, maybe that's because I am not used to FakeTime ^^. How would you do it?

julianoes · 2018-03-01T16:06:21Z

grpc/server/src/connection_initiator.h

+        LogInfo() << "Waiting to discover device...";
+
+        dc.register_on_discover([this](uint64_t uuid) {
+            std::call_once(_discovery_flag, [this, uuid]() {


Does this mean that it is only called once? So it is not called anymore after a timeout and then discovery.

I think it doesn't matter here, as I only want to initiate the connection? And the dronecore plugin (or telemetry? I don't remember) is the one sending the state to the client.

And actually as soon as the first connection is established, the dronecore plugin will probably overwrite the register_on_discover callback since, for now, there is only one callback saved at a time.

shakthi-prashanth-m

I added few minor questions.

shakthi-prashanth-m · 2018-03-02T05:15:39Z

grpc/server/src/CMakeLists.txt


-file(STRINGS ${PLUGINS_DIR}/plugins.conf PLUGINS_LIST)
+set(COMPONENTS_LIST core action mission telemetry)


Oh! No plugin list ?

For now, it is hard-coded. We need to deliver a first version soon. So our first version will not really support plugins (i.e. the backend has all our plugins). Later, we'll upgrade for a more dynamic architecture.

Still, the frontend now should look like the plugins are loaded dynamically, so that it is transparent to the user when we update the backend :-).

shakthi-prashanth-m · 2018-03-02T05:19:36Z

grpc/server/src/backend.cpp

+#include <grpc++/server_context.h>
+#include <grpc++/security/server_credentials.h>
+
+#include "action/actionrpc_impl.h"


So ideally, backend shall include all plugin header files as it doesn't know what a user wants. Is that right ?

Ideally, plugins are somehow loaded dynamically. But the first version is hard-coded because it is faster to do xD.

shakthi-prashanth-m · 2018-03-02T05:21:19Z

grpc/server/src/connection_initiator.h

+    ConnectionInitiator() {}
+    ~ConnectionInitiator() {}
+
+    bool start(DroneCore &dc, const int port)


What about connection URL ?

Some further enhancement as well :-). Shouldn't be difficult to add, but not part of the "minimum viable product".

shakthi-prashanth-m · 2018-03-02T05:22:28Z

grpc/server/test/connection_initiator_test.cpp

+    EXPECT_CALL(dc, register_on_discover(_))
+    .Times(1);
+
+    initiator.start(dc, ARBITRARY_PORT);


Can we also add one that initiates via connection URL ?

shakthi-prashanth-m · 2018-03-02T05:23:40Z

grpc/server/src/plugins/action/actionrpc_impl.h

        response->set_result(static_cast<rpc::action::ActionResult::Result>(action_result));
        response->set_result_str(Action::result_str(action_result));
        return Status::OK;
    }

 private:
-    DroneCore *dc;
-    std::shared_ptr<Action> action;
+    const Action &action;


Its good that you switched to references. :)

* Required for some platforms (e.g. iOS) * Rename dronecore_server into libdronecore_backend and backend_bin * Use smart pointers and references instead of pointers

JonasVautherin self-assigned this Feb 28, 2018

JonasVautherin requested review from avinash-palleti, julianoes and shakthi-prashanth-m February 28, 2018 16:24

JonasVautherin added the in progress label Feb 28, 2018

JonasVautherin commented Feb 28, 2018

View reviewed changes

JonasVautherin force-pushed the 238-refactor-dronecore-server branch 2 times, most recently from d693b1d to 24ac4a2 Compare March 1, 2018 10:13

julianoes approved these changes Mar 1, 2018

View reviewed changes

JonasVautherin force-pushed the 238-refactor-dronecore-server branch from 24ac4a2 to 2ac2e5c Compare March 1, 2018 16:18

shakthi-prashanth-m approved these changes Mar 2, 2018

View reviewed changes

JonasVautherin added 4 commits March 2, 2018 10:05

dronecore_server: simplify plugins instantiation

dcccd07

dronecore_server: various refactoring

8f7b9ec

dronecore_server: make a library out of the grpc server

6e5b783

* Required for some platforms (e.g. iOS) * Rename dronecore_server into libdronecore_backend and backend_bin * Use smart pointers and references instead of pointers

backend: add connection_initiator and corresponding tests

8139246

JonasVautherin force-pushed the 238-refactor-dronecore-server branch from 2ac2e5c to 8139246 Compare March 2, 2018 09:05

JonasVautherin merged commit 12cedcc into develop Mar 2, 2018

JonasVautherin removed the in progress label Mar 2, 2018

JonasVautherin deleted the 238-refactor-dronecore-server branch March 2, 2018 09:27

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

238 refactor dronecore server #295

238 refactor dronecore server #295

JonasVautherin commented Feb 28, 2018

JonasVautherin Feb 28, 2018

julianoes Mar 1, 2018

julianoes left a comment

julianoes Mar 1, 2018

julianoes Mar 1, 2018

julianoes Mar 1, 2018

julianoes Mar 1, 2018

JonasVautherin Mar 1, 2018

JonasVautherin Mar 1, 2018

julianoes Mar 1, 2018

JonasVautherin Mar 1, 2018

JonasVautherin Mar 1, 2018

shakthi-prashanth-m left a comment

shakthi-prashanth-m Mar 2, 2018

JonasVautherin Mar 2, 2018

shakthi-prashanth-m Mar 2, 2018

shakthi-prashanth-m Mar 2, 2018

JonasVautherin Mar 2, 2018

shakthi-prashanth-m Mar 2, 2018

JonasVautherin Mar 2, 2018

shakthi-prashanth-m Mar 2, 2018

shakthi-prashanth-m Mar 2, 2018


		file(STRINGS ${PLUGINS_DIR}/plugins.conf PLUGINS_LIST)
		set(COMPONENTS_LIST core action mission telemetry)

238 refactor dronecore server #295

238 refactor dronecore server #295

Conversation

JonasVautherin commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakthi-prashanth-m 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment