Ideas to slightly improve performance

[anti-social]

With current approach I can see that it is not possible to eliminate extra copying:

        for c in data.iter_mut() {
            *c = self.buf.pop_front().unwrap_or(0);
        }

What I want is to rid of the circular buffer. To be able to do that I would like to change the parser using some kind of a state machine:

enum State {
    WaitingForSync,
    WaitingForLen,
    InvalidLen(u8), // invalid length that should be returned when `next_packet` is called
    ExpectData(u8), // length of data to expect

That will move initial parsing to the push_bytes method.

Also push_bytes should return non-consumed slice of bytes. When push_bytes returns non-empty slice, it means next_packet must be called. It will allow to keep max buffer size of at most 64 bytes. As I can see at the moment we just silently swallow data when a buffer is overflowed.

And finally I think we can allow a user to pass a buffer to a parser. Then we could just return a slice of the buffer without any copying at all (when we use next_raw_packet).

If you don't mind I will try to implement that.

[tact1m4n3]

Sure. Go ahead. But try to keep the api simple. I mean don't overcomplicate things on the user end.

[tact1m4n3]

I implemented this. Closing issue... Also I thought about removing copies completely, but I couldn't find a way to achieve it (packets may not end in the same buffer)

[anti-social]

Also had a pretty similar patch but haven't finished it.

Also I thought about removing copies completely, but I couldn't find a way to achieve it (packets may not end in the same buffer)

Yeah, it is definitely not possible to completely remove copying.

I also had an idea that we don't need parse_packet method at all as we could return a parsed packet from push_bytes method if we have consumed all the packet data. It reduces API complexity (a user won't forget to call parse_packet method before pushing next chunk), but in this case we should return more complex type. Something like:

pub fn push_bytes(
    &mut self,
    bytes: &[u8]
) -> (Option<Result<RawPacket<'_>, PacketError>>, usize)  // usize is a number of consumed bytes

Either we should wrap it in into some struct.

[tact1m4n3]

I agree this is better but either way the API is quite hard to use. Maybe another way is to pass to the packet reader a struct implementing a trait like ReaderSource. It can be an Uart port(or anything really as long as it implements that trait, even a wrapper around a slice of bytes). This would eliminate any kind of copying as the user would no longer have to read from the source manually. Also I was thinking of implementing a complete parse function in the packet struct that would take in a slice of bytes (for parsing single packets, that don't come from a stream). Reopening the issue :))

[tact1m4n3]

Actually, on second thought, a trait might not be appropriate here.

[peterkrull]

I think doing a wrapper trait around hardware/streams makes the library less usable. For example I do not like how rust-mavlink does not just focus on parsing, but also wants to do the actual reading from hardware or a stream. At least it made me trying to do async impossible using the official API. I like the approach of just taking in a buffer, reading some or all bytes, and returning a tuple with the result of parsing, and how much of the buffer is available or taken. Then the user of the library is free to obtain the buffer however they see fit. I would probably prefer the following interface, where I also handle the case of not being able to parse due to the state machine not being finished as an error, since the function is supposed to both take bytes and parse.

pub fn push_bytes_try_parse<'a>(
    &mut self,
    bytes: &'a [u8]
) -> (Result<Packet, PacketError>, &'a [u8])

I think worrying too much about getting rid of every last copy at the expense of usability and API clarity would be a bad idea. Currently there are other areas where we could get much more performance at little to no cost :) #15 #16

[tact1m4n3]

Yep. I definitely agree with you. I was thinking of adding apart from the usual push_bytes function a push_bytes_and_parse function, but the return type would become very complex - (Option<Result<Packet, ParseError>>, usize). Also there must be a way to access the address and the extended destination and source of packets. This is another thing that I have no idea where to put. Initially I was thinking of making methods for this in the raw packet struct but idk if it's the best way to do this.

[anti-social]

I also thought of making a wrapper type but had a doubt that in this case a user can mistakenly ignore the rest of the buffer.

loop {
    // read data into buffer
    let parse_res = parser.push_bytes(buf);
    let packet = parse_res.packet()?;
    // work with the packet
}

There won't be any warnings that the code is wrong and the rest of the buffer should be pushed into the parser again (if it is not empty).