Skip to content

sys/checksum: Adding three new crc16 variations#18516

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
Teufelchen1:feat/crc_fixup
Aug 31, 2022
Merged

sys/checksum: Adding three new crc16 variations#18516
miri64 merged 1 commit intoRIOT-OS:masterfrom
Teufelchen1:feat/crc_fixup

Conversation

@Teufelchen1
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 commented Aug 26, 2022

Moin! 👋

Contribution description

This PR adds three new crc16 variations:

  • ccitt-false
  • ccitt-kermit
  • mcrf4xx

The old "ccitt" was renamed to ccitt-aug as this is the "correct name" for the given implementation.
Since CRCs aren't really standardised, the names are vague, many have alias. I tried to stick close to what other software projects use (linux, pypi.org/project/crccheck, etc.).

Testing procedure

I kept the original test - duplicated & renamed it accordingly.

@Teufelchen1 Teufelchen1 requested a review from miri64 as a code owner August 26, 2022 16:00
@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 26, 2022
@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 26, 2022

The old "ccitt" was renamed to ccitt-aug as this is the "correct name" for the given implementation.

This might lead to confusion for users that use the old ccitt. Maybe, it is best to move this through proper API aliasing and deprecation, i.e. introduce new crc16_ccitt_*() inline functions that wrap around crc16_ccitt_aug_* and add a @deprecation note that this function will be removed in 2 release cycles (i.e. 2023.01 if this PR gets merged soon). That node could also include a mention that crc16_ccitt_aug_* should be used instead.

@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Aug 26, 2022
Teufelchen1
Teufelchen1 commented Aug 29, 2022
View reviewed changes
sys/include/checksum/crc16_ccitt.h Outdated
/**
* @brief Update CRC16-CCITT
* @deprecated Use @ref crc16_ccitt_false_update instead.
* Will be removed after 2023.01 release.
Copy link
Copy Markdown
Contributor Author

@Teufelchen1 Teufelchen1 Aug 29, 2022

Choose a reason for hiding this comment

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

This might lead to confusion for users that use the old ccitt. Maybe, it is best to move this through proper API aliasing and deprecation, i.e. introduce new crc16_ccitt_() inline functions that wrap around crc16_ccitt_aug_ and add a @Deprecation note that this function will be removed in 2 release cycles (i.e. 2023.01 if this PR gets merged soon). That node could also include a mention that crc16_ccitt_aug_* should be used instead.

@miri64 like this? :)

miri64
miri64 requested changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

This might lead to confusion for users that use the old ccitt. Maybe, it is best to move this through proper API aliasing and deprecation, i.e. introduce new crc16_ccitt__() inline functions that wrap around crc16_ccitt_aug__ and add a @Deprecation note that this function will be removed in 2 release cycles (i.e. 2023.01 if this PR gets merged soon). That node could also include a mention that crc16_ccitt_aug_* should be used instead.

@miri64 like this? :)

Yes, but you also could make this function also now a static inline ... around crc16_ccitt_false_update() now to make the pending deprecation clearer.

Other than that, some formatting issues remain.

miri64
miri64 reviewed Aug 29, 2022
View reviewed changes
miri64
miri64 reviewed Aug 30, 2022
View reviewed changes
sys/include/checksum/crc16_ccitt.h Outdated
Comment on lines +145 to +147
* @details polynom=0x1021
* seed=0x1d0f
* check=0xe5cc
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 30, 2022

Choose a reason for hiding this comment

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

Is this what you intend the documentation to look like?

Flat

In case you do not and my code-based proposal did not work for you (which would look like this)...

Code

... a table could also be an option:

Suggested change
* @details polynom=0x1021
* seed=0x1d0f
* check=0xe5cc
*
* Parameter | Value
* --------: | :----
* Polynom | `0x1021`
* Seed | `0x1d0f`
* Check | `0xe5cc`

Table

Copy link
Copy Markdown
Contributor Author

@Teufelchen1 Teufelchen1 Aug 30, 2022

Choose a reason for hiding this comment

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

[..] and my code-based proposal did not work for you (which would look like this)...

You proposal was great, I just forgot about it - should not have pushed last minute before heading home...
I like the table idea - I picked it up and added some other values as well. Without going to much into detail: These parameter specify / identify one particular CRC algorithm, no more naming confusion! I initially omitted them since the check is already enough but the tables were just asking for more data. 😀

Thanks for providing your insights, I valued it a lot! :)

@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Aug 30, 2022
@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 30, 2022

Please fix the issues pointed out by the static-tests. Other than that, I think this is ready for ACK now.

miri64
miri64 approved these changes Aug 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Ran the new tests on native and iotlab-m3. Found a minor nit. Feel free to address or not.

@MrKevinWeiss MrKevinWeiss removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Aug 30, 2022
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 30, 2022

Kicking this out of the CI queue until master is fixed (or blacklisted)

@MrKevinWeiss MrKevinWeiss added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 30, 2022
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 31, 2022

Rerunning now that master should pass...

@miri64 miri64 merged commit 803ff1f into RIOT-OS:master Aug 31, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@kfessel kfessel mentioned this pull request Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

Assignees

No one assigned

Labels

Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

4 participants

@Teufelchen1 @miri64 @MrKevinWeiss @maribu