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

Improvement for static array support #48

Open
JiangengDong opened this issue May 22, 2023 · 4 comments
Open

Improvement for static array support #48

JiangengDong opened this issue May 22, 2023 · 4 comments

Comments

@JiangengDong
Copy link

I came into a problem with static array support when using r2r.

As is described in the ROS 2 interfaces, ROS message supports static arrays where the array size is known at compile time.

For example, if I have a message ValueGroup.msg,

# file: example/msg/ValueGroup.msg
uint8[5] values

The corresponding C struct is

typedef struct example__msg__ValueGroup
{
  uint8_t values[6];
} example__msg__ValueGroup;

However, the generated Rust binding uses Vec.

pub struct ValueGroup {
    values: Vec<u8>
}

And the publisher panics if it receives a ValueGroup with an incorrect size.

I saw a comment from 4 years ago. https://github.com/sequenceplanner/r2r/blob/master/r2r_msg_gen/src/lib.rs#L392

// actually lets use a vector anyway because its more convenient with traits. assert on the fixed size instead!

However, since "const generic" has been stable for a long time (since Rust 1.51), is it possible to bring the static array back?

Again thanks for writing and maintaining this awesome crate! Forgive me if I raise issues too frequently.

@m-dahl
Copy link
Collaborator

m-dahl commented May 23, 2023

No I want to thank you for wanting to improve the crate. I think it makes sense to go back to arrays to keep the implementation a bit closer to the c code. In fact we already rely on retain_mut being in the standard library, which was added in 1.61, so the minimum supported rust version is not an issue for this. It might be a while until I have time to work on it though.

@JiangengDong
Copy link
Author

How much workload do you think it is? I am willing to raise a PR for this.

@JiangengDong
Copy link
Author

And would you mind if I rewrite the generate_rust_msg function with quote!? The string interpolation style looks crazy to me.

@m-dahl
Copy link
Collaborator

m-dahl commented May 24, 2023

How much workload do you think it is? I am willing to raise a PR for this.

It's probably not much work, perhaps you only need to revert to the code in the old comment. But then some work to verify that it works properly.

And would you mind if I rewrite the generate_rust_msg function with quote!? The string interpolation style looks crazy to me.

I would love it, have been thinking about it many times myself. Perhaps as a separate PR?

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

No branches or pull requests

2 participants