Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose bindings #31

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Conversation

danieleades
Copy link
Collaborator

this is a sketch of a different repository layout, which may be a partial solution to the discussion in #23 (comment)

there's no additional overhead (in either complexity or compile time) to split the repo into two crates.

The first crate mavsdk-bindings directly exposes the generated gRPC bindings. This satisfies the requirement that generated code is always perfectly aligned with the proto definitions, without any additional effort to maintain.

the second crate is a higher-level wrapper which can

  • expose a more idiomatic API
  • potentially provide greater API stability than the bindings (if that's a goal)

there's not additional effort to publish both, so consumers of the API would be free to choose their guarantees.

or maybe the raw bindings turn out to be good enough that the wrapper doesn't add any value...

if you want to be really clever, you could also stick the separate plugins behind cfg flags, so that you only need to pay for what you need (although you'll still get the full prost package, which i assume is the bulk of the binary size)

@danieleades danieleades mentioned this pull request Oct 26, 2021
@danieleades danieleades marked this pull request as ready for review October 28, 2021 12:54
@danieleades
Copy link
Collaborator Author

oh, worth mentioning i dropped the lib prefix from the library name in this PR. Not sure how controversial that is...
I think that's a C/C++ convention.

@danieleades danieleades force-pushed the expose-bindings branch 2 times, most recently from 069a9f4 to 709af28 Compare October 28, 2021 13:29
@danieleades danieleades marked this pull request as draft October 28, 2021 13:36
@danieleades danieleades marked this pull request as ready for review October 28, 2021 15:18
@julianoes
Copy link
Collaborator

Can you show me an example what the raw/direct API looks like and what the nicer/generated one would be like? That would help me understand what you mean.

@danieleades
Copy link
Collaborator Author

Can you show me an example what the raw/direct API looks like and what the nicer/generated one would be like? That would help me understand what you mean.

absolutely. easiest way is navigate to the mavsdk-bindings directory and run cargo doc --no-deps --open. that's the raw, tonic-generated bindings.

the api exposed by the main crate is unchanged from the existing implementation. (cargo doc --no-deps --open from the root of the repo will show you that one)

@danieleades
Copy link
Collaborator Author

the original crate has two layers

  1. the gRPC bindings generated by tonic
  2. the handwritten wrappers around those bindings, which are exposed as the public API

in this PR i've split those two layers into separate crates. So i guess the question now is

  • are the tonic-generated bindings 'good enough' to expose directly? if that's the case, you can just expose those, remove the 'wrappers' crate, and call it a day
  • if they're not good enough, then you want the second crate to wrap the generated code. but exposing the raw bindings as a separate crate is a good stop-gap for anyone wanting to use features in the latest proto definitions, but which is not yet implemented in the wrapper

there are other reasons for using a wrapping crate though

  • you can make the wrapping crate more stable than the generated code, because you won't necessarily need to expose all breaking changes to the user
  • you can convert some things into more idiomatic types. for example in the info module, there's an object with semantic versions in signed integers. I have no idea what it means to have a negative semantic version, so you could convert that into an unsigned integer. or even use the semver::Version struct.

obviously the price you pay is greater maintenance burden

@danieleades
Copy link
Collaborator Author

i was looking at publishing this. looks like people are camping on every possible permutation of "mavlink" and "sdk" on crates.io already...

@julianoes
Copy link
Collaborator

i was looking at publishing this. looks like people are camping on every possible permutation of "mavlink" and "sdk" on crates.io already...

mavsdk_rust taken?

@JonasVautherin
Copy link
Collaborator

Haven't looked very closely, but right now protoc-gen-mavsdk is not used at all, right (that would generate the higher-level interface using the templates)? So the "raw bindings" are the direct gRPC interfaces, and on top of that it's all written manually. Am I right?

Just wondering because for me, ideally we would do the same as in the other language bindings (mavsdk-java, mavsdk-python, mavsdk-swift), i.e. generate most of the code from the templates. And there gRPC is an implementation detail. Actually, for this repo (mavsdk-rust), I would even think it may be worth looking into bypassing gRPC entirely and generating code that calls C++ directly.

Does that make sense?

@danieleades
Copy link
Collaborator Author

Haven't looked very closely, but right now protoc-gen-mavsdk is not used at all, right (that would generate the higher-level interface using the templates)? So the "raw bindings" are the direct gRPC interfaces, and on top of that it's all written manually. Am I right?

yes, protoc-gen-mavsdk is not in the loop at all here. that's the case right now, not something that i'm introducing with this pull request. but the 'manual' code could be changed to use templates for generation, absolutely. I'd be interested to know what you would gain in this case. Is the template-generated code somehow nicer, or more idiomatic than that generated by tonic? I would have to see examples I guess.

Just wondering because for me, ideally we would do the same as in the other language bindings (mavsdk-java, mavsdk-python, mavsdk-swift), i.e. generate most of the code from the templates. And there gRPC is an implementation detail. Actually, for this repo (mavsdk-rust), I would even think it may be worth looking into bypassing gRPC entirely and generating code that calls C++ directly.

Does that make sense?

that definitely makes sense. in that case you would use an FFI rather than gRPC, right? with a library such as cxx. You'd use templates to generate the glue code, and the Rust types. Not something I have any experience with, but certainly you'd have some big savings in binary size and likely performance by doing that.

@danieleades
Copy link
Collaborator Author

i'm going to mark this as a draft, as I don't necessarily expect it should be merged. It's been useful as a discussion!

@danieleades danieleades marked this pull request as draft October 29, 2021 05:34
@JonasVautherin
Copy link
Collaborator

I mean on my end it can be merged like this 👍. @julianoes what do you think?

I just wanted to mention the templates discussion. I don't know Rust, so I cannot write the code, but I'm happy to help with the templates. Question would be whether we want to continue with gRPC or use FFI and call C++ directly (I would vote for the latter), and then you would need to write a few representative functions before starting looking at the templates, I would say.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants