238 update action proto #302

JonasVautherin · 2018-03-05T16:15:25Z

Update backend to latest action proto file (the one we agreed on a few weeks ago) and add unit tests for that.

You'll find some comments inlined.

Fixes #238.

* remove 'using namespace' from header * create implementation file for actionrpc_impl * update action proto file and change actionrpc_impl accordingly Fixes #238.

* add unit tests to verify conversions * add MockAction * make ActionServiceImpl use a template for injecting MockAction * reorder Action::Result enum for convenience

JonasVautherin · 2018-03-05T16:19:35Z

cmake/compiler_flags.cmake

@@ -23,7 +23,7 @@ else()
            set(warnings "${warnings} -Wno-missing-field-initializers")
        endif()

-        set(warnings "${warnings} -Wuseless-cast -Wlogical-op")


This fails when we generate files with protoc, and we don't have control on those files.

I removed the option here, because I learnt that we should ideally even give the -Werror as an option to the user, because our code might fail on some compilers because of that. So ideally we would want a cmake option (e.g. CMAKE_WARNINGS_AS_ERRORS).

Using -Wno-useless-cast in the backend only is not a good solution because it forces users of libbackend to inherit it in their code.

I agree. We only want warnings as errors during development.

JonasVautherin · 2018-03-05T16:20:29Z

grpc/server/test/action_service_impl_test.cpp

+#include <gmock/gmock.h>
+#include <string>
+
+// TODO remove this include


I would be in favor of moving ActionResult in its own file, just like I did with ConnectionResult.

I like it if it's namespace inside the module but up to you.

I would put it in dronecore::action::ActionResult, but in another file. Is that what you mean with the namespace inside the module?

JonasVautherin · 2018-03-05T16:21:05Z

plugins/action/mocks/action_mock.h

@@ -0,0 +1,18 @@
+#include <gmock/gmock.h>
+
+// TODO remove this include!


That's the place were it really feels wrong to include action.h, so action_result.h would be much nicer.

Right, I see.

JonasVautherin · 2018-03-05T16:23:00Z

plugins/action/action.h

@@ -46,7 +45,8 @@ class Action : public PluginBase
     * @brief Possible results returned for commanded actions.
     */
    enum class Result {
-        SUCCESS = 0, /**< @brief Success. The action command was accepted by the vehicle. */
+        UNKNOWN, /**< @brief Unspecified error. */


I put UNKNOWN at the top of the list, so that we can use the easy static_cast<> when converting to the gRPC enum. Having UNKNOWN at the end of the list feels wrong because it will need to keep its index (meaning that if we add a new value, UNKNOWN will not be the last one anymore) or we will need more boilerplate for the conversion in ActionServiceImpl, which is not desirable.

Oh, I actually ran into this before. However, it made sense because a new error code added is essentially unknown. So then I would always just shift UNKNOWN down but I guess that's not an option in proto?

No. In proto, UNKNOWN = 0 will stay forever. If you remove UNKNOWN, you cannot use "0" for any other value anymore, forever :-). So we cannot count on an easy conversion from the C++ enum to the proto one, which is why I am very careful with the static_cast<> we currently do (hence the unit tests).

JonasVautherin · 2018-03-05T16:24:44Z

grpc/server/test/action_service_impl_test.cpp

+std::string takeoffAndGetTranslatedResult(dronecore::Action::Result takeoff_result);
+std::string landAndGetTranslatedResult(dronecore::Action::Result land_result);
+
+TEST(ActionServiceImpl, armSuccessIsCorrectlyTranslated)


Many tests here, but if you look closely, you'll see that it checks all the cases for those commands. This ensures that we don't modify ActionResult in action/action.h without making sure the conversion to the gRPC enum is correct, so it will probably avoid weird bugs.

Oh wow, tedious! But I appreciate the tests but I doubt that many tests like this will scale.

Doesn't the comiler complain if you don't cover all enum fields in a switch case? So if you don't add the default case, it would tell you when a new enum was added that is not checked for?

The real problem is that we have to translate Action::Result::SUCCESS into the gRPC enum as defined by the proto file (note that in the proto world, an enumeration can have fields "0, 3, 4, 5, 22", unlike c++ that would give "0, 1, 2, 3, 4"). And in this case we do static_cast, and we are happy because luckily, UNKNOWN is "0" on both sides.

Now say you swap two of them in action/action.h, I'm quite sure that might pass a review and get into the code. And then, some customers might receive the messages inverted. And since some messages are pretty rare, good luck for debugging :-). I'd rather have a few more unit tests like this.

Maybe there is a less verbose way to write them in C++, or maybe that might be a situation for using macros, I don't know. But I believe those tests are valuable, and still much easier to write as unit tests than doing integration testing.

Oh, and note that those unit tests are really high level, so I would not expect them to change, even if we change the implementation of the ActionServiceImpl functions (that's actually a goal of unit tests, I think: they only depend on the public interface).

So it takes some time to write them, but I don't expect to have to maintain them, like, at all :D.

In the Yuneec SDK Android wrapper I used C macros:

#define CASE(_name) \ case YuneecSt16::Result::_name: \ enumStateField = menv->GetStaticFieldID( \ ResultEnum, #_name, "Lcom/yuneec/sdk/YuneecSt16$Result$ResultID;"); \ break; switch (result) { CASE(SUCCESS) CASE(BUSY) CASE(ALREADY_PAIRED) CASE(NOT_PAIRED) CASE(UNSUPPORTED_PLATFORM) CASE(UNKNOWN) } #undef CASE

Of course, that's JNI specific.

Yeah, that may be a solution, but I must say I am not comfortable reading macros. Another way would be to create a C++ map so that we can for-loop on the different values we have, and also use some lambdas to choose the function (now it is repeated for arm(), land(), takeoff()).

But all of them make the code more complex, and if a test fails, you have to go read those constructions to understand what happened. Whereas the hardcoded ones (if they scale, somehow) are very straightforward to understand when they break.

I would suggest we keep the hardcoded ones and wait to see how it evolves, if that's fine for you.

Fair point, ok sure, let's do that for now.

JonasVautherin added 4 commits March 5, 2018 17:11

backend: update action proto file

65f7f08

* remove 'using namespace' from header * create implementation file for actionrpc_impl * update action proto file and change actionrpc_impl accordingly Fixes #238.

backend: rename actionrpc_impl into action_service_impl

ad658ca

backend: cleanup includes

bc6f604

backend: refactor and add unit tests to action service

d84e409

* add unit tests to verify conversions * add MockAction * make ActionServiceImpl use a template for injecting MockAction * reorder Action::Result enum for convenience

JonasVautherin requested review from avinash-palleti, julianoes, Rjasuja and shakthi-prashanth-m March 5, 2018 16:15

JonasVautherin added the in progress label Mar 5, 2018

JonasVautherin commented Mar 5, 2018

View reviewed changes

JonasVautherin self-assigned this Mar 5, 2018

julianoes approved these changes Mar 5, 2018

View reviewed changes

JonasVautherin merged commit 7242703 into develop Mar 5, 2018

JonasVautherin removed the in progress label Mar 5, 2018

JonasVautherin deleted the 238-update-action-proto branch March 5, 2018 19:50

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 update action proto #302

238 update action proto #302

JonasVautherin commented Mar 5, 2018 •

edited

Loading

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018

JonasVautherin Mar 5, 2018

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018

JonasVautherin Mar 5, 2018

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018

JonasVautherin Mar 5, 2018 •

edited

Loading

JonasVautherin Mar 5, 2018

julianoes Mar 5, 2018 •

edited

Loading

JonasVautherin Mar 5, 2018 •

edited

Loading

julianoes Mar 5, 2018

		@@ -0,0 +1,18 @@
		#include <gmock/gmock.h>

		// TODO remove this include!

238 update action proto #302

238 update action proto #302

Conversation

JonasVautherin commented Mar 5, 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

JonasVautherin Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

JonasVautherin Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVautherin commented Mar 5, 2018 •

edited

Loading

JonasVautherin Mar 5, 2018 •

edited

Loading

julianoes Mar 5, 2018 •

edited

Loading

JonasVautherin Mar 5, 2018 •

edited

Loading