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

🐞Predefined Crc16.CCITT is confusing #145

Closed
cmcqueen opened this issue Mar 19, 2024 · 4 comments · Fixed by #147 or #157
Closed

🐞Predefined Crc16.CCITT is confusing #145

cmcqueen opened this issue Mar 19, 2024 · 4 comments · Fixed by #147 or #157
Labels

Comments

@cmcqueen
Copy link

Summary

The predefined Crc16.CCITT has a misleading name. Referring to CRC-16/KERMIT in the CRC catalog, which says it has alias CRC-CCITT, I would expect it to implement that. But it instead appears to implement CRC-16/XMODEM.

Reproducing the Issue

I would expect the predefined Crc16.CCITT to be equivalent to

crc_ccitt_config = crc.Configuration(width=16, polynomial=0x1021, reverse_input=True, reverse_output=True)

And so calculating the "check" value:

crc_ccitt = crc.Calculator(crc_ccitt_config)
hex(crc_ccitt.checksum(b'123456789'))
'0x2189'

But, that's not the definition of Crc16.CCITT. Rather, it appears to really be CRC-16/XMODEM:

crc_ccitt_really_xmodem_config = crc.Crc16.CCITT
crc_ccitt_really_xmodem = crc.Calculator(crc_ccitt_really_xmodem_config)
hex(crc_ccitt_really_xmodem.checksum(b'123456789'))
'0x31c3'

Documentation

It would be helpful if the documentation has a table of all the predefined CRC algorithms, listing all their configuration parameters, along with a link to the CRC catalog..

@cmcqueen cmcqueen added the bug label Mar 19, 2024
@Nicoretti
Copy link
Owner

Nicoretti commented Mar 19, 2024

Hi @cmcqueen,

Thanks for reaching out. As far as I understand CCITT-16 encompasses a range of similar configurations, not just one. I wasn't fully aware of this in the past, leading to the current approach. I will address this in the future when releasing the next major version. I'm always a bit cautious about potential breaking changes. For now, I see the best step as improving documentation to mitigate this.

You can find a basic outline of the configurations here:

Acknowledging this isn't ideal, I'm considering creating a MkDocs plugin to enhance the documentation in regard to the CRC configurations.

best
Nico

@Nicoretti
Copy link
Owner

Will be addressed in 7.0.0

@cmcqueen
Copy link
Author

cmcqueen commented Apr 12, 2024

As far as I understand CCITT-16 encompasses a range of similar configurations…

One source of confusion is, some literature refers to "the CCITT polynomial" (𝑥16 + 𝑥12 + 𝑥5 + 1; 0x1021) only caring about the polynomial itself, without regard to the other parameters that constitute a CRC algorithm. So from that point-of-view, any CRC that uses the "CCITT polynomial" might be confusingly named a CCITT CRC.

Eg Cyclic redundancy check — Polynomial representations (Wikipedia) lists various CRC polynomials including CCITT without other parameters.

Since the Catalogue of parametrised CRC algorithms exists and provides a more precise identification of CRC algorithms, it makes sense to follow its nomenclature.

Will be addressed in 7.0.0

Thanks for that. I appreciate your work to improve this.

@Nicoretti
Copy link
Owner

Hi @cmcqueen,

thanks for response, yes that's pretty much what happend to me 😅.

best
Nico

@Nicoretti Nicoretti linked a pull request Apr 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants