tact1m4n3 / crsf-rs

A work in progress crsf protocol packet parser
MIT License
10 stars 7 forks source link

[wip] Optionally parse sync (address) byte #22

Closed anti-social closed 4 months ago

anti-social commented 4 months ago

To be honest I'm not sure it is good solution for now.

It provides several constructors to get PacketReader that produces different RawPacket types.

Another option is to put address: Option<PacketAddress> field into RawPacket and Packet structs but the drawback is that we will have to check it before access (or unwrap).

tact1m4n3 commented 4 months ago

I think the buffer struct is a good idea anyway. In relation to the address encoder decoder generics, it maybe kind off confusing for anyone trying to use the crate(that just wants to parse some packets from a rx :)) ). Maybe we can make it a feature?

anti-social commented 4 months ago

I can imagine situation when someone will need both readers. For example some kind of a proxy between an i2c device and a uart one.

tact1m4n3 commented 4 months ago

We should probably ignore i2c for now as we don't have any examples of it being used. Also, as a side note, there is no specification that the sync byte is actually a packet address :))

I noticed in your code that the header length is no longer a constant. Why is that?

peterkrull commented 4 months ago

I don't like adding too many things, such as generics or optionals, that will have no use for the +90% of users that might just want to parse RcChannelsPacked. I am still a proponent of the owned RawPacket, and I think the simplest version would suffice (I have opted for a heapless::Vec in my wip) where the sync/addr byte should be manually re-evaluated if used for routing:

/// Represents the not-yet-parsed data of a packet.
pub struct RawPacket(Vec<u8, 64>);

or alternatively, with a PacketAddress always present:

/// Represents the not-yet-parsed data of a packet.
/// The `addr` field represents the "sync" byte used
/// to find the start of the packet, which is sometimes
/// also used as a destination address.
pub struct RawPacket {
    addr: PacketAddress, 
    buf: Vec<u8, 64>,
};

And then still use the PacketAddressFlags bitflags when searching for the sync/address byte. The 90% of users will probably never even bother to work with the RawPacket anyway, and would just want the finished parsed Packet.

Since this use case is not really a part of the spec, I would not want many things relating to it to bleed out where others may stumble into it. Wouldn't it be enough to to have the sync byte be customizable via a method, and the contents of the RawPacket available?

tact1m4n3 commented 4 months ago

I am still a proponent of the owned RawPacket

It's fine by me. It's almost certain that it won't be a performance issue and it can make the api easier to work with

tact1m4n3 commented 4 months ago

Wouldn't it be enough to to have the sync byte be customizable via a method, and the contents of the RawPacket available?

I agree with you. Since it's something not in accordance with the spec we shouldn't support it make everything so much more complicated. Making a method is an option, as well as having a list of allowed sync bytes that would contain 0xC8, 0xEE and others that may appear and having a sync() -> u8 method in the raw packet struct that would return that byte.