Use little endian by default

[vovagorodok]

Use little endian by default

[RobTillaart]

Thanks for this issue,
Will look into this asap

[vovagorodok]

The same result I have by default from:
Pyhon zlib: https://github.com/vovagorodok/ArduinoBleOTA/blob/b2a0dec00a5e516ad5dc97f9790dfc3db99f39a7/tools/uploader.py#LL166C40-L166C40
Dart archive_io: https://github.com/vovagorodok/ble_ota_app/blob/0116ef0dd9d70739418b245d00d90b0965359929/lib/src/ble/ble_uploader.dart#L132

And arduino library bakercp/CRC32 that I want to replace by your one

[RobTillaart]

@vovagorodok
Is performance an issue?

(the bakercp uses a table which is probably faster)

[RobTillaart]

Please note that the current implementation is verified with

• http://zorc.breitbandkatze.de/crc.html - online CRC calculator (any base up to 64 is supported.)
• https://crccalc.com/ - online CRC calculator to verify.
• https://www.lddgo.net/en/encrypt/crc - online CRC calculator

[vovagorodok]

Is performance an issue?
(the bakercp uses a table which is probably faster)

Little bit faster. But I see, if use lookup table for each type of CRC i takes a lot of memory.

Please note that the current implementation is verified with

Thanks, now I see why its implemented like that.
Give me some time to think about it and maybe propose you some possible improvements

[RobTillaart]

An option is to create CRC32LE()
An optimized little endian version,
No need to reverse every input / output.
Just a thought.

[vovagorodok]

An optimized little endian version,

I messed up a bit, looks that it's not related to endians. Just default parameters intarpretation.
I think that CRC32 can use it default params.

Created pull req: #32
Sorry for this huge change. Decided to do it once. I made it carefully with checkig everything after each step.
Please teke a look and let me know if its ok, or should I restore something back.
Your library has very big potential and will be nice to have v1.0.0

Additional nice calculator: http://www.sunshine2k.de/coding/javascript/crc/crc_js.html

[RobTillaart]

Thanks fot the PR
This looks like a HUGE pr with large changes. (Github doesnt show them anymore by default). So I did not look into them any further for now.

Expect it will take a lot of time to verify the changes do not break the working, and are an improvement.
Therefor I have to think about how and when to proceed.

Note that this lib is used by many already as it is,

[vovagorodok]

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.
Most of changes are reusing magic numbers and algorithms.

Expect it will take a lot of time to verify the changes do not break the working, and are an improvement.
Therefor I have to think about how and when to proceed.

Can I hep you somehow with that?

Note that this lib is used by many already as it is,

Sure. Here are two interface changes:

1. Removed setters/getters and using constructor with default params and initialization list instead
2. Removing yield functionality from default and created separate methods/functions for them

Trying to thing about easiest interface and I'm interesting on your preferences.
If interfaces will be changed, than ver should be increased a lot in order to indicate it.

[RobTillaart]

Its because moving source code to src folder.

OK

Removed setters/getters and using constructor with default params and initialization list instead

The setters are one of the unique features of the library as it allowed runtime changing of parameters.
I have at least one application that needs them.

Same for the restart function, this functions were added with a purpose.
If you do not use them the compiler optimizes them away.

If interfaces will be changed, than ver should be increased a lot in order to indicate it.

Agree, but to serve existing users I try to keep the library backwards compatible.
Your proposal, and I really appreciate all the improvements, breaks this compatibility.

[vovagorodok]

The setters are one of the unique features of the library as it allowed runtime changing of parameters.

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.

I have at least one application that needs them.

Can you show link? I'll take a look

Same for the restart function, this functions were added with a purpose.

See it, two different methods reset and restart are added because of setters/getters. This two methods provide misunderstanding what each is for (maybe better resetParameters?). In this PR author doesn't recognize the difference https://github.com/vovagorodok/ArduinoBleOTA/pull/11/files

If you do not use them the compiler optimizes them away.

If params will be const than compiler optimizes better :p

Agree, but to serve existing users I try to keep the library backwards compatible.
Your proposal, and I really appreciate all the improvements, breaks this compatibility.

That why I have pushed all planed changes in one PR (it shows that it require breaking interface a bit).
I know that breaking interface will impact on existing projects, that why we should discuss all advantages carefully before/if changing. If after discussions we will decide break interfaces than users will be indicated by never version changing to (for example) v.0.4.0 (than PlatformIO will stay older projects on 0.3.3).
No pressure, decisions are on your side. I'm only try do my best to help with improvements and show you some advantages and disadvantages of different solutions. But personally prefer to change interfaces a bit :)

[RobTillaart]

But it's only my PoV, I'll change it back if you prefer.

I understand your point of view, however I prefer to keep them.

I have at least one application that needs them.
Can you show link? I'll take a look

Customer specific, sorry no link.

If params will be const than compiler optimizes better :p

If performance is the issue nothing beats a dedicated CRC (except ignoring :)

... do my best to help with improvements and show you some advantages and disadvantages of different solutions.

Really appreciated!

[vovagorodok]

I understand your point of view, however I prefer to keep them.

Ok, I'll move it back

If performance is the issue nothing beats a dedicated CRC (except ignoring :)

Even code size. Lets keep them non const

Hmm, what about reset and restart? Move them back? What about changing reset to something like:

void reset(
    uint32_t polynome = CRC32_POLYNOME,
    uint32_t initial  = CRC32_INITIAL,
    uint32_t xorOut   = CRC32_XOR_OUT,
    bool reverseIn    = CRC32_REF_IN,
    bool reverseOut   = CRC32_REF_OUT);

[RobTillaart]

Semantically I think the following makes sense.

Reset should reset all params to default.
Restart should only reset the initial value.

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.
Users can save the params from start of a block quite easily and redo a block.

[vovagorodok]

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.
Users can save the params from start of a block quite easily and redo a block.

If it will require additional space than maybe better to leave this functionality to the user side implementation

[RobTillaart]

Definitely, it was an scenario that did not make it into the library. Too specific and easy for the user to implement.

[vovagorodok]

Lets move discussion to PR. I have prepared some opened questions there