API should be better aligned with NonCryptographicHashAlgorithm

[maettu-this]

Hi Ben,

CrcAlgorithm already is quite close to NonCryptographicHashAlgorithm, the base for e.g. Crc32 (https://learn.microsoft.com/en-us/dotnet/api/system.io.hashing.crc32), but there are subtle differencies:

• Width rather than HashLengthInBytes
• No support for Append(Stream)
• Clear() rather than Reset()
• No GetHashAndReset()
• ...

Aligning the API would enable deriving from NonCryptographicHashAlgorithm for .NET 6+.

Also note the remark "this stable output is the byte sequence { 0x1C, 0xDF, 0x44, 0x21 }, the Little Endian representation of 0x2144DF1C" which relates to your "TODO" (https://github.com/invertedtomato/crc/blob/master/Library/CrcAlgorithm.cs#L11). I think this "TODO" can be completed by equally documenting the output. A user can still reverse/rearrange the resulting CRC as needed.

Best regards, Matthias

[invertedtomato]

Hi Matthias. Thanks for pointing out NonCryptographicHashAlgorithm. I'm open to renaming symbols to match the pattern set by NonCryptographicHashAlgorithm, but I'm hesitant to make NonCryptographicHashAlgorithm a dependancy now as it raises the minimum supported framework to .NET 6 and adds a dependancy, both seemingly adding minimal value to it's function.

[maettu-this]

Hi Ben, I fully agree, it would just prepare such future step. You might be able to conditionally build for .NET 6+ with deriving from NonCryptographicHashAlgorithm, but keeping at least .NET Standard 2.0 builds is essential for compatibility with .NET Framework. My application actually still targets 4.8.

[maettu-this]

PS in case you missed that in https://github.com/invertedtomato/crc/pull/7:

More considerations for future refinement and extension of the library:

• Rust crc_all, at least those CRCs that are a multiple of 8
• Use of mixed-case names
• Clarifying CRC-A

And I have just come across another extensive list of CRCs:

• https://reveng.sourceforge.io/crc-catalogue/all.htm

[maettu-this]

Endianness sometimes is a beast, the "TODO" in your code isn't for nothing:

I have created https://dotnetfiddle.net/OYNy6O that uses https://learn.microsoft.com/en-us/dotnet/api/system.io.hashing.crc32. https://crccalc.com/?crc=123456789&method=CRC-32&datatype=ascii&outtype=0 results in CRC of 0xCBF43926. The fiddle results in the same.

Now, crc.GetCurrentHash() "emits the answer in the Little Endian byte order", e.g. the LSB [0] is 0x26. This sequence is the reversed of the result of your CrcAlgorithm.ToByteArray(), which is:

It took me quite a while to conclude on this. The method's summary "...the CRC of the bytes that..." doesn't help much. While it is OK to have CrcAlgorithm.ToByteArray() reverse to big-endian, I think this must be improved, as other users likely fall into the same pit. Ideas:

• Leave ToByteArray() as is, but better comment its behavior.
• Add new methods following the NonCryptographicHashAlgorithm like [GetCurrentHash](https://learn.microsoft.com/en-us/dotnet/api/system.io.hashing.noncryptographichashalgorithm.getcurrenthash?view=dotnet-plat-ext-7.0#system-io-hashing-noncryptographichashalgorithm-getcurrenthash), with equal semantic.
• Deprecate ToByteArray().

[invertedtomato]

@maettu-this , I've split the endian discussion off into another issue to stay sane. Could you please expand on what you mean by these?

• Rust crc_all, at least those CRCs that are a multiple of 8 - How do you mean? I see many non-8 multiples in that list
• Use of mixed-case names - As far as C# symbols go, I'm keen to follow the .NET conventions. But maybe you mean something else?
• Clarifying CRC-A - I did miss your comment on the other issue. No, I can't recall where this one came from. Do you happen to know more about what it could be? Happy to rename if it's named wrong, or even remove if there's little evidence of it's widespread use.
• https://reveng.sourceforge.io/crc-catalogue/all.htm - Happy to have more CRCs added, though I'm low on time currently. Would you be inspired to do this in a PR?

I'm glad for your feedback too @maettu-this!

[maettu-this]

8-bit bound algorithms covered by Rust crc_all not yet covered by InvertedTomato.Crc:

• CRC-8/AUTOSAR
• CRC-8/BLUETOOTH
• CRC-8/GSM-A
• CRC-8/GSM-B
• CRC-8/LTE
• CRC-8/NRSC-5
• CRC-8/OPENSAFETY
• CRC-8/SAE-J1850
• CRC-8/SMBUS
• CRC-8/TECH-3250

https://reveng.sourceforge.io/crc-catalogue/1-15.htm lists them as well.

• CRC-16/CMS
• CRC-16/GSM
• CRC-16/LJ1200
• CRC-16/NRSC-5
• CRC-16/OPENSAFETY-A
• CRC-16/OPENSAFETY-B
• CRC-16/PROFIBUS

https://reveng.sourceforge.io/crc-catalogue/16.htm lists them as well. Interestingly, this link also reveals "CRC-A" as "CRC-16/ISO-IEC-14443-3-A".

• CRC-24/BLE

• CRC-24/INTERLAKEN

• CRC-24/LTE-A

• CRC-24/LTE-B

• CRC-24/OS-9

• CRC-32/AUTOSAR

https://reveng.sourceforge.io/crc-catalogue/17plus.htm lists them as well. Even more:

• CRC-64/GO-ISO
• CRC-64/MS
• CRC-64/REDIS

Maybe better to extract this into a separate issue.

[maettu-this]

Ragarding "mixed-case names" I meant the string assigned to the algorithm's Name property. But maybe this isn't that good of an idea, if all resources stick the CAPITAL names.

[invertedtomato]

Yeah, using all-capital names is a stylistic decision that most seem to follow. I'm not keen to change that.

In regards to the additional CRCs, would you be interested in raising a PR to add these? I'm happy to incorporate them but I'm low on time myself

[maettu-this]

Extracted the CRC coverage to separate #13, which can then track PRs adding CRCs as people need more. Currently I am happy with those covered already, just saw that other resources list additional CRCs that people could find useful here as well.

So this issue is again reduced to the title: API should be better aligned with NonCryptographicHashAlgorithm.