nfc-tools / libfreefare

A convenience API for NFC cards manipulations on top of libnfc.
Other
395 stars 106 forks source link

Replace the bitwise CRC calculation method with a bytewise one #107

Closed lighterowl closed 9 months ago

lighterowl commented 5 years ago

Bytewise methods for CRC calculation have much better performance, as I've recently shown in a very simple benchmark, though for a different polynomial. Here's some proof of concept code which (hopefully) proves that the results are identical.

smortex commented 4 years ago

I would prefer we do not loose how the CRC is computed because it's done in a unusual way (and the documentation is so inaccurate I was lucky to be helped by someone to get it right: feb240ee7310344aa38aee5d9cf987b578ed42ce). I know developers relied on the libfreefare code to write support libraries in other languages, I would avoid providing "magic values" to these users.

That being said, we can either have the static values bellow a comment detailing the algorithm used to build that array; or we can build the array the first time the function is called and just use the array after instead of computing each value each time.

@darconeous what do you think?

darconeous commented 4 years ago

I would add a unit test to confirm that the output is identical for all inputs between the two mechanisms, and add a comment saying that the original bit-by-bit algorithm can be found in the unit test code.

lighterowl commented 4 years ago

@darconeous I moved the original implementation to test_mad.c and provided a simple wrapper function which calls both implementations and asserts that the results are equal. If you want to actually test all input values (thank goodness it's just a 8-bit CRC!), I can of course add a test like this as well.