Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to choose addresses to handle #26

Closed
wants to merge 1 commit into from

Conversation

anti-social
Copy link
Contributor

No description provided.

@anti-social
Copy link
Contributor Author

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

@peterkrull
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

tact1m4n3 commented Apr 27, 2024

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

@anti-social
Copy link
Contributor Author

anti-social commented Apr 27, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

@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
Copy link
Contributor Author

In favor of #28

@anti-social anti-social closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants