Initial MoCap service

[irsdkv]

This is a draft to MoCap plugin API implementation.
I allowed myself to move slightly away from the usual API. May be you will find it useful.
Please let me know is that interface ok.

Links:
External Positioning
VISION_POSITION_ESTIMATE
MAVSDK PR

[irsdkv]

I'm founded that MAVSDK-Python protoc generator don't agree with my API :)

[JonasVautherin]

I'm founded that MAVSDK-Python protoc generator don't agree with my API :)

Oh, because you send a stream. Yep, that's something new 😅. But fortunately the last of 4 types of RPC supported by gRPC 🙂.

[irsdkv]

I'm founded that MAVSDK-Python protoc generator don't agree with my API :)

Oh, because you send a stream. Yep, that's something new . But fortunately the last of 4 types of RPC supported by gRPC .

Yes, API change resolved this problem. I was not tested yet, but generation works ok :)

[julianoes]

I would like to see this interface be limited to a tested and supported easy to use subset instead of just exposing/duplicating everything that MAVLink has.

Not that this is part of the reason why MAVSDK exists in the first place. It's because the MAVLink API is huge and it's unclear what is supported, what should be used and how and by having a small simple stupid API it becomes much more usable.

[irsdkv]

Hm.
I found that Python generator doesn't work fine outside naming like SetWhatever(SetWhateverRequest) returns([stream] SetWhateverResponse).
It is not ok for testing and using.
At this time I tested single request-response scheme and it work ok in around of 30Hz on 433MHz radio.
Now I think that fastest and clearest way to implement this module will be using this "old" way (single request-response).

[irsdkv]

Small test result:
This implementation (with this commit in Backend) has reached more than 250Hz frequency of sending VisualPositionEstimate using MAVSDK-Python and ESP8266-12e link.

[julianoes]

Oh nice!

[JonasVautherin]

I found that Python generator doesn't work fine outside naming like SetWhatever(SetWhateverRequest) returns([stream] SetWhateverResponse).

Yes, because we would need to extend the generator to support client streams.

@julianoes: do you think we should go for this implementation right now, and possibly move to a client-side stream later if that gets contributed?

@irsdkv: how would you represent a client stream in python? A server stream becomes a generator, but I'm not sure what a client stream should be 😊

[irsdkv]

@julianoes: do you think we should go for this implementation right now, and possibly move to a client-side stream later if that gets contributed?

One more day for me please :)
Now I'm trying to implement Telemetry Odometry stream. I would try to reach more compatibility for Odometry types in Mocap and Telemetry (may be in future they will go outside plugins in common types header).

@irsdkv: how would you represent a client stream in python? A server stream becomes a generator, but I'm not sure what a client stream should be

I tried to hardcode it, but it is too complicated for me now. So I have no idea how client stream should be implemented :)

[JonasVautherin]

So I have no idea how client stream should be implemented :)

I did not mean how the protoc plugin should be implemented, but the actual python code. Say SetVisionPositionEstimate is a client stream, that could be something like:

position_estimate_subject = drone.mocap.set_vision_position_estimate()

while True:
    position_estimate_subject.post("the new value I want to send to the server")

position_estimate_subject.close()

For instance. Well, adapted for asyncio, somehow. What do you think?

[irsdkv]

For instance. Well, adapted for asyncio, somehow. What do you think?

Hm. May be I will try it later.

[irsdkv]

position_estimate_subject = drone.mocap.set_vision_position_estimate()

while True:
    position_estimate_subject.post("the new value I want to send to the server")

position_estimate_subject.close()

I definitely try it later (may be next week). But may be make sense go for this implementation at this moment?
So we will have some worked implementation as the reference.

[irsdkv]

So, tests showed that the current implementation works correctly.

In this case need to adjust FMU parameters but all related to plugin works ok.

"Fake" VisualPositionEstimate streaming (x10 time-scale):

Real position OTA VisualPositionEstimate streaming (x10 time-scale):

[JonasVautherin]

I definitely try it later (may be next week). But may be make sense go for this implementation at this moment?

Sure, let's do that.

[JonasVautherin]

Why do we need a STUB for protobuf?

[irsdkv]

In MAVSDK (without STUB):

$ ./generate_from_protos.sh 
-------------------------------
 Generating pb and grpc.pb files
    * protoc --version: libprotoc 3.9.1
-------------------------------

mocap/mocap.proto:64:20: The first enum value must be zero in proto3.

[JonasVautherin]

Doesn't that mean that BODY_FRD should be = 0 instead of = 12? See here: https://developers.google.com/protocol-buffers/docs/proto3#enum.

And then the next ones like BODY_FLU could be 1, 2, etc. Or does it conflict with something else?

[irsdkv]

I think that setting BODY_FRD = 0 may be confusing for gRPC-client users.
For example in MAVSDK-Python:
I manually changed generated code to delete STUB and set real values of enum members.
This will not conflict for any user-defined code which operated with MAVLink-defined constants (for example MOCAP_NED eq 14).
And also, on the other hand, bindings for gRPC plugins to other languages will be produced with users which familiar with MAVSDK and MAVLink and they will can also remove stubs from final API.

[JonasVautherin]

How is BODY_FRD = 0 confusing? I don't get that 😕.

[JonasVautherin]

And also static_cast will not work for casting ever more in plugin_service_impl.h :)

Yes, static_cast is convenient, but it requires a unit test to make sure that it works. If it doesn't, then we need to write more verbose code, which I believe can still be auto-generated. So I believe that I'm fine with losing the ability to static_cast: it would force us to have clearer (but more verbose) code in C++, and it wouldn't add a weird case in the templates.

My fear is this: if we make it difficult to write a template, then it will make it difficult to contribute new languages.

[irsdkv]

Few more info from docs:

For enums, the default value is the first defined enum value, which must be 0.

So 0 is default. I don't know in which cases uses default values. May be this also need to take into consideration.

[JonasVautherin]

That's the default value in case you don't specify it. In many cases we have UNKNOWN = 0. But also, we always specify a value in the code we generate. So I don't think that has any effect for us.

[JonasVautherin]

I would vote for this: we remove STUB, and we start the others at 0 (i.e. we don't care about the MAVLink enum ids). If then we see that this makes it very confusing for python users, we can change the python templates to make it more intuitive.

This way we don't propagate a weird STUB to all the other languages.

What do you think? 😊

[irsdkv]

I think that this make sense. I will do corresponding changes.

[julianoes]

Nice, I think this might be sane to get in and then go from there.