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

[protobuf][wpimath] Move serde functions to separate classes #5918

Merged
merged 18 commits into from
Nov 21, 2023

Conversation

pjreiniger
Copy link
Contributor

Prep work for #5888

The protobuf and struct de/serialization tooling is ripe for auto generation. Moving the utility classes to their own files will make that a whole lot easier.

In addition, this adds units to the field names in the protobuf messages. Like java, the generated protobuf files have no concept of what unit they represent (not even with comments at the moment). I've updated all of the fields that use some unit in C++. Having the unit type in the name can also assist with the generation process.

I did not update the struct schema with unit names. I don't know if there is some magic where that has to map to the protobuf name or if it is just for display purposes

@PeterJohnson
Copy link
Member

If we're going to embed the unit type in the field names, we should do it in both struct and proto for consistency. In both cases it will show up as the field's name in our introspective tools like OutlineViewer and AdvantageScope. I actually considered using the protobuf extensions method to add a custom "units" option, but it didn't play nicely with QuickBuffers (the Java protobuf implementation we're using).

@PeterJohnson
Copy link
Member

PeterJohnson commented Nov 14, 2023

Thinking about this, organizationally I think it would be better to have separate "struct" and "proto" directories/packages, rather than a unified "serde" package. I also don't see a reason for the class names to have Serde appended, they're already in a package and uniquely named with Struct and Proto.

@PeterJohnson
Copy link
Member

For consistency and to reduce potential confusion, let's name the .h files in the proto/ and struct/ directories with Proto and Struct suffixes the same way as the Java files are.

@pjreiniger
Copy link
Contributor Author

/format

@PeterJohnson
Copy link
Member

Please break the units change into a separate PR (we'll merge the change to separate classes first). I think there's probably more discussion/changes to be had re: adding units, such as documenting that the units of X and Y are meters on the Java side.

@pjreiniger pjreiniger changed the title [protobuf][wpimath] Move serde functions to separate classes, add units to protobuf messages [protobuf][wpimath] Move serde functions to separate classes Nov 20, 2023
@PeterJohnson PeterJohnson merged commit 35744a0 into wpilibsuite:main Nov 21, 2023
22 of 24 checks passed
Starlight220 pushed a commit to Starlight220/allwpilib that referenced this pull request Dec 1, 2023
@pjreiniger pjreiniger deleted the add_units_to_protobuf branch October 13, 2024 00:47
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