From 719fec669836f28e3683ff8e8c6fd35de8b8ff44 Mon Sep 17 00:00:00 2001 From: Daniel Eades Date: Sat, 20 Dec 2025 18:39:25 +0000 Subject: [PATCH 1/2] remove RustDefault --- CODE-REVIEW.md | 57 +++++++++++++++++++++++++++++++++++ mavlink-bindgen/src/parser.rs | 6 +--- mavlink-core/src/lib.rs | 2 -- mavlink-core/src/types.rs | 4 +-- mavlink-core/src/utils.rs | 41 ------------------------- mavlink/src/lib.rs | 4 --- 6 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 CODE-REVIEW.md diff --git a/CODE-REVIEW.md b/CODE-REVIEW.md new file mode 100644 index 00000000000..8a3ec127e44 --- /dev/null +++ b/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 diff --git a/mavlink-bindgen/src/parser.rs b/mavlink-bindgen/src/parser.rs index 98286f6b418..3a42ef2600f 100644 --- a/mavlink-bindgen/src/parser.rs +++ b/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!() }; diff --git a/mavlink-core/src/lib.rs b/mavlink-core/src/lib.rs index dbe5ed5c1f7..c1d17879297 100644 --- a/mavlink-core/src/lib.rs +++ b/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}; diff --git a/mavlink-core/src/types.rs b/mavlink-core/src/types.rs index ee72e056320..3b5f2ac635f 100644 --- a/mavlink-core/src/types.rs +++ b/mavlink-core/src/types.rs @@ -115,9 +115,9 @@ impl From<&str> for CharArray { } } -impl crate::utils::RustDefault for CharArray { +impl Default for CharArray { #[inline(always)] - fn rust_default() -> Self { + fn default() -> Self { Self::new([0u8; N]) } } diff --git a/mavlink-core/src/utils.rs b/mavlink-core/src/utils.rs index 0603c77f61d..2eed9c88aa7 100644 --- a/mavlink-core/src/utils.rs +++ b/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 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::*; diff --git a/mavlink/src/lib.rs b/mavlink/src/lib.rs index 836ff57af6b..02c0067d08c 100644 --- a/mavlink/src/lib.rs +++ b/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; From b890b20c8b932fac866c7f55a49941967fe73b16 Mon Sep 17 00:00:00 2001 From: Daniel Eades Date: Sat, 20 Dec 2025 18:40:33 +0000 Subject: [PATCH 2/2] fixup --- CODE-REVIEW.md | 57 -------------------------------------------------- 1 file changed, 57 deletions(-) delete mode 100644 CODE-REVIEW.md diff --git a/CODE-REVIEW.md b/CODE-REVIEW.md deleted file mode 100644 index 8a3ec127e44..00000000000 --- a/CODE-REVIEW.md +++ /dev/null @@ -1,57 +0,0 @@ -# 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