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

Support new PhotonVision struct format #175

Open
mcm001 opened this issue Sep 2, 2024 · 7 comments
Open

Support new PhotonVision struct format #175

mcm001 opened this issue Sep 2, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@mcm001
Copy link
Contributor

mcm001 commented Sep 2, 2024

For 2025,photon is moving to auto-generated struct code (see PhotonVision/photonvision@169595e ). 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 mcm001 added the enhancement New feature or request label Sep 2, 2024
@mcm001
Copy link
Contributor Author

mcm001 commented Sep 2, 2024

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

@mcm001
Copy link
Contributor Author

mcm001 commented Sep 10, 2024

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
Copy link
Member

jwbonner commented Sep 10, 2024

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
Copy link
Contributor Author

mcm001 commented Sep 11, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mcm001 commented Sep 11, 2024

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
Copy link
Member

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.

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

No branches or pull requests

2 participants