Mechanical-Advantage / AdvantageScope

Robot telemetry application for FRC
https://docs.advantagescope.org/
MIT License
188 stars 52 forks source link

Support new PhotonVision struct format #175

Open mcm001 opened 2 months ago

mcm001 commented 2 months ago

For 2025,photon is moving to auto-generated struct code (see https://github.com/PhotonVision/photonvision/commit/169595e56ed40e2f8c63e047e04589fd8a3d6ec1 ). This exposes the type string as a NT topic property. If we still want to support photon packed types in AdvantageScope, we should probably drop support for the old thing and move to doing decoding entirely dynamically or something.

https://github.com/PhotonVision/photonvision/blob/c38b50911db28fec4d0a78f96e129ab6a506811e/photon-lib/py/photonlibpy/generated/PhotonPipelineResultSerde.py#L30

As an interface example, here's the format for PhotonPipelineResult in the new IDL. [?] means a VLA, and ? means an optional type.

PhotonPipelineMetadata metadata;PhotonTrackedTarget[?] targets;MultiTargetPNPResult? multiTagResult;

mcm001 commented 2 months ago

We still also need to figure out publishing message definitions for nested types - open to suggestions on how best to do that

mcm001 commented 2 months ago

I've created a first pass at an ICD with a first pass at schema database definitions. Would love feedback on how we can refine this to make dashboard implementations easier.

jwbonner commented 2 months ago

The system for publishing schemas seems reasonable to me, and it shouldn't be a problem since it's very similar to the way structs are already handled. The bigger change will be the decoder (by necessity), since the approach AdvantageScope uses for structs is to "compile" the struct once to a mapping of bits for each field. In this case the schema strings would probably need to be "compiled" to some higher-level definition of the components which are used to decode values.

Is the plan that this format will also support some of the more complex features of struct (bitfields, enums, etc)? Those account for a good chunk of the complexity in the current struct decoder, and was the main reason I wanted to pre-calculate bit positions for all of the fields. If everything is aligned to the bytes that would simplify things significantly.

mcm001 commented 2 months ago

Photon doesn't currently have a need for enums or bitfield types (and bytes are cheap), but my original intent was to make this a superset of the wpistruct feature set. Does it make more sense to exclude enums and bit fields from this initial release and add support back if this protocol ever gets upstreamed?

jwbonner commented 2 months ago

Possibly, or make it part of the spec from the start and have AdvantageScope throw a warning about it for now. Honestly I’m not sure exactly how complex the implementation is likely to be, so it might end up being reasonable. Regardless it sounds like a low priority for the immediate future.

mcm001 commented 2 months ago

Yeah agreed. I'll make them optional extensions for now, with a promise that future revisions will include support. I can take a stab at copy-pasting the current struct parsing code and adapting it over the next few weeks, but if you wanna do it instead (and not need to rewrite all my code) go for it haha

jwbonner commented 2 months ago

I doubt I'll have much time to work on this before the first beta in a few weeks, but later in the fall I can look at it in more detail. Feel free to put something together in the meantime if you like.