tact1m4n3 / crsf-rs

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

Allow to choose addresses to handle #26

Closed anti-social closed 4 months ago

anti-social commented 4 months ago

I'm not sure should we have a constructor with all the addresses enabled?

peterkrull commented 4 months ago

I have started to wonder about this approach. Having more "sync" bytes enabled makes it more likely to start parsing prematurely, and to then miss an actual sync byte. And I feel that it might be overcomplicating the library for a fairly niche use case.

What if we just keep it at a single sync byte, but allow it to be customized? Then the user of the library is free to create multiple parsers if they need to receive different sync bytes. That would keep things simple, and would even reduce the risk of a parser missing its sync byte.

I'm not sure should we have a constructor with all the addresses enabled?

Is there even a use case where one would need to have all addresses enabled?

tact1m4n3 commented 4 months ago

I have started to wonder about this approach. Having more "sync" bytes enabled makes it more likely to start parsing prematurely, and to then miss an actual sync byte. And I feel that it might be overcomplicating the library for a fairly niche use case.

What if we just keep it at a single sync byte, but allow it to be customized? Then the user of the library is free to create multiple parsers if they need to receive different sync bytes. That would keep things simple, and would even reduce the risk of a parser missing its sync byte.

That's exactly what I was thinking about. I'd prefer this approach instead. Also if there is a single sync byte, there is no need to return it.

anti-social commented 4 months ago

I need to process traffic between a handset and a transmitter. And they use different addresses in their packets. So the only way to work with such traffic is to be able to handle multiple addresses.

tact1m4n3 commented 4 months ago

Maybe you can push the same data in two parsers? (each with his own sync byte)

anti-social commented 4 months ago

They use a single wire uart. In case of different parsers I also see possibility of invalid data. For example, if you have a parser that handles only packets that start with a specific sync byte it should skip packets with an another sync byte, but that data can contain the first sync byte. This can produce tricky bugs I think.

anti-social commented 4 months ago

I agree that we don't need parser that handles packets with any address we know about. I asked about it because it was default behavior before the PR.

tact1m4n3 commented 4 months ago

@peterkrull proposed a variant that I really like in #27. Maybe the push_bytes_with_cfg function could include a list of sync bytes not just one. And the raw packet struct can have a method that returns the sync byte

anti-social commented 4 months ago

In favor of #28