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

Introduces CRCs #98

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Introduces CRCs #98

merged 4 commits into from
Apr 22, 2023

Conversation

huntc
Copy link
Contributor

@huntc huntc commented Apr 4, 2023

A crc feature has been added that brings in the crc crate and a new CrcModifier flavour. This modifier flavour is similar to the COBS implementation in how it wraps another flavour.

Convenience functions for serde are also introduced.

TODO:

  • Provide serialisation
  • Provide deserialisation

@netlify
Copy link

netlify bot commented Apr 4, 2023

Deploy Preview for cute-starship-2d9c9b canceled.

Name Link
🔨 Latest commit e437a16
🔍 Latest deploy log https://app.netlify.com/sites/cute-starship-2d9c9b/deploys/64427aea3753f40008db59db

A `crc` feature has been added that brings in the `crc` crate and a new `CrcModifier` flavour. This modifier flavour is similar to the COBS implementation in how it wraps another flavour.
src/de/flavors.rs Outdated Show resolved Hide resolved
@huntc huntc marked this pull request as ready for review April 5, 2023 10:14
@huntc huntc force-pushed the crc branch 2 times, most recently from 9f7539e to dc9dece Compare April 6, 2023 08:27
Convenience functions for serde usage
optional = true

[dependencies.paste]
version = "1.0.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some help with our declarative macros.

Copy link
Owner

Choose a reason for hiding this comment

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

Would it make sense to make this optional and gate it on the crc feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, although perhaps that adds a little complexity in case we need it elsewhere?

Alternatively, let's get rid of the macros and the need for this. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit 2c9ed55 makes the paste crate optional and includes it when use-crc is enabled.

src/de/mod.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
tests/crc.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! I don't love the use of macros, but I'll play around with this in a follow-on PR before releasing.

One minor comment, then should be ready to merge.

Thanks!

@huntc
Copy link
Contributor Author

huntc commented Apr 7, 2023

Overall looks good to me! I don't love the use of macros, but I'll play around with this in a follow-on PR before releasing.

I don't like the macros either, but I just couldn't get the crc crate's generics to play. Perhaps you can?

One minor comment, then should be ready to merge.

Lemme know if you insist on paste being removed as per my reply to your comment.

Thanks!

My pleasure. This library was great to work on, as well as use! Thanks for providing it!

huntc and others added 2 commits April 8, 2023 16:22
The paste crate is included now only if `use-crc` feature is enabled. This may help improve build performance where CRCs are not required.
This uses the correct digest sizes, and adds a test for
a random CRC8 variant.
@jamesmunns
Copy link
Owner

Hey @huntc, I think e437a16 was a legitimate bug, let me know if you think it looks good now.

I did try to reduce the size of the macro (so far: only on deser), but I don't think it's actually an improvement, just "not great in other ways": huntc#1

I'm inclined to merge this as-is now, once I have your ACK on that last commit.

@jamesmunns
Copy link
Owner

Part of the challenge in my generic approach is: mrhooray/crc-rs#79.

@huntc
Copy link
Contributor Author

huntc commented Apr 21, 2023

Hey @huntc, I think e437a16 was a legitimate bug, let me know if you think it looks good now.

I did try to reduce the size of the macro (so far: only on deser), but I don't think it's actually an improvement, just "not great in other ways": huntc#1

I'm inclined to merge this as-is now, once I have your ACK on that last commit.

Thanks James. Sorry about the bug there. Good catch!

The issues you found with generalising the width appear to be the same I had.

I’d say the main aspect of this PR is that its public API is maintainable going forward. If the underlying CRC lib was to change then I think we should remain in a good place.

@jamesmunns jamesmunns merged commit a0ef8a2 into jamesmunns:main Apr 22, 2023
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.

2 participants