rr- / CRC-manipulator

Change CRC checksums of your files.
MIT License
91 stars 21 forks source link

Add support for CRC16XMODEM #10

Closed dave-epstein closed 8 years ago

dave-epstein commented 8 years ago

Hi,

I tried adding this on my end but it didn't work - CRC16XMODEM is identical to CRC16CCITT, except it uses a check/test value of 0x31C3 instead of 0x2189 (taken from here). I tried manually both creating a new CRC spec with this value (createCRC16XMODEM) and replacing it in crc_factories.cc (in createCRC16CCITT) before compiling to no avail, though I very well might be missing something.

It would be great if you could add support for this.

Thanks!

rr- commented 8 years ago

@csm725 please let me know if https://github.com/rr-/CRC-manipulator/commit/caad18d250d55237959d572e17439bcca414e54f is okay with you.

rr-@tornado:~/src/maintained/CRC-manipulator$ ./build/crcmanip-cli p input.txt output.txt dead -a CRC16XMODEM
Output started
Output finished
rr-@tornado:~/src/maintained/CRC-manipulator$ ./build/crcmanip-cli c output.txt -a CRC16XMODEM
DEAD
dave-epstein commented 8 years ago

@rr- Ah, looks like I forgot to add the line in the .h file. Could that be why it was behaving identically to CCITT?

Uninstalling is as simple as deleting the build folder, right?

I will confirm the changes later today when I get home but I'm pretty confident at this point that any issue would be on my end.

Thanks for the prompt response!

rr- commented 8 years ago

Could that be why it was behaving identically to CCITT?

I don't think so. Come to think of it, I've implemented it without thinking much - the test vector is actually just that: a test vector, that doesn't participate in CRC calculation and is used in tests instead E.g. the same input which as far as I recall is of form 123456789, should have CRC-CCITT of 0x2189, and CRC-XMODEM 0x31C3.

Which is why it shouldn't be surprising that the tests for my commit fail (I've committed without running them first.)

This means, if CCITT and XMODEM differ in their test vectors, they need to differ in some other way as well, so the commit needs some extra changes. I'll investigate these in detail.

dave-epstein commented 8 years ago

Oh, I see. Maybe they use a different polynomial or something? The comparison table I linked above showed no differences in their parameters. What else could differ?

I will comment here if I discover anything significant. Thanks again.

rr- commented 8 years ago

Turns out it differs from CRC16CCITT in the sense that CRC16XMODEM uses big endian, whereas CCITT uses little endian. The linked table reveals that difference in the Reversed? column: kermit == CCITT and xmodem have true and false respectively. I updated the proposed commit: 502c4701deaefb0ce1def55f50ab34825b4d95ae.

Additionally, being the first algorithm in this program to use big endian with < 32 bits, it revealed a bug in computing partial checksums where the higher non-zeroed bits could bleed their way through to the significant bits. I fixed this as well in b414aaa376815e2a96ae4d3286f55c516181f73d.

Please let me know at a later time if these changes are satisfactory.

dave-epstein commented 8 years ago

Yup, works great! Thanks very much.

rr- commented 8 years ago

Merged to master.