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 for Fixed Sized Arrays #150

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Support for Fixed Sized Arrays #150

merged 7 commits into from
Feb 16, 2024

Conversation

Carter12s
Copy link
Collaborator

@Carter12s Carter12s commented Feb 8, 2024

Description

ROS1 serialization with serde_rosmsg requires that fixed sized arrays be represented not as Vec<> but as [; N]. The two are sent over the wire differently. Most of the support for this was already plumbed through, so this just closed the loop at the end, but is hitting the "const generic" barrier.

Fixes

Closes: #148

Checklist

  • Update CHANGELOG.md

@Carter12s
Copy link
Collaborator Author

@ssnover Do you have any good ideas on how we should handle this situtation?

  1. Is there a workaround where maybe we can use some other type than [T; N] to represent a fixed sized array?
  2. Can we codegen our own trait impls for larger array sizes? (This seems awful)
  3. Do we just declare our crate only works with fixed sized arrays <32 elements until const generics land? can we limit that impact to ROS1 usage only? probably not cause it affects codegen...

Am I just missing an obvious solve here.

@ssnover
Copy link
Collaborator

ssnover commented Feb 8, 2024

We have at least one instance showing in the codegen for a 36 member array, so we probably ought to at least support that as a basic ROS message.

Only alternative I can think of is to represent with a tuple of Vec and the fixed size and then define our own serialization and deserialization for serde_rosmsg?

@ssnover
Copy link
Collaborator

ssnover commented Feb 8, 2024

Actually, how does serde_rosmsg handle Box<[T]>?

@Carter12s
Copy link
Collaborator Author

I have a little side test setup in my fork of serde_rosmsg, from when the bug was originally reported, where I'm deserializing a NavSatFix.msg with the library. NavSatFix has an [f64; 9] in it.

For Box<[f64]> serde_rosmsg spat out an underflow error at runtime.
For &[f64] we get a compile time "Deserialize<'_> is not implemented"

So don't see a good path there...
I may open an issue on serde_rosmsg and see if they have a solve...

@Carter12s
Copy link
Collaborator Author

Carter12s commented Feb 8, 2024

https://stackoverflow.com/questions/62665558/how-can-i-implement-serdedeserialize-for-arrays-larger-than-32

Has lead me to: https://crates.io/crates/serde-big-array

Which I'm going to play with and see if it can solve this problem...

See root serde issue here: serde-rs/serde#1937

@Carter12s
Copy link
Collaborator Author

So it looks like a combination of some fuckery with smart_default's _code directive and this serde-big-array crate I may have gotten a working solution.

Biggest problem I see is 1) the code is super messy and could use some unit tests, and 2) we're leaking yet another crate dependency through the codegen interface in not a great way...

I'd love your eyes on this @ssnover

@Carter12s Carter12s changed the title [Not Ready for Merge] Support for Fixed Sized Arrays Support for Fixed Sized Arrays Feb 8, 2024
Copy link
Collaborator

@ssnover ssnover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for taking the time to figure out the attribute macro magic required to get this functioning and adding tests.

I pointed out the current CI failures in comments.

roslibrust_codegen/src/gen.rs Outdated Show resolved Hide resolved
roslibrust_codegen/src/parse/mod.rs Show resolved Hide resolved
roslibrust_codegen/src/lib.rs Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Show resolved Hide resolved
@Carter12s Carter12s dismissed ssnover’s stale review February 16, 2024 03:05

Actions completed, not sure how to "say done"

@Carter12s Carter12s merged commit 599332b into master Feb 16, 2024
5 checks passed
@Carter12s Carter12s deleted the codegen-fixed-arrays branch May 21, 2024 21:15
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.

ROS1 MAVROS Decode Failure: Decoded data is shorter than predicted value length
2 participants