Support async contexts and make the crate more transport-agnostic

[allsey87]

Hi there, I needed to decode MAVLink messages in an async application based on the Tokio executor, so I went ahead and implemented a decoder using tokio_util's Decoder trait and this library here: codec.rs. With this trait in place, I am able to decode messages asynchronously with just a few lines of code:

async fn example() {
    let mut mavlink = match TcpStream::connect((MAVLINK_IP, MAVLINK_PORT)).await {
        Ok(stream) => {
            FramedRead::new(stream, codec::MavMessageDecoder::<mavlink::common::MavMessage>::new())
        },
        Err(error) => {
            log::error!("Failed to connect");
            return;
        }
    };
    while let Some(message) = mavlink.next().await {
        match message {
            Ok(message) => log::info!("{:?}", message),
            Err(error) => log::error!("{}", error),
        }
    }
}

This exercise has revealed a couple design flaws in this library. One particular code smell that I picked up on was that I had to duplicate a lot of code in this library to achieve what I was doing. Referring back to codec.rs, I don't think lines 39 to 85 belong in my code and this functionality should be provided by the library.

I am still relatively new to Rust, but I have seen enough of the ecosystem to put forward the following proposals for the library:

1. This crate should do one thing and do it well, provide types and traits for MAVLink messages and a means for converting them from/to u8 buffers or perhaps from/to the Buf, BufMut traits from the bytes crate. In particular, something like:

   impl<B: bytes::Buf, M: mavlink::Message> std::convert::TryFrom<B> for M {
   type Error = mavlink::error::MessageReadError;
   
   fn try_from(buffer: B) -> std::result::Result<Self, Self::Error> {
       todo!()
   }
   }

2. The concepts, types, and details of transports such as TCP, UDP, or UART and the various libraries that implement these transports should not leak into crate. Referring to issue #16, I disagree that this library should have any methods like recv regardless of whether they are synchronous or asynchronous. For sure, any use case will involve one of these transports, but that code and its dependencies belong in the examples.

What I propose above are indeed breaking changes, however, it would make the library simpler, more flexible (i.e., regarding async vs. sync) and agnostic to the underlying transport. I believe this would also make no_std support easier (i.e. less functionality hidden behind feature gates).

[wucke13]

This sounds like a good idea! I'd also prefer having a pure types & serde library, and another, separate lib for the communication interfaces. I started working on some kind of communication library over at https://crates.io/crates/async-mavlink . I think it would make sense to just provide an async, libstd communication library. I don't think we should prioritize a communication library for no_std, as the landscape of IO is too heterogene there.

Edit: Also this might bring some new light for https://github.com/mavlink/rust-mavlink/issues/78 .

[danieleades]

this proposal makes a lot of sense. Likely possible to do this without breaking changes by pulling the transport-agnostic code into a separate 'core' crate in the workspace.

[al2950]

Hi, just wondering if any progress was made with this ticket. I agree with the points made by the OP, with regard to the fact the this library should focus on just mavlink message parsing.

I also think this is a much simpler issue to solve, please see small update I made here https://github.com/mavlink/rust-mavlink/commit/751c8dda0a5adacb34ba6952935d8e948fcabbfe

By making a couple of functions public and updating the new() functions you can do something like this (simplified for this example);

async fn recv_msg_async<R: AsyncReadExt + Unpin>(
    stream: &mut R,
) -> Result<mavlink::ardupilotmega::MavMessage, String> {
    loop {
        let byte = stream.read_u8().await.map_err(|e| e.to_string())?;

        if byte != mavlink::MAV_STX_V2 {
            continue;
        }

        let mut msg_raw = mavlink::MAVLinkV2MessageRaw::new();
        stream
            .read_exact(msg_raw.mut_header())
            .await
            .map_err(|e| e.to_string())?;
        stream
            .read_exact(msg_raw.mut_payload_and_checksum_and_sign())
            .await
            .map_err(|e| e.to_string())?;

        if !msg_raw.has_valid_crc::<mavlink::ardupilotmega::MavMessage>() {
            return Err("CRC Failed".to_owned());
        }

        let mav_msg = <mavlink::ardupilotmega::MavMessage as mavlink::Message>::parse(
            mavlink::MavlinkVersion::V2,
            msg_raw.message_id(),
            msg_raw.payload(),
        )
        .map_err(|e| e.to_string())?;

        return Ok(mav_msg);
    }
}

Happy to make a PR if this is an acceptable approach.