tact1m4n3 / crsf-rs

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

Access to raw data when processing packets #3

Closed anti-social closed 6 months ago

anti-social commented 6 months ago

I'm writing some kind of proxy for a CRSF protocol. I need to analyze packets and then send them further. So I suggest either next_packet would return a tuple with Packet and some raw data struct (which wraps [u8; 64] and a packet length) or it should return only raw data and then user will call Packet::parse. Possibly you could suggest another way how to do that.

anti-social commented 6 months ago

Possibly we could have another method next_packet_data to keep backward compatibility.

tact1m4n3 commented 6 months ago

Interesting project :)). Hmm. I think it's better that we use a separate function for returning raw data because most people might not need this.

tact1m4n3 commented 6 months ago

Also, should the Packet struct parse method take in as parameter a raw data struct? Or should we not use a struct at all for the return value of the next_packet_raw function?

tact1m4n3 commented 6 months ago

`#[derive(Debug, Clone)] pub struct RawPacket { data: [u8; Packet::MAX_LENGTH], len: usize, }

impl RawPacket { pub fn data(&self) -> &[u8] { &self.data[..self.len] } }` it could look something like this

tact1m4n3 commented 6 months ago

Another way of accomplishing this would be to create a serialization function for each packet(also we should add the remaining packets from the elrs docs). And in your case you could just serialize the packet and send it. I wanted to add packet serialization anyway. Would this fit your use case? If not, we could add both :))

anti-social commented 6 months ago

Also, should the Packet struct parse method take in as parameter a raw data struct? Or should we not use a struct at all for the return value of the next_packet_raw function?

I also prefer a struct. As you mentioned it can have a method that return a data slice. So it is less error prone approach.

anti-social commented 6 months ago

Another way of accomplishing this would be to create a serialization function for each packet(also we should add the remaining packets from the elrs docs). And in your case you could just serialize the packet and send it. I wanted to add packet serialization anyway. Would this fit your use case? If not, we could add both :))

We definitely need a serialization method for a parsed packet. But at the moment there are a lot of unimplemented packet types. Also I don't need to modify packets so passing raw data should be slightly faster.

anti-social commented 6 months ago

Also I think we need a way to clear a buffer. I've seen betaflight does that after some period of inactivity: https://github.com/betaflight/betaflight/blob/c6250fee6e5e9833cfcc4f6e8f8541ff67ca203a/src/main/rx/crsf.c#L371

anti-social commented 6 months ago

By the way I think we should have ability to distinguish when there is no packets or we just cannot parse data. I mean this code inside a next_packet method:

            if let Some(packet) = Packet::parse(&data[..len]) {
                break Some(packet);
            }

Possibly there should be some Unknown variant for Packet enum.

anti-social commented 6 months ago

Either we should return an error when we cannot parse a packet (or validation is failed) so the next_packet method should return Option<Result<Packet, Error>>

tact1m4n3 commented 6 months ago

Definitely. Then I suggest that we move the packet validation method since next packet raw should return a valid packet as well. I don’t really know where to put it tho. I also thought of having a raw packet parse method that would take in the raw packet struct(as that would be already validated). Also I plan to implement the rest of the packet types soon.

tact1m4n3 commented 6 months ago

Also I think we need a way to clear a buffer. I've seen betaflight does that after some period of inactivity: https://github.com/betaflight/betaflight/blob/c6250fee6e5e9833cfcc4f6e8f8541ff67ca203a/src/main/rx/crsf.c#L371

Sure

tact1m4n3 commented 6 months ago

Also, regarding your project, how are you planning to implement the proxy and what are you going to use it for? :))

anti-social commented 6 months ago

I want 2 pilots could share a copter. Of course at the specific time only one pilot can control the copter. When another pilot switches some designated aux channel they take control.

To do that I've assembled an stm32 controller with 2 receivers connected to it. The controller tracks state of both switches and chooses which input should be proxied to an FC.

tact1m4n3 commented 6 months ago

I want 2 pilots could share a copter. Of course at the specific time only one pilot can control the copter. When another pilot switches some designated aux channel they take control.

To do that I've assembled an stm32 controller with 2 receivers connected to it. The controller tracks state of both switches and chooses which input should be proxied to an FC.

Nice. Good luck with it.

anti-social commented 6 months ago

Here is the project: https://github.com/anti-social/radio-switcher/blob/main/src/main.rs