remove RustDefault

Author: danieleades

CODE-REVIEW.md

@@ -0,0 +1,57 @@
+# Code Review
+
+## Findings (ordered by severity)
+
+- [High] Signed 24-bit encode/decode is incorrect: decode zero-extends and encode rejects negative values.
+  - mavlink-core/src/bytes.rs:145
+  - mavlink-core/src/bytes_mut.rs:121
+  - Impact: negative `int24` values are parsed as large positives and serialization panics for valid negative values.
+  - Fix: sign-extend on read and use a negative MIN (-(1 << 23)) on write; consider unit tests for `i24` round-trips.
+
+- [High] Build script requires `git`, mutates the submodule, and ignores non-zero exit statuses.
+  - mavlink/build/main.rs:13
+  - mavlink/build/main.rs:19
+  - mavlink/build/main.rs:35
+  - Impact: builds can fail in offline/crates.io environments or produce inconsistent definitions; patch failures are silently ignored because only spawn errors are checked.
+  - Fix: ship patched XML definitions (or pre-generated Rust), avoid VCS operations in `build.rs`, and check `status.success()` for `git` calls if they remain.
+
+- [Medium] `tokio-1` and `embedded` are documented as incompatible but not enforced, and they define duplicate async APIs.
+  - mavlink-core/src/lib.rs:875
+  - mavlink-core/src/lib.rs:899
+  - Impact: enabling both features produces duplicate symbol errors and confusing APIs.
+  - Fix: add a `compile_error!` guard for `cfg(all(feature = "tokio-1", feature = "embedded"))` or gate one set of functions with `not(feature = "embedded")`.
+
+- [Medium] UDP (sync/async) `recv` loops swallow all errors, and file `recv` ignores non-EOF I/O errors.
+  - mavlink-core/src/connection/udp.rs:90
+  - mavlink-core/src/async_connection/udp.rs:92
+  - mavlink-core/src/connection/file.rs:45
+  - Impact: persistent I/O errors turn into infinite loops or busy-spins; callers cannot observe disconnects or socket failures.
+  - Fix: only ignore parse/CRC errors; propagate `MessageReadError::Io` (except maybe `WouldBlock`/`UnexpectedEof` where appropriate).
+
+- [Low] `PeekReader`/`AsyncPeekReader` rejects exact buffer-size reads even though docs say “more than BUFFER_SIZE”.
+  - mavlink-core/src/peek_reader.rs:141
+  - mavlink-core/src/async_peek_reader.rs:139
+  - Impact: `peek_exact(BUFFER_SIZE)` panics; API behavior does not match documentation.
+  - Fix: change `< BUFFER_SIZE` to `<= BUFFER_SIZE` and update docs/tests.
+
+- [Low] Address format docs and examples use `udpbcast`, but the parser accepts `udpcast`.
+  - mavlink-core/src/connectable.rs:78
+  - mavlink-core/src/async_connection/mod.rs:91
+  - mavlink/examples/mavlink-dump/src/main.rs:10
+  - Impact: user confusion and copy/paste failures.
+  - Fix: accept both or standardize documentation and CLI help on one string.
+
+## Simplification / existing-crate opportunities
+
+- Replace custom `bytes`/`bytes_mut` helpers with `byteorder::ByteOrder` and/or `bytes::Buf`/`BufMut` for most primitives, keeping a small custom helper only for `u24/i24`. This reduces bespoke parsing code and risk (the current i24 bug is a good example).
+  - mavlink-core/src/bytes.rs
+  - mavlink-core/src/bytes_mut.rs
+  - mavlink-bindgen/src/parser.rs:806
+
+- Drop `utils::RustDefault` in favor of `Default` (arrays implement `Default` on Rust 1.80). Update codegen to use `#[serde(default)]` instead of a custom default function.
+  - mavlink-core/src/utils.rs
+  - mavlink-bindgen/src/parser.rs:752
+  - mavlink/src/lib.rs:68
+
+- Consider splitting the 2300-line `mavlink-core/src/lib.rs` into focused modules (frame structs, parsing, write APIs, connection glue) or using small macros for the repeated v1/v2 + sync/async variants to reduce duplication and review surface.
+  - mavlink-core/src/lib.rs

mavlink-bindgen/src/parser.rs

@@ -746,11 +746,7 @@ impl MavMessage {
                 // If sent by an implementation that doesn't have the extensions fields
                 // then the recipient will see zero values for the extensions fields.
                 let serde_default = if field.is_extension {
-                    if field.enumtype.is_some() {
-                        quote!(#[cfg_attr(feature = "serde", serde(default))])
-                    } else {
-                        quote!(#[cfg_attr(feature = "serde", serde(default = "crate::RustDefault::rust_default"))])
-                    }
+                    quote!(#[cfg_attr(feature = "serde", serde(default))])
                 } else {
                     quote!()
                 };

mavlink-core/src/lib.rs

@@ -90,8 +90,6 @@ use core::result::Result;
 use std::io::{Read, Write};
 
 pub mod utils;
-#[allow(unused_imports)]
-use utils::{remove_trailing_zeroes, RustDefault};
 
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};

mavlink-core/src/types.rs

@@ -115,9 +115,9 @@ impl<const N: usize> From<&str> for CharArray<N> {
     }
 }
 
-impl<const N: usize> crate::utils::RustDefault for CharArray<N> {
+impl<const N: usize> Default for CharArray<N> {
     #[inline(always)]
-    fn rust_default() -> Self {
+    fn default() -> Self {
         Self::new([0u8; N])
     }
 }

mavlink-core/src/utils.rs

@@ -16,47 +16,6 @@ pub fn remove_trailing_zeroes(data: &[u8]) -> usize {
     len
 }
 
-/// A trait very similar to [`Default`] but is only implemented for the equivalent Rust types to
-/// `MavType`s.
-///
-/// This is only needed because rust doesn't currently implement `Default` for arrays
-/// of all sizes. In particular this trait is only ever used when the "serde" feature is enabled.
-/// For more information, check out [this issue](https://users.rust-lang.org/t/issue-for-derives-for-arrays-greater-than-size-32/59055/3).
-pub trait RustDefault: Copy {
-    fn rust_default() -> Self;
-}
-
-impl<T: RustDefault, const N: usize> RustDefault for [T; N] {
-    #[inline(always)]
-    fn rust_default() -> Self {
-        let val: T = RustDefault::rust_default();
-        [val; N]
-    }
-}
-
-macro_rules! impl_rust_default {
-    ($($t:ty => $val:expr),* $(,)?) => {
-        $(impl RustDefault for $t {
-            #[inline(always)]
-            fn rust_default() -> Self { $val }
-        })*
-    };
-}
-
-impl_rust_default! {
-    u8 => 0,
-    i8 => 0,
-    u16 => 0,
-    i16 => 0,
-    u32 => 0,
-    i32 => 0,
-    u64 => 0,
-    i64 => 0,
-    f32 => 0.0,
-    f64 => 0.0,
-    char => '\0',
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

mavlink/src/lib.rs

@@ -63,10 +63,6 @@ include!(concat!(env!("OUT_DIR"), "/mod.rs"));
 
 pub use mavlink_core::*;
 
-#[cfg(feature = "emit-extensions")]
-#[allow(unused_imports)]
-pub(crate) use mavlink_core::utils::RustDefault;
-
 #[cfg(feature = "serde")]
 #[allow(unused_imports)]
 pub(crate) use mavlink_core::utils::nulstr;