postcard-schema: nalgebra schema is incorrect

[jamesmunns]

Hey @avsaase - I was wondering how you chose Seq(T) for the schema of nalgebra::Matrix. A Seq is a length prefixed item, so a 3x3 matrix of u8s should be 10 bytes. However this test fails:

use nalgebra_v0_33::{Const, Matrix};
#[test]
fn smoke() {
    let x = nalgebra_v0_33::Matrix::<u8, Const<3>, Const<3>, _>::new(
        1, 2, 3,
        4, 5, 6,
        7, 8, 9,
    );
    let y = postcard::to_stdvec(&x).unwrap();
    assert_eq!(&[9, 1, 4, 7, 2, 5, 8, 3, 6, 9], y.as_slice());
}

test impls::nalgebra_v0_33::test::smoke ... FAILED

failures:

---- impls::nalgebra_v0_33::test::smoke stdout ----
thread 'impls::nalgebra_v0_33::test::smoke' panicked at source/postcard-schema/src/impls/nalgebra_v0_33.rs:32:9:
assertion `left == right` failed
  left: [9, 1, 4, 7, 2, 5, 8, 3, 6, 9]
 right: [1, 4, 7, 2, 5, 8, 3, 6, 9]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Was the choice of Seq(T) a guess? Or were you basing this on something else? Just making sure I understand the original choice before I change it.

It appears to me that the actual serialized form is actually [u8; 9], which is not a length prefixed, but is actually modeled as a tuple of size N, which avoids serializing a fixed size len:

impl<T: Schema, const N: usize> Schema for [T; N] {
    const SCHEMA: &'static NamedType = &NamedType {
        name: "[T; N]",
        ty: &DataModelType::Tuple(&[T::SCHEMA; N]),
    };
}

[avsaase]

Hi, I don't recall exactly why I picked a Seq(T). I guess I thought this would be the correct format but I should have added this test myself to confirm. Sorry about that!

[jamesmunns]

So it turns out this is very frustrating to fix, this is what I've come up with:

#[cfg_attr(docsrs, doc(cfg(feature = "nalgebra-v0_33")))]
impl<T, const R: usize, const C: usize> Schema
    for nalgebra_v0_33::Matrix<
        T,
        nalgebra_v0_33::Const<R>,
        nalgebra_v0_33::Const<C>,
        nalgebra_v0_33::ArrayStorage<T, R, C>,
    >
where
    T: Schema + nalgebra_v0_33::Scalar,
{
    /// Warning! This is not TECHNICALLY correct. nalgebra actually serializes the
    /// ArrayStorage as `[T; R * C]`, however there is no way to express this below,
    /// as we cannot use generics from the outer context in const context to do
    /// what we really want here, which is `<[T; R * C]>::SCHEMA.ty`. For the
    /// purposes of postcard, these two are actually equivalent with respect to
    /// size and other meaning, but crates like `postcard-dyn` may make this
    /// difference visible, and give the user the nested arrays instead of the
    /// flat array.
    const SCHEMA: &'static NamedType = &NamedType {
        name: "nalgebra::Matrix<T, R, C, ArrayStorage<T, R, C>>",
        ty: <[[T; R]; C]>::SCHEMA.ty,
    };
}

This will need to be a breaking change to postcard-schema, since changing the schema will break users of postcard-rpc (if they update the HOST but not the MCU, then they will end up unable to use endpoints that contain nalgebra types).

I definitely need to raise the bar for required testing to add schema types!

Also no worries @avsaase, I made the same mistake with Uuid before I released, it's pretty annoying that it is necessary to do this manually.

So many things to learn :)