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

Consider replacing mavlink-bindgen #287

Open
G1gg1L3s opened this issue Sep 25, 2024 · 6 comments
Open

Consider replacing mavlink-bindgen #287

G1gg1L3s opened this issue Sep 25, 2024 · 6 comments

Comments

@G1gg1L3s
Copy link
Contributor

G1gg1L3s commented Sep 25, 2024

Hi! First, thanks for your work here!

For the past two month, I've working on rewriting rust mavlink code generator because I found some bugs in the current version of mavlink-bindgen. I definitely had the same issues as reported in #285. I also had some messages being generated empty and problems with bitflags.

I tried to write a generator that is compatible with current rust-mavlink code and generates more or less the same principle and code structure as used in mavlink-bindgen because I hoped that it may be merged here someday.

You can find the code here - https://github.com/G1gg1L3s/mavgen

It has a number of features, for example:

  1. The architecture is build more explicitly, similar to how compilers work: we have frontend (xml parser), intermediate representation (which gets checked for consistency, like "this messages refers to a valid enum" or "this enum is redefined", ec) and backend (or code generator). This allows to test the generator easily, and hopefully it will open options for more code generators (like maybe for different languages) or more gradual configuration.

  2. The project has many tests for a number of cases: from xml parsing, to cycles in file imports, to tests for generator methods.

  3. The project even includes integration tests with pymavlink to verify that both version can indeed exchange messages without corruptions.

  4. The generator handles some exotic configurations, like arrays of enums, which we have in some recent mavlink definitions. I don't remember if rust-mavlink handles them, but I suspect it doesn't.

  5. Many things are verified, like if enum fits into a message field, if two messages have the same name, etc.

It also has differences and some incompatibilities if compared with current mavlink-bindgen. For instance:

  1. Generated name items try to follow rust conventions, so structs and enums are converted to PascalCase, where fields and modules are snake_case.

  2. Generator targets bitfield >= 2.0, because older version was producing errors for me, even with rust-mavlink generator.

  3. For now, serde support is omitted, but it should be easy to add. Serde is included, but it generates structure a bit differently than the current version (due to name conflicts).

  4. The code always includes docs and is always formatted with prettyplease. Formatting actually takes a big chunk of generator's execution time.

  5. Enum sizes are generated as minimal as possible (instead of always using #[repr(u32)]).

  6. #[deprecated] attributes are generated for the corresponding items.

  7. Codegen derives Eq on the message if it can.

  8. Field names that are reserved rust words (like type) are automatically escaped with raw idents (r#type).

  9. It's divided into 3 crates: mavgen, mavgen-test and mavgen-cli to avoid bringing unneeded dependencies for CLI for example. The malink-rust uses features to hide cli, but personally I always forget to turn them on when compiling the binary.

And probably more small things I don't recall.

As the goal of the project was to make the codegen work with the current mavlink definitions, I think I achieved it. There are still some polishing and testing to do, for example, testing with V1 messages, adding proper handling of extension fields and serde support. But overall, I'm happy with the result.

So, that's why I'm suggesting merging this tool here. I believe it will be most useful under the mavlink organization for other people to improve and contribute. I understand that it's a big patch of work, so that's why I create this issue so we can discuss if you want to proceed, and if yes, how to plan this path.

Thank you!

@pv42
Copy link
Contributor

pv42 commented Sep 26, 2024

I was concerned that the more complex codegenerator might lead to a lot longer compile times so I ran some (crude) benchmarks to see how the new parser performs.
I was building mavlink with different features enabled. CPU is Intel i5-9400, OS is Ubuntu 2204. Since this compiles all of mavlink there is a obviously a lot in there that is the same in both cases. Realtime and User CPU time are shown bellow.

mavlink-bindgen mavgen
cargo build 52.0s (63.5 cpu) 26.3s (43.3s cpu)
--fearures format-generated-code emit-extension emit-documentation 61.8s (73.3s cpu) (same as above)
--no-default-features --features format-generated-code emit-extensions emit-description std tcp udp direct-serial all-dialects 58.9s (71.9s cpu) 62.7s (83.0s cpu)*

*The last test on the newer generator has the features format-generated-code, emit-extensions and emit-description disabled (since this functionality does not need features).

It seems that the new generator performs generally better then the old one but seems to struggle when more dialects are enabled.
Presumable adding serde support could worsen the performance a bit.

If someone want to run this themselves https://github.com/pv42/rust-mavlink/tree/new_bindgen (this is not pretty).

@patrickelectric
Copy link
Member

Hi @G1gg1L3s, nice work! Is it possible for you to open a PR ? Serde is kind of a requirement for us to merge since some critical applications need it.

@G1gg1L3s
Copy link
Contributor Author

Hey, @patrickelectric, sure! If you are okay with many breaking changes and a potentially huge PR, I can create it rightaway.

As for the serde - I think it can be merged right after the PR. I already added some basic support, which you can find in a separate branch. However, it also contains some breaking changes. For example, enums cannot have serde(tag = "type") becase some messages have the field named type and they conflict with the tag name.

@patrickelectric
Copy link
Member

Can you create an example folder for us to take a look in the API and test with some real vehicles ? Just to be sure that everything is working

@G1gg1L3s
Copy link
Contributor Author

Sorry, could you elaborate? You mean compile some definitions and push them to the repo for you to see the generated code and try it out?

@av-stefan
Copy link

Any updates on this? Would be interesting to get this moving forward to be able to use the newest mavlink specification

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

No branches or pull requests

4 participants