Extended packet format support

[tact1m4n3]

@anti-social @peterkrull Currently we have no way of dumping an extended packet(and specifying the extended dst/src fields). I found three ways of accomplishing this:

1. The dump method should take in the optional extended dst and src addresses(not that great)

   pub fn dump(&self, buf: &mut [u8], addr: PacketAddress, extended_src: Option<PacketAddress>, extended_dst: Option<PacketAddress>) -> Result<usize, BufferLenError>

2. We could make an Extended variant in the Packet enum, that would contain a new struct ExtendedPacket that further contains an ExtendedPacketPayload and the extended dst and src addresses. Also, we could make the Packet enum a struct that stores a PacketVariant and an address(which is currently overlooked) (this is kind off absurd :laughing:).

   
   pub struct Packet {
   addr: PacketAddress,
   payload: PacketPayload,
   }

pub enum PacketPayload { LinkStatistics(LinkStatistics), RcChannels(RcChannels), Extended(ExtendedPacket), }

pub struct ExtendedPacket { dst_addr: PacketAddress, src_addr: PacketAddress, payload: ExtendedPacketPayload, }

3. Or we can store in the new packet struct an Optional<(PacketAddress, PacketAddress)>(or smth of the kind) (this is what I would prefer)
```rust
pub struct Packet {
    addr: PacketAddress,
    src_addr: Option<PacketAddress>, // or stored in a separate struct like ExtendedPacketFields
    dst_addr: Option<PacketAddress>,
    payload: PacketPayload,
}

[peterkrull]

I am not sure I even understand how the extended format is supposed to work. Can any payload be sent with src_addr and dst_addr, or is it just the messages marked with Y in https://github.com/crsf-wg/crsf/wiki/Packet-Types? Or is it mandatory for those, and optional for others? Also how is addr different from dst_addr? or is the first addr really supposed to be a sync byte instead, as per the doc: "All serial CRSF packets begin with the CRSF SYNC byte 0xC8" Then maybe we could model it like this?

enum Payload {
    Classic {
        extended: Option<ExtendedFields>,
        payload: ClassicPayload,
    },
    Extended {
        extended: ExtendedFields,
        payload: ExtendedPayload,
    }
}

struct ExtendedFields {
    src_addr: PacketAddress,
    dst_addr: PacketAddress,
}

// Everything below type 0x28
enum ClassicPayload {
    ....
}

// Everything at or above type 0x28
enum ExtendedPayload {
    ....
}

[tact1m4n3]

Only packets with type >= 28 have the extended src and dst addr included right before their payload. Packets with type < 28 don't include those two bytes and start their payload sooner. Basically, extended packets begin their payload with those two addresses. AFAIK, the two addresses from the extended packets are the same as the ones used in the ordinary "classic" packets.

[tact1m4n3]

In your model, classic payload should not contain that option. Also, do all packets begin with 0xC8? Because if they do, we shouldn't really return the address(sync byte) to the user. This is what confuses me: I2C CRSF packets begin with a CRSF address.

[peterkrull]

I2c does not require the sync byte since it is not a stream serial data in the same way as UART. But if one is using I2c, and the message is being sent to a specific I2c address, also having the device address in the payload itself seems redundant?

But that is also why I assumed Payload::Classic would have the optional ExtendedFields, since otherwise how would forwarding of those packets work if we use the SYNC byte at the start?

[tact1m4n3]

I2c does not require the sync byte since it is not a stream serial data in the same way as UART.

Ok. I didn't know that about I2C. I'm just getting started with embedded programming :))

But if one is using I2c, and the message is being sent to a specific I2c address, also having the device address in the payload itself seems redundant?

I have no idea. But it does make sense to be this way.

But that is also why I assumed Payload::Classic would have the optional ExtendedFields

I am pretty sure that they don't.

I just looked at the CRSF_FRAMETYPE_DEVICE_PING docs(which is an extended packet). Those extended addresses are used to determine who that packet is addressed to and, thus, weather it should respond to the packet with CRSF_FRAMETYPE_DEVICE_INFO. So we must return those to the user :)).

Also we should probably refactor the code where we check the sync byte to only use 0xC8(and maybe 0xEE as stated in the docs).

[peterkrull]

Is passing along packets not a part of the protocol? It sounded like @anti-social #3 was working on something along those lines. I just have a hard time seeing how some packets are always non-addressed and some are always addressed. It would suggest the < 0x28 messages are always broadcast, but one would still need to known the destination of a heartbeat message, right?

We could ask over in the doc repo, though it sounds like (from some of their discussions: crsf-wg/crsf#5) they are not really an arbiter of truth either, and has no official affiliation with TBS Crossfire. Instead it sounds like a community attempt at standardizing the existing iterations of the protocol.

[tact1m4n3]

In @anti-social 's project, as far as I understand, he is just forwarding packets from the rx to the fc. So he does not need the sync byte which from the docs does not provide any info as it is only 0xC8 or 0xEE . The HEARTBEAT message contains in it's payload an origin address.

[peterkrull]

Oh.. I should have looked at the heartbeat contents.. So it must be that < 0x28 messages are broadcast only, with assumed (or sent in payload) source address, and for >= 0x28 to always have explicit source and destination?

[tact1m4n3]

Yes

[peterkrull]

Got it. Then we could enforce that invariant with something like:

pub enum Packet {
    LinkStatistics(LinkStatistics),
    RcChannels(RcChannels),
    Extended {
        src_addr: PacketAddress,
        dst_addr: PacketAddress,
        payload: ExtendedPayload,
    }
}

// Everything at or above type 0x28
enum ExtendedPayload {
    ....
}

And since the first byte is a SYNC and not an address, the enum is just the entire packet now.

[anti-social]

I propose following variant:

pub struct Packet {
    pub address: PacketAddress,
    pub payload: PacketPayload,
    pub ext_addresses: Option<PacketExtAddresses>,
}

pub enum PacketPayload {
    LinkStatistics(LinkStatistics),
    RcChannels(RcChannels),
}

pub struct PacketExtAddresses {
    pub src: PacketAddress,
    pub dst: PacketAddress,
}

[tact1m4n3]

Since the first byte(address) is does no longer have to be returned, I think @peterkrull 's variant is more appropriate. What do you think?

[anti-social]

In @anti-social 's project, as far as I understand, he is just forwarding packets from the rx to the fc. So he does not need the sync byte which from the docs does not provide any info as it is only 0xC8 or 0xEE . The HEARTBEAT message contains in it's payload an origin address.

I have a project with 2 parts:

• an air part that connects 2 receivers and chooses which one is active at the moment
• and an optional ground part that allows to connect 2 transmitters to a handset

So it is some kind of diversity when you have 2 independent radio channels and you can switch between them just on the fly.

With the ground part they use a single wire uart, so when I send a packet I also receive it. It means I should ignore packages that I sent to not fall into recursion. I came up with only filtering packets by address. It would be nice to somehow mute rx part of a uart when sending a packet but I didn't find a way how to do it with Embassy.

So I need an address of a packet to be able to understend its direction.

Possibly it should be optional?

[anti-social]

I think we should be able to configure PacketReader to expect sync (address) byte or not.

[anti-social]

It would suggest the < 0x28 messages are always broadcast

Unfortunately it is not so for communication between a handset and a transmitter. They really use different addresses for their packets and as I described before these addresses are important.

[tact1m4n3]

So I need an address of a packet to be able to understend its direction.

So the address(first byte) does change if the packet comes from the rx or fc. But this seems to be against what is said in the crsf documentation: "All serial CRSF packets begin with the CRSF SYNC byte 0xC8, with the exception of EdgeTX's outgoing channel/telemetry packets, which begin with 0xEE. Due to this, for compatibility, new code should support either for the SYNC byte, but all transmitted packets MUST begin with 0xC8.". Or maybe I don't understand it correctly, it is quite confusing

[tact1m4n3]

It would suggest the < 0x28 messages are always broadcast

Unfortunately it is not so for communication between a handset and a transmitter. They really use different addresses for their packets and as I described before these addresses are important.

Alright. Than we can use a struct for a Packet with an address field

[anti-social]

Here are examples of my code:

                            // Ignore data that we are sending to a handset
                            if data[0] != PacketAddress::Transmitter as u8 {
                                continue;
                            }

                            // ...

                            // Ignore data that we are sending to a transmitter
                            if data[0] != PacketAddress::Handset as u8 {
                                continue;

[peterkrull]

I understand the use case, but I am just not sure the protocol is designed that way, or whether some implementations just chose to modify the protocol slightly to fit their requirements. The last comment in crsf-wg/crsf#5 seems to suggest so. He also believes that TBS and Ardupilot cooperated to support crsf, so maybe that could be the closest things to a ground truth implementation?

[tact1m4n3]

https://github.com/betaflight/betaflight/blob/master/src/main/telemetry/crsf.c#L211 I've just looked in the betaflight source, and they use 0xC8 for both receiving packets and transmitting telemetry

[tact1m4n3]

It would suggest the < 0x28 messages are always broadcast

Unfortunately it is not so for communication between a handset and a transmitter. They really use different addresses for their packets and as I described before these addresses are important.

So I guess the conclusion is the handset and transmitter use that byte in their communication, but between the receiver and the flight controller that is not the case.

[tact1m4n3]

pub enum PacketAddress {
    Broadcast = 0x00,
    Usb = 0x10,
    Bluetooth = 0x12,
    TbsCorePnpPro = 0x80,
    Reserved1 = 0x8A,
    CurrentSensor = 0xC0,
    Gps = 0xC2,
    TbsBlackbox = 0xC4,
    FlightController = 0xC8,
    Reserved2 = 0xCA,
    RaceTag = 0xCC,
    Handset = 0xEA,
    Receiver = 0xEC,
    Transmitter = 0xEE,
}

This is the current PacketAddress enum according to the crsf docs. The problem that I see with matching the first byte to this enum is that we might not detect the start of the packet correctly(the sync packet 0xC8 might have been chosen from the fact that it does not appear in any packet). We can fix this by using a configurable sync byte(as @anti-social suggested): either we create a struct PacketReaderConfig that has a field which configures the packet address, or we pass the sync byte as constant generic, or something else if you have any suggestions. Also that sync byte should have nothing to do with the actual PacketAddress enum.

[anti-social]

the sync packet 0xC8 might have been chosen from the fact that it does not appear in any packet

I'm pretty sure it can be inside a packet. But it is unlikely that we'll stuck forever when parsing real traffic.

[anti-social]

We can fix this by using a configurable sync byte

I would say an array of sync bytes

[anti-social]

I am just not sure the protocol is designed that way

6 months ago there wasn't any bit of documentation about CRSF protocol (((

I think that's why developers did all these strange things

[peterkrull]

Yeah I was about to say, your use case would require the WaitingForSync step to look for multiple potential sync bytes. Maybe we could use a u16 bitflag to represent which addresses we are looking for, and have it default to just having the bit corresponding to FlightController set.

[tact1m4n3]

Also, if there are multiple possibilities for the sync byte, we should return it somehow. If there was only one there was no need for this.

[anti-social]

Just for history.

How EdgeTX creates packets when a single wire uart is used: https://github.com/EdgeTX/edgetx/blob/25bf3334ccc73d624a8dc86d4f001a24346c171c/radio/src/pulses/crossfire.cpp#L65

ExpressLRS couterpart: https://github.com/ExpressLRS/ExpressLRS/blob/57cf9d78bfb1fda6a70678044967892176cf27c4/src/lib/Handset/CRSFHandset.cpp#L148

[peterkrull]

Using bitflags we could do:

bitflags! {
    pub struct PacketAddressFlags: u16 {
        const BROADCAST = 1 << 0;
        const USB = 1 << 1;
        const BLUETOOTH = 1 << 2;
        const TBSCOREPNPPRO = 1 << 3;
        const RESERVED1 = 1 << 4;
        const CURRENTSENSOR = 1 << 5;
        const GPS = 1 << 6;
        const TBSBLACKBOX = 1 << 7;
        const FLIGHTCONTROLLER = 1 << 8;
        const RESERVED2 = 1 << 9;
        const RACETAG = 1 << 10;
        const HANDSET = 1 << 11;
        const RECEIVER = 1 << 12;
        const TRANSMITTER = 1 << 13;
    }
}

impl Default for PacketAddressFlags {
    fn default() -> Self {
        PacketAddressFlags::FLIGHTCONTROLLER
    }
}

impl PacketAddressFlags {
    pub fn is_set(&self, value: u8) -> bool {
        let flag = match value {
            0x00 => Self::BROADCAST,
            0x10 => Self::USB,
            0x12 => Self::BLUETOOTH,
            0x80 => Self::TBSCOREPNPPRO,
            0x8A => Self::RESERVED1,
            0xC0 => Self::CURRENTSENSOR,
            0xC2 => Self::GPS,
            0xC4 => Self::TBSBLACKBOX,
            0xC8 => Self::FLIGHTCONTROLLER,
            0xCA => Self::RESERVED2,
            0xCC => Self::RACETAG,
            0xEA => Self::HANDSET,
            0xEC => Self::RECEIVER,
            0xEE => Self::TRANSMITTER,
            _ => return false, // return false for unknown values
        };

        self.contains(flag)
    }
}

And then just use self.sync_flag.is_set(byte) in WaitingForSync. And yes then we would need to return the sync/address also.

[anti-social]

I think those bitflags should be an implementation detail, not a public interface.

[peterkrull]

Perhaps. Would you then want the user to supply a slice: &[PacketAddress] instead?

[anti-social]

Yes, I would. For user it should be much clear way.

[tact1m4n3]

This is what I suggest we do. Since this is not an "official" aspect of crsf, I suggest that we have a const array of allowed sync bytes for now.

pub enum Packet {
    LinkStatistics(LinkStatistics),
    RcChannels(RcChannels),
    Extended {
        src_addr: PacketAddress,
        dst_addr: PacketAddress,
        payload: ExtendedPayload,
    }
}

// Everything at or above type 0x28
enum ExtendedPayload {
    ....
}

I suggest we still use this as a packet. Also to get the sync byte, type etc. from the packet, we could add some functions to RawPacket instead.

impl RawPacket {
    pub fn sync(&self) -> u8 {
        self.buf[0]
    }

    pub fn typ(&self) -> Option<PacketType> {
        PacketType::try_from_primitive(self.buf[2]).ok()
    }

}

This would keep the library as compliant as possible with the new crsf docs :)). The thing is that most people might not care what the sync byte is and this would force them to think about something that they don't necessarily need to. What do you think of this? If you decide on the other implementation that's fine by me.

[anti-social]

All packet types CRSF_FRAMETYPE 0x28 and higher use Extended Packet Format.

The question is can another packet types be wrapped into an extended format? I don't see any prohibitions of that.

[tact1m4n3]

All packet types CRSF_FRAMETYPE 0x28 and higher use Extended Packet Format.

The question is can another packet types be wrapped into an extended format? I don't see any prohibitions of that.

I am pretty sure they cannot be wrapped. It wouldn't make much sense. Also the documentation does not say anything about this. But this does not mean there aren't some rogue implementations doing this.

[peterkrull]

That is what I was thinking too. I think It would make sense to make the "base" messages optionally addressed, especially if the first byte is never supposed to be used as an address.

And it would be pretty clear whether we are receiving an extended base message or just a base message, since the len byte is 2 larger for extended.

[anti-social]

I managed to make a generic version of PacketReader that supports optional packet address. For example RawPacket looks like:

pub struct RawPacket<'a, T> {
    addr: T,
    buf: &'a [u8; Packet::MAX_LENGTH],
    len: usize,
}

Where T type can be either () or PacketAddress.

I don't like optional variant because it forces a user to always check if an address exists.

[peterkrull]

Maybe off-topic for the issue, but on the subject of RawPacket, is there a reason for it to be a reference? I thought, why not define the RawPacket with an owned buffer:

pub struct RawPacket([u8; Packet::MAX_LENGTH]);

Which is then propagated through the the ReadState, and is not a part of the PacketReader itself:

enum ReadState {
    WaitingForSync, // the RawPacket is created here, and the sync byte is added
    WaitingForLen { raw: RawPacket },
    Reading { raw: RawPacket, idx: usize, len: usize },
}

Then once the CRC is verified, we can pass the owned RawPacket to the user, which is not limited by lifetimes, and we still only do the initial copy into the buffer, which we end up giving to the user.

This would allow us to implement an iterator (I had a hard time getting the borrow checker to let me do this otherwise), which I think could provide a quite nice interface.

pub struct IterRawPackets<'a, 'b> {
    parser: &'a mut PacketReader,
    buf: &'b [u8],
}

impl<'a, 'b> Iterator for IterRawPackets<'a, 'b> {
    type Item = Result<RawPacket, ParserError>;

    fn next(&mut self) -> Option<Self::Item> {

        match self.parser.push_bytes(self.buf) { // modified 
            (Ok(packet), remaining) => {
                self.buf = remaining;
                Some(Ok(packet))
            },
            (_, remaining) if remaining.len() == 0 => None,
            (Err(error), remaining) => {
                self.buf = remaining;
                Some(Err(error))
            }
        }
    } 
}

Which would allow the use:

let buf = [0x5A, 0x0A, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x5A];
let mut parser = PacketReader::new();

for result in parser.iter_raw_packets(&buf) {
    match result {
        Ok(packet) => {
            println!("{:?}", packet);
        },
        Err(error) => {
            println!("{:?}", error);
        }
    }
}

[anti-social]

Maybe off-topic for the issue, but on the subject of RawPacket, is there a reason for it to be a reference?

Then it will be copied when returning from a parsing function.

[peterkrull]

For my current implementation I am not implementing Copy (or Clone for that matter, for now) for RawPacket. When I return the RawPacket, I am returning the buffer the bytes were initially copied to.

[anti-social]

The bytes are moved. And to move them they should be copied.

You have a function push_bytes, it has an array of [u8; 64] on its stack. After the function finishes its stack frame is destroyed. So data that are returned should be copied on a stack frame of a function that called push_bytes.

Compiler may inline push_bytes to a call site thus eliminating copying but it depends on a lot of things.

Disclaimer. It is only my understanding how it works.

[peterkrull]

Ah yeah that sound right. But then again, we are talking about fairly low-level optimizations. I cant imaging these copies take any measurable amount of time, especially with the compiler being able to do its tricks. I just think the ergonomics of having the RawPacket without being bound by a lifetime would be nice.

[tact1m4n3]

I think the compiler would most likely optimize that out(no copy would take place). But it is not a certainty. An article about this

[peterkrull]

So even without in-lining the compiler got some tricks, neat! And even if it is forced to copy, there are instructions which can efficiently copy a larger amount of memory. Then the compiler just needs to ensure it is properly aligned. I honestly think it should be a non-issue.

[anti-social]

I2c does not require the sync byte since it is not a stream serial data in the same way as UART. But if one is using I2c, and the message is being sent to a specific I2c address, also having the device address in the payload itself seems redundant?

Do you know any examples of i2c devices that communicate each other using CRSF?

Rereading the documentation I think sync byte is mandatory.

[peterkrull]

I don't know any, and searching returns nothing apart from the docs we are referencing. Maybe we should just forget about everything relating to I2c for now, and the quirks it brings. I agree that the sync byte is mandatory for serial streams.

[tact1m4n3]

Maybe we should just forget about everything relating to I2c for now, and the quirks it brings.

I agree.

Totally unrelated, how do you guys handle a version mismatch(like a crate uses an older version of another crate than the version that I use for that other crate). I am working on an embedded project on a rpi pico using rtic and I noticed that the last release of the monotonics library uses an older version of the pac, and, therefore, I can't use it :)) I managed to fix the issue by using the github repo of rtic directly that contains an updated version of the code, not released yet.

[peterkrull]

Totally unrelated, how do you guys handle a version mismatch(like a crate uses an older version of another crate than the version that I use for that other crate). I am working on an embedded project on a rpi pico using rtic and I noticed that the last release of the monotonics library uses an older version of the pac, and, therefore, I can't use it :)) I managed to fix the issue by using the github repo of rtic directly that contains an updated version of the code, not released yet.

I just learned about this yesterday, and have never done it myself, but it might be what you want: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section