Refactor dronecore server #266

JonasVautherin · 2018-02-15T10:20:57Z

some cleanup on the proto generation (build_system)
use references and smart pointers instead of old-style pointers
make a library out of dronecore_server (necessary for running from iOS)
rename dronecore_server into backend

A PR actually updating the proto files will come just after this one is merged.

Connects to #238.

julianoes · 2018-02-15T14:18:51Z

core/device.cpp

@@ -384,7 +384,7 @@ void Device::set_disconnected()
        _parent->notify_on_timeout(_target_uuid);

        // Let's reset the flag hope again for the next time we see this target.
-        _target_uuid_initialized = 0;
+        _target_uuid_initialized = false;


julianoes · 2018-02-15T14:21:56Z

grpc/server/src/backend.cpp

+    LogInfo() << "Device discovered [UUID: " << discoveryFuture.get() << "]";
+}
+
+std::future<uint64_t> DroneCoreBackend::wrapped_register_on_discover()


I think we should either go for snake_case or camelCase, or is the mix intentional?

No it's not, sorry. Good catch. dronecore_server was using camelCase, so I continued with that, but dronecore is using snake_case... so maybe it's the occasion to make it more consistent and go back to snake_case. I don't mind, as long as we are consistent (which I wasn't here :D).

Same, I don't care either one as long as it's consistent. The rest of the API is snake_case, so presumably continue with that.

julianoes · 2018-02-15T14:34:42Z

grpc/server/src/backend.cpp

+{
+    LogInfo() << "Waiting to discover device...";
+    auto discoveryFuture = wrapped_register_on_discover();
+    discoveryFuture.wait();


The problem here is that std::promise in C++11 does not support triggering the callback multiple times. Therefore, this will crash when the vehicle is discovered a second time.

That's one of those cases where I wish we had streams :D.

I thought it was not called the second time, as the device stays the same. I'll see what I can do here. But it sounds like a mutex and it's more complicated than what I would like to have :).

boost::fibers would support something like that as well.

But since we don't depend on boost yet, I'm happy to try to avoid it unless necessary :D. I'm not sure I want to build it for Android/iOS...

shakthi-prashanth-m · 2018-02-16T05:27:04Z

grpc/server/src/CMakeLists.txt

@@ -70,12 +36,16 @@ target_include_directories(
    ${PLUGINS_DIR}
 )

-target_link_libraries(
-    dronecore_server
+add_executable(backend_bin


How about renaming it as backend_server ?

But then should the library be called libdronecore_backend_server? I assumed that "backend" was enough, as a "grpc backend" is necessarily a server somehow :D.

What do you think?

If "backend" is the name used in practice, I am fine with it.

shakthi-prashanth-m · 2018-02-16T05:29:56Z

grpc/server/src/backend.cpp

+
+namespace dronecore::backend {
+
+int DroneCoreBackend::run(const int mavlink_listen_port)


const doesn't serve purpose here, right ? Its call by value.

Doesn't hurt either :-). It says that mavlink_listen_port is not modified in run. Not much, but it's also about the principle: I try to make const everything I can :-).

shakthi-prashanth-m · 2018-02-16T05:40:05Z

grpc/server/src/backend.cpp

+    }
+
+    log_uuid_on_timeout();
+    wait_for_discovery();


return value omitted.

Good catch :-).

Ok! I said that you have omitted returning an int value from this function.

shakthi-prashanth-m · 2018-02-16T05:40:41Z

grpc/server/src/backend.cpp

+    return 0;
+}
+
+int DroneCoreBackend::connect_to_vehicle(const int port)


Why not boolean as return value ?

Yeah, you're right.

shakthi-prashanth-m · 2018-02-16T05:45:38Z

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;


If its const, then how does action.arm() can work ? IMO, const is used when you do read-only. Operations that change the state (such as arm, takeoff, etc) can't be const. Please correct me if I am wrong.

The Action object doesn't have state. It just passes the message further. By having it const, it makes it clear. Mission has some internal state and, therefore, isn't const.

shakthi-prashanth-m · 2018-02-16T05:50:55Z

grpc/server/src/plugins/mission/missionrpc_impl.h

-        mission = std::make_shared<Mission>(&dc->device());
-    }
+    MissionServiceImpl(Mission &mission)
+        : mission(mission) {}

    Status SendMission(ServerContext *context, const rpc::mission::SendMissionRequest *request,


Can we name it as UploadMission() (or just Upload() as its mission upload indeed, because we call as mission.upload(...)) ? Makes sense ?

I don't have a strong opinion on it, but note that this implies changing the proto files, and very soon we won't be able to change the proto files anymore as it will break the API for all the modules.

This said, I had put SendMission instead of UploadMission because I remember having had weird discussion with users who were assuming that "upload" is over the internet, and hence you "upload the mission to some website" :D. But maybe that's not relevant here.

@julianoes do you have an opinion on the naming?

Hm, when it is SendMission what's the other direction? ReceiveMission, RequestMission?

Actually, I am completely wrong. It is already UploadMission. It's just not in the scope of this PR :-). Sorry about that :-).

…r QGC

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

julianoes

Feel free to merge when that's done.

JonasVautherin · 2018-02-22T20:19:53Z

Yes sorry, I got stuck on the iOS build. I'm working on this right now.

JonasVautherin · 2018-02-27T11:14:36Z

Coming back with some unit tests soon.

JonasVautherin requested review from avinash-palleti, julianoes and shakthi-prashanth-m February 15, 2018 10:20

JonasVautherin added the in progress label Feb 15, 2018

JonasVautherin self-assigned this Feb 15, 2018

julianoes requested changes Feb 15, 2018

View reviewed changes

JonasVautherin force-pushed the 238-refactor-dronecore-server branch from 5e17e66 to 721a9a8 Compare February 15, 2018 16:30

shakthi-prashanth-m reviewed Feb 16, 2018

View reviewed changes

JonasVautherin force-pushed the 238-refactor-dronecore-server branch from 721a9a8 to 252f49e Compare February 16, 2018 09:37

JonasVautherin added 4 commits February 16, 2018 10:49

dronecore_server: dronecore port is supposed to be 14540. 14550 is fo…

1455d97

…r QGC

dronecore_server: simplify plugins instantiation

67eed9b

dronecore_server: various refactoring

291bc69

dronecore_server: make a library out of the grpc server

bf048bb

* 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 force-pushed the 238-refactor-dronecore-server branch from 252f49e to bf048bb Compare February 16, 2018 09:59

julianoes approved these changes Feb 22, 2018

View reviewed changes

JonasVautherin closed this Feb 27, 2018

JonasVautherin removed the in progress label Feb 27, 2018

JonasVautherin deleted the 238-refactor-dronecore-server branch February 27, 2018 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dronecore server #266

Refactor dronecore server #266

JonasVautherin commented Feb 15, 2018

julianoes Feb 15, 2018

julianoes Feb 15, 2018

JonasVautherin Feb 15, 2018 •

edited

Loading

julianoes Feb 15, 2018

julianoes Feb 15, 2018

JonasVautherin Feb 15, 2018

julianoes Feb 15, 2018

JonasVautherin Feb 15, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

shakthi-prashanth-m Feb 16, 2018

JonasVautherin Feb 16, 2018

julianoes Feb 16, 2018

JonasVautherin Feb 16, 2018

julianoes left a comment

JonasVautherin commented Feb 22, 2018

JonasVautherin commented Feb 27, 2018


		namespace dronecore::backend {

		int DroneCoreBackend::run(const int mavlink_listen_port)

Refactor dronecore server #266

Refactor dronecore server #266

Conversation

JonasVautherin commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVautherin Feb 15, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

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

JonasVautherin commented Feb 22, 2018

JonasVautherin commented Feb 27, 2018

JonasVautherin Feb 15, 2018 •

edited

Loading