Add gRPC backend support for Offboard plugin

[cswkim]

Also add plugin mock and backend test (which is currently missing - WIP):

backend/test/CMakeLists.txt (to be modified)
backend/test/offboard_service_impl_test.cpp (to be added)

Resolves #702.

[cswkim]

The backend unit test is still in progress, but I've just added a few items to that and would like some feedback before continuing. Disclaimer - I have very limited experience with C++ (and that was MANY years ago) and absolutely no experience using Googletest. I tried piecing together the test file for offboard by looking at the other backend test files, which might have confused me even more hah. I have some general questions, I'm not sure a PR comments thread is the appropriate place for them (probably better suited for the forum), but I'll leave them here for now:

• If a function does not take any arguments, is it required (or common) to have a test for a null request like so?
• Likewise, if a function does not return anything, is it required/common to have a test for a null response like so? This and the above seem like common patterns.
• When dealing with custom type arguments, like for set_video_stream_settings(VideoStreamSettings) in the camera service, there is a test here that involves translating from rpc. I'm pretty clueless as to why this translation is needed. The backend camera service defines the translations here, does every struct for Offboard (AttitudeRate, PositionNEDYaw, VelocityBodyYawspeed, VelocityNEDYaw) need to/from translate functions?
• I'm also unsure about the use of _allocated_ and .release() syntax here. Are these only used with rpc translations?

[JonasVautherin]

If a function does not take any arguments, is it required (or common) to have a test for a null request like so?

My opinion is that if the code needs to handle "invalid" arguments, it has to be tested. There is logic specific to receiving a nullptr, so I write a test for it. So that if somebody ever removes this logic, the test will fail.

Likewise, if a function does not return anything, is it required/common to have a test for a null response like so? This and the above seem like common patterns.

Same as above. If not handling that in the implementation may result a crash, then we want to handle it. And if we want to handle it, we want a test to ensure that we actually do :-).

When dealing with custom type arguments, like for set_video_stream_settings(VideoStreamSettings) in the camera service, there is a test here that involves translating from rpc. I'm pretty clueless as to why this translation is needed. The backend camera service defines the translations here, does every struct for Offboard (AttitudeRate, PositionNEDYaw, VelocityBodyYawspeed, VelocityNEDYaw) need to/from translate functions?

That's a good point. The thing is that we basically convert an enum into another one, using their int value. If the C++ implementation suddenly inverts two elements of the enum, their int value will change and the conversion will be wrong. So there is this test that UNKNOWN stays UNKNOWN and doesn't become, say, DENIED during the conversion.

I'm also unsure about the use of allocated and .release() syntax here. Are these only used with rpc translations?

_allocated_ comes from protobuf: that's related to how they manage the memory. Basic types like booleans are not _allocated_, but structs are. And that's how we deal with them in the autogeneration: we hardcode what is a "primitive" type and what is "allocated" ("11" is a TYPE_MESSAGE and "14" is a TYPE_ENUM).

.release() is probably for the unique_ptr: we release the ownership and pass the pointer to protobuf, which takes a raw pointer.

[JonasVautherin]

That is looking very good to me, good job! I haven't tested the code, though. Is it working?

One thing I believe may be missing in your setup is the offboard.proto file. Did you update the src/backend/proto submodule to a commit containing it (your commit is 71c2e7350015f358f972d9b32a5b24cc500eba74, it would make sense to update the submodule to that)?

And just to make sure: are you aware that once you compile the C++ backend with your changes, you get this build/default/backend/src/backend_bin binary, that you can test directly with the Python wrappers? Given that you generate the Python wrappers with the offboard.proto commit as well ;-).

[cswkim]

Thanks for all the help @JonasVautherin ! I've been so wrapped up in trying to understand the code that I haven't been able to test yet 😆

Yeah I need to update the protos submodule to include the offboard.proto. After I've added the remaining testing code, I'll make sure the backend unit test succeeds and play around with the python wrappers before moving this PR from draft to ready for review.

[julianoes]

I had to stare at the compiler warnings for a bit to figure out what was wrong here 😄. I made some hints in the review what to fix.

Don't forget to compile with this:

make BUILD_BACKEND=1 && build/default/unit_tests_runner && build/default/backend/test/unit_tests_backend

And BUILD_BACKEND=1 is ignored unless you do make clean.

[JonasVautherin]

LGTM, passing unit tests on my side (core + backend). Good for me if it passes the CI!

Thanks a lot @cswkim!

[julianoes]

Thanks!