-
Notifications
You must be signed in to change notification settings - Fork 611
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
[wpiutil, ntcore] Add structured data support #5391
Conversation
Very high level question (didn't see it in the discord chat) - I do like having protobuf available. However, thinking from the user end, I'm curious if there's possibility to streamline down to one method. What are some of the specific use cases for Protobuf ? Specifically, what cases couldn't be solved well by padding or assuming fixed size? |
Example use cases of protobuf are things like coprocessor data sets (PhotonVision or LimeLight). In 2023, PhotonVision used a custom binary format, and LimeLight used JSON. In at least PhotonVision's case, I know they need to send both global detection data and a list of detections. Padding this out to some maximum number of detections might be possible, but is likely undesireable. Protobuf also allows for forwards and backwards compatibility, eg accommodates data structure evolution as fields are added, which is important in the coprocessor use case in particular (as you might upgrade one side and not the other). The other nice thing with protobuf is the serialization/deserialization code is generated, versus struct is manual (we can provide more utility functions to make it easier e.g. for fixed size strings, but there's no plan to autogenerate that code). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know how many of these things you're already aware of, just want to note these down before they get lost in the sea of changes.
Also, why does the struct array stuff acquire locks to reuse the same buffer instead of creating new ones like the plain struct stuff?
It's a performance tradeoff given that we don't know how large the buffer needs to be. We pay the mutex overhead, but avoid allocating a new buffer every call (calling the allocator has a large and non-deterministic cost). We could alternatively assume some "typical" max number of elements (or a max size in total, e.g. below 256 bytes) so it's done lock-free on the stack for the small size "fast" case, and then hit the mutex/shared buffer/allocation path only in the larger size "slow" case. |
Makes sense, either option sounds fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts about the circular reference checking which were hard to fit to individual snippets of code:
Currently, there's code to check for circular references in both CheckCircular
and CalculateOffsets
, which among other things allows the error format to be different. There's a couple ways to avoid CalculateOffsets
having its own code- Moving the responsibility of checking to its caller (which would make a unified error format easier), or having CalculateOffsets
call CheckCircular
only when necessary, indicated with a parameter (which could still unify the error format if it's created by CheckCircular
).
This may be a moot point, though, since CalculateOffsets
checks if all of its struct fields are valid and circular references never become valid, meaning it effectively already checks for circular references. This means that the stack check is just a fail-safe, so the error format doesn't matter that much, and the other costs of code duplication are somewhat reduced as well. In fact, having different error messages could even be useful since it could indicate to devs that something weird happened- Maybe adding a note about also encountering an internal error (and instructions to report it) would be helpful?
Yes, it's just a fail-safe. I can tweak the message to be something like "internal consistency error" or something like that. |
/format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be constants (probably in the Struct
interface) for the sizes of primitive values?
Would anonymous object declarations (subclassing Struct<T>
/Protobuf<T, MessageType>
) be preferable to declaring AStruct
/AProto
and instantiating it?
It's not possible to create an anonymous static class in Java (see discussion here: https://stackoverflow.com/questions/758570/is-it-possible-to-make-anonymous-inner-classes-in-java-static). Sure, we can add some constants for fixed size type sizes. |
wpiutil/src/main/java/edu/wpi/first/util/struct/DynamicStruct.java
Outdated
Show resolved
Hide resolved
wpiutil/src/main/java/edu/wpi/first/util/struct/DynamicStruct.java
Outdated
Show resolved
Hide resolved
wpiutil/src/main/java/edu/wpi/first/util/struct/DynamicStruct.java
Outdated
Show resolved
Hide resolved
ntcore/src/main/native/include/networktables/NetworkTableInstance.inc
Outdated
Show resolved
Hide resolved
/format |
86934a1
to
6e2844f
Compare
6e2844f
to
0c96bce
Compare
0c96bce
to
9b78a17
Compare
9b78a17
to
5ac0b7b
Compare
This adds support for two serialization formats for complex data types:
Deserialization can be done either by creating a new object (for immutable objects) or overwriting the contents of an existing object (for mutable objects).
Implementing classes should provide inner classes that implement the
Protobuf
orStruct
interface (in Java) or specialize thewpi::Protobuf
orwpi::Struct
struct (in C++). It is possible for classes to implement both. If the class itself does not implement serialization, it's possible for third parties/users to provide an implementation instead.Uses the Google protobuf implementation for C++ and the QuickBuffers alternative protobuf implementation for Java.
TODO:
use hash of schema to ensure unique struct type string when publishing(deferred)SendableBuilder class support(deferred to telemetry redesign)SmartDashboard/Shuffleboard class support(deferred, may do as part of telemetry redesign instead)Fixes #4587.