Message serialization refactoring #140

qwerty19106 · 2022-11-26T12:42:22Z

This refactoring optimizes code for embedded context and tiny increases performance generally.
Also it simplify AsyncReader code, which I plan to send in next PR.

Message serialization moved to MAVLinkV1MessageRaw::serialize_message and MAVLinkV2MessageRaw::serialize_message methods to get common serialization API. The write_v1_msg and write_v2_msg methods has been left as thin wrapper.
As variant, this code should be placed to MavFrame::ser method. But MavFrame::ser body causes the some questions.
MAVLinkV1MessageRaw and MAVLinkV2MessageRaw has been make as thin struct over byte array. It allows send serialized message over Direct Memory Acces on embedded and other contexts. DMA is very fast in comparison with interrupts or polling.
It break MAVLinkV1MessageRaw and MAVLinkV2MessageRaw API: fields becomes methods.
I removed unnecessary bytes copying at payload serialization. BytesMut was been rewrite as thin wrapper over slice.
It is breaking change: Message::ser method was changed. But I hope that no one uses Message::ser method directly.

qwerty19106 · 2022-11-26T13:35:33Z

The v1_encode_decode_tests is failed, but CI is succesfull. It is strange.

qwerty19106 · 2022-11-28T11:51:06Z

It is ready to review

src/bytes_mut.rs

patrickelectric · 2022-11-28T19:20:30Z

The code looks great, the only thing necessary is to remove the old commented code.

src/lib.rs

patrickelectric · 2022-12-01T15:40:01Z

I would just recommend to squash the latest commit on the specific commits that it's fixing.
@danieleades, any more comments ?

danieleades · 2022-12-01T15:46:18Z

src/lib.rs

-
-    let payload = &mut message.payload_buffer[..message.payload_length as usize];
-    reader.read_exact(payload)?;
+    let mut message = MAVLinkV1MessageRaw::new();


Suggested change

let mut message = MAVLinkV1MessageRaw::new();

let mut message = Self::new();

The read_v1_raw_message function is not method. I could not apply Self here.

danieleades · 2022-12-01T15:48:50Z

src/lib.rs

    pub fn payload(&self) -> &[u8] {
-        &self.payload_buffer[..self.payload_length as usize]
+        let payload_length = self.payload_length() as usize;


lossless conversions-

Suggested change

let payload_length = self.payload_length() as usize;

let payload_length: u8 = self.payload_length().into();

the type annotation might not even be required here

The payload_length method return u8. Casting from u8 to usize is loseless.

Casting from a smaller integer to a larger integer (e.g. u8 -> u32) will
zero-extend if the source is unsigned

I could change payload_length return type to usize if needed.

The payload_length method return u8. Casting from u8 to usize is loseless.

Casting from a smaller integer to a larger integer (e.g. u8 -> u32) will
zero-extend if the source is unsigned

I could change payload_length return type to usize if needed.

I understand the cast is lossless. That's why you might as well use the explicitly lossless method.

Doing the conversion once, inside the payload_length method, sounds like a good approach

the mavlink protocol defines the payload_length as u8, we should stay with it to make the protocol definition clear.

danieleades · 2022-12-01T15:49:32Z

src/lib.rs

-        &mut self.payload_buffer[..self.payload_length as usize]
+    #[inline]
+    pub fn checksum(&self) -> u16 {
+        let payload_length = self.payload_length() as usize;


use lossless conversion

danieleades · 2022-12-01T15:50:33Z

src/lib.rs

+    }
+
+    fn mut_payload_and_checksum_and_sign(&mut self) -> &mut [u8] {
+        let payload_length = self.payload_length() as usize;


use lossless conversion

i won't bother writing all of these out. You should be find them all with something like

cargo +nightly clippy -- -A clippy::all -W clippy::cast_lossless

danieleades · 2022-12-01T15:52:01Z

src/utils.rs

@@ -1,14 +1,11 @@
-use crate::{bytes_mut::BytesMut, MAX_FRAME_SIZE};
-
 /// Removes the trailing zeroes in the payload
 ///
 /// # Note:
 ///
 /// There must always be at least one remaining byte even if it is a
 /// zero byte.
 #[allow(dead_code)]


why do we need to allow dead code here?

That's not related to the PR.

It was #[allow(dead_code)] before this PR. I will remove it.

danieleades

Just a couple of little nits

qwerty19106 · 2022-12-02T11:56:14Z

@patrickelectric, could I squash all commits to single commit?

patrickelectric · 2022-12-02T12:06:47Z

@patrickelectric, could I squash all commits to single commit?

Yes.

patrickelectric · 2022-12-02T13:17:10Z

Thanks for the PR @qwerty19106, nice work!

qwerty19106 force-pushed the refactor_message_raw branch from b0a0ffe to 440645d Compare November 26, 2022 13:45

qwerty19106 changed the title ~~[WIP] Message serialization refactoring~~ Message serialization refactoring Nov 28, 2022

patrickelectric requested changes Nov 28, 2022

View reviewed changes

src/bytes_mut.rs Outdated Show resolved Hide resolved

danieleades reviewed Nov 29, 2022

View reviewed changes

src/lib.rs Show resolved Hide resolved

danieleades reviewed Nov 29, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

danieleades reviewed Nov 29, 2022

View reviewed changes

src/lib.rs Show resolved Hide resolved

patrickelectric reviewed Nov 30, 2022

View reviewed changes

src/lib.rs Show resolved Hide resolved

patrickelectric approved these changes Nov 30, 2022

View reviewed changes

qwerty19106 force-pushed the refactor_message_raw branch from b539218 to 5ef6078 Compare November 30, 2022 13:42

danieleades reviewed Dec 1, 2022

View reviewed changes

danieleades suggested changes Dec 2, 2022

View reviewed changes

MAVLinkV1MessageRaw and MAVLinkV2MessageRaw refactoring

68938b1

qwerty19106 force-pushed the refactor_message_raw branch from 5ef6078 to 68938b1 Compare December 2, 2022 12:39

patrickelectric merged commit 2f9ef39 into mavlink:master Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message serialization refactoring #140

Message serialization refactoring #140

qwerty19106 commented Nov 26, 2022

qwerty19106 commented Nov 26, 2022

qwerty19106 commented Nov 28, 2022

patrickelectric commented Nov 28, 2022

patrickelectric commented Dec 1, 2022

danieleades Dec 1, 2022

qwerty19106 Dec 2, 2022

danieleades Dec 2, 2022

danieleades Dec 1, 2022 •

edited

Loading

qwerty19106 Dec 2, 2022

danieleades Dec 2, 2022

patrickelectric Dec 2, 2022

danieleades Dec 1, 2022

danieleades Dec 1, 2022

danieleades Dec 1, 2022

patrickelectric Dec 2, 2022 •

edited

Loading

qwerty19106 Dec 2, 2022

danieleades left a comment

qwerty19106 commented Dec 2, 2022

patrickelectric commented Dec 2, 2022

patrickelectric commented Dec 2, 2022

	let mut message = MAVLinkV1MessageRaw::new();
	let mut message = Self::new();

	let payload_length = self.payload_length() as usize;
	let payload_length: u8 = self.payload_length().into();

Message serialization refactoring #140

Message serialization refactoring #140

Conversation

qwerty19106 commented Nov 26, 2022

qwerty19106 commented Nov 26, 2022

qwerty19106 commented Nov 28, 2022

patrickelectric commented Nov 28, 2022

patrickelectric commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieleades Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieleades left a comment

Choose a reason for hiding this comment

qwerty19106 commented Dec 2, 2022

patrickelectric commented Dec 2, 2022

patrickelectric commented Dec 2, 2022

danieleades Dec 1, 2022 •

edited

Loading

patrickelectric Dec 2, 2022 •

edited

Loading