sequenceplanner/r2r

Improvement for static array support

JiangengDong opened this issue · 4 comments

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 commented

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.

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

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

m-dahl commented

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?