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

Fixed wrong type in serial CRC function #176

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

hairymnstr
Copy link
Contributor

The CRC function was calculating the wrong CRC due to a regression in the variable type.

@cguimaraes
Copy link
Member

Is this introducing any any warning or error in your setup / scenario?
This change of type was intentionally introduced by the Windows port.

@hairymnstr
Copy link
Contributor Author

Without this fix I get CRC errors when talking to zenohd over the serial transport:

[2023-03-21T07:48:37Z ERROR zenoh_link_serial::unicast] Read error on Serial link serial//dev/ttyACM0 => serial/f7c0887d-f068-4ba2-910a-b2e7deb108ee: CRC does not match Received 54CF142 Computed F82CABD3 at io/zenoh-links/zenoh-link-serial/src/unicast.rs:150.

@cguimaraes
Copy link
Member

Let me take a look then.
I want to avoid warnings on Windows as well before merging it.

In the meantime, in order to be able to accept any PR, you need to sign the Eclipse Contributor Agreement. Click on the failing test to see how.

@cguimaraes
Copy link
Member

@hairymnstr I pushed two commits on top of your PR.
Can you double check that everything still works on your side?

Upon validation, as well as a valid ECA on your side, we will be able to merge your contribution.

@hairymnstr
Copy link
Contributor Author

I've tested the code with your commits and it's working for me.

The bit with the issue was (crc >> 1) which is an arithmetic shift (i.e. sign extends in signed types).

@cguimaraes
Copy link
Member

Fixes #177 .

@cguimaraes cguimaraes merged commit 1ca97fa into eclipse-zenoh:master Mar 21, 2023
@cguimaraes cguimaraes self-requested a review March 21, 2023 13:33
Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

2 participants