-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use little endian by default #31
Comments
Thanks for this issue, |
The same result I have by default from: And arduino library bakercp/CRC32 that I want to replace by your one |
@vovagorodok (the bakercp uses a table which is probably faster) |
Please note that the current implementation is verified with
|
Little bit faster. But I see, if use lookup table for each type of CRC i takes a lot of memory.
Thanks, now I see why its implemented like that. |
An option is to create CRC32LE() |
I messed up a bit, looks that it's not related to endians. Just default parameters intarpretation. Created pull req: #32 Additional nice calculator: http://www.sunshine2k.de/coding/javascript/crc/crc_js.html |
Thanks fot the PR Expect it will take a lot of time to verify the changes do not break the working, and are an improvement. Note that this lib is used by many already as it is, |
Its because moving source code to src folder. Tried to move back but easier will be to see full new representation. Can move back from src if it help You.
Can I hep you somehow with that?
Sure. Here are two interface changes:
Trying to thing about easiest interface and I'm interesting on your preferences. |
OK
The setters are one of the unique features of the library as it allowed runtime changing of parameters. Same for the restart function, this functions were added with a purpose.
Agree, but to serve existing users I try to keep the library backwards compatible. |
From my PoV, changing of one parameter without other will create misunderstanding what CRC algorithm now we operate on. Better will push full list of parameters in the same time in order to easily determinate algorithm. But it's only my PoV, I'll change it back if you prefer.
Can you show link? I'll take a look
See it, two different methods
If params will be const than compiler optimizes better :p
That why I have pushed all planed changes in one PR (it shows that it require breaking interface a bit). |
I understand your point of view, however I prefer to keep them.
Customer specific, sorry no link.
If performance is the issue nothing beats a dedicated CRC (except ignoring :)
Really appreciated! |
Ok, I'll move it back
Even code size. Lets keep them non const Hmm, what about
|
Semantically I think the following makes sense. Reset should reset all params to default. I have been thinking about a redo/recalc function,it could be used to recalculate crc of one block within a stream. However it never made it to the library. |
If it will require additional space than maybe better to leave this functionality to the user side implementation |
Definitely, it was an scenario that did not make it into the library. Too specific and easy for the user to implement. |
Lets move discussion to PR. I have prepared some opened questions there |
I have CRC32 calculated using (android and python) libraries and it doesn't match with result. Only after:
it works.
Most of architectures (esp32/avr/arm/x86) has little endian. And for them we should use this combination.
What about using little endian by default, and allow to change in specific cases, when big endian is needed ?
The text was updated successfully, but these errors were encountered: