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

Add: Checksum check on decoder #14

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

RaulTrombin
Copy link
Member

Actually, the checksum isn't checked on the decoder itself;
it's only verified when you attempt to build a ProtocolMessage from it.
PR propose validating it upon receipt and returning an error promptly.

Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question -- do we want to prohibit access to the message when the CRC is wrong, or do we want to allow the user to access it even if the CRC is wrong?

@patrickelectric
Copy link
Member

A question -- do we want to prohibit access to the message when the CRC is wrong, or do we want to allow the user to access it even if the CRC is wrong?

I think that would be better for us return the message inside the error enum

@RaulTrombin
Copy link
Member Author

RaulTrombin commented Apr 4, 2024

A question -- do we want to prohibit access to the message when the CRC is wrong, or do we want to allow the user to access it even if the CRC is wrong?

I think that would be better for us return the message inside the error enum

@joaoantoniocardoso humm, then would be possible try to recover the msg,
we can do something like @patrickelectric suggested, should I modify this PR for it?

@patrickelectric
Copy link
Member

A question -- do we want to prohibit access to the message when the CRC is wrong, or do we want to allow the user to access it even if the CRC is wrong?

I think that would be better for us return the message inside the error enum

@joaoantoniocardoso humm, then would be possible try to recover the msg, we can do something like @patrickelectric suggested, should I modify this PR for it?

You only need to update ChecksumError declaration

@RaulTrombin
Copy link
Member Author

@joaoantoniocardoso thks for input.
Modief ParseError::CheksumError to contain ProtocolMessage as Success does.

src/decoder.rs Outdated Show resolved Hide resolved
@patrickelectric patrickelectric merged commit e86ff5e into bluerobotics:master Apr 4, 2024
2 checks passed
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