Reused parameters and algorithms #32

vovagorodok · 2023-05-08T21:33:02Z

What was changed:

Moved source code to src folder
Created CrcParameters.h where all CRC parameters will be stored instead only polynomes. Reused this param constants everywhere.
CRC32 class and crc32 func now use default CRC32 params
Created specific yieldAdd() methods and yieldCrc..() functions instead yeald by default
crc..() functions now use the same codebase as classes
Created FastCRC32 based on lookup table for default CRC32

Proposed to change:

Rename getCRC() methods to calc() or calculate()
Rename crc..() functions to calcCRC..()

test/unit_test_crc16.cpp

This reverts commit 5cf7fa1.

RobTillaart · 2023-05-09T09:17:17Z

Thanks for these explanations, I'll try to review this week.

RobTillaart · 2023-05-09T09:20:00Z

Changing default behavior is probably the cause of the fail of the unit test
Please be aware that existing projects depend on earlier defaults.

vovagorodok · 2023-05-09T09:29:03Z

Fixed CRC64 crc(); was interpreted as function

Changing default behavior is probably the cause of the fail of the unit test
Please be aware that existing projects depend on earlier defaults.

Defaults are not the same only for CRC32, for other CRC types defaults were correct

RobTillaart · 2023-06-05T10:43:14Z

Found a few hours to do a comparison test with ESP32 and UNO using the CRC_performance.ino example.
(IDE 1.8.19)

For ESP32 things look just fine, UNO (AVR) pays a serious penalty.
Opinion?

Arduino UNO (16 MHz)

Compile size

Version	Size / vars
0.3.3	5184 / 512
1.0.0	6018 / 512
Diff	830 / 0

The 1.0.0 version has a larger footprint (for UNO this is substantial).
As the sketch uses 4 CRC's it is not representative, but even 1/4 == 200+
bytes is quite a lot for an UNO.

Runtimes

algo	output 0.3.3	output 1.0.0	time 0.3.3	time 1.0.0	delta %
CRC8	2	2	412	800	+94%
CRC8	3E	3E	580	964	+66%
CRC16	6E99	6E99	1016	1120	+10%
CRC16	7480	7480	1208	1288	+07%
CRC32	50FD78E4	50FD78E4	760	1824	+140%
CRC32	1D73421	1D73421	992	2004	+102%
CRC64	BFF3AA1C182ABDB1	BFF3AA1C182ABDB1	5084	7380	+45%
CRC64	451DF73FA9BFFA5A	451DF73FA9BFFA5A	5176	7608	+47%

Output is identical (as should be).
Timing is worse than expected.
(CRC64 is imho less important, but CRC8 and CRC32 are)

ESP32 (240 MHz)

Compile size

less interesting as sufficient RAM.

Runtimes

algo	output 0.3.3	output 1.0.0	time 0.3.3	time 1.0.0	delta %
CRC8	2	2	66	65	-01%
CRC8	3E	3E	61	61	+00%
CRC16	6E99	6E99	61	61	+00%
CRC16	7480	7480	66	66	+00%
CRC32	50FD78E4	50FD78E4	63	63	+00%
CRC32	1D73421	1D73421	67	66	-01%
CRC64	BFF3AA1C182ABDB1	BFF3AA1C182ABDB1	81	80	-01%
CRC64	451DF73FA9BFFA5A	451DF73FA9BFFA5A	91	91	+00%

Output is identical (as should be).
Timing is identical or a minor gain.

vovagorodok · 2023-06-05T11:22:49Z

Hmm, only one serious difference I see. Perhaps it's an issue. In 1.0.0 we use common type for length/size size_t (perhaps uint32_t everywhere). Before was uint16_t. Perhaps for UNO (8bit architecture) is hard to calculate 32bit values.

What if use custom type for size:

#if defined(CRC_CUSTOM_SIZE)
using crc_size_t = CRC_CUSTOM_SIZE;
#elif defined(__AVR__)
using crc_size_t = uint16_t;
#else
using crc_size_t = size_t;
#endif

?

vovagorodok · 2023-06-05T11:30:06Z

For binary size of UNO perhaps compiler/linker for avr doesn't optimize code as gcc for esp32. We can remove FastCRC32 and check if binary size become less

RobTillaart · 2023-06-05T11:44:00Z

For binary size of UNO perhaps compiler/linker for avr doesn't optimize code as gcc for esp32. We can remove FastCRC32 and check if binary size become less

Manually removed the FastCRC32 gave the same binary size. So that works well.

No time today to do the size_t test, maybe later this week.
Yes AVR is 16 bit intern, so yes it can be the cause.

vovagorodok · 2023-06-05T11:48:09Z

Let's check it. I'll prepare commit with crc_size_t today

vovagorodok · 2023-06-05T11:55:54Z

Manually removed the FastCRC32 gave the same binary size. So that works well.

I see. We left a lot of deprecated functions and methods:

[[deprecated("Use reverse8bits() instead")]] uint8_t reverse8(uint8_t in);
[[deprecated("Use reverse12bits() instead")]] uint16_t reverse16(uint16_t in);
[[deprecated("Use reverse16bits() instead")]] uint16_t reverse12(uint16_t in);
[[deprecated("Use reverse32bits() instead")]] uint32_t reverse32(uint32_t in);
[[deprecated("Use reverse64bits() instead")]] uint64_t reverse64(uint64_t in);

[[deprecated("Use calcCRC8() instead")]]
uint8_t crc8(...)

and for 12, 16..

  [[deprecated("Use calc() instead")]]
  uint64_t getCRC() const;
  [[deprecated("Use setInitial() instead")]]
  void setStartXOR(uint64_t initial) { _initial = initial; }
  [[deprecated("Use setXorOut() instead")]]
  void setEndXOR(uint64_t xorOut) { _xorOut = xorOut; }
  [[deprecated("Use getInitial() instead")]]
  uint64_t getStartXOR() const { return _initial; }
  [[deprecated("Use getXorOut() instead")]]
  uint64_t getEndXOR() const { return _xorOut; }
  [[deprecated("Use add() with yieldPeriod instead")]]
  void enableYield() const {}
  [[deprecated("Use add() without yieldPeriod instead")]]
  void disableYield() const {}

and for 12, 16..

That why binary size is higher. After removing them I think binary size should be less even than 0.3.3, because we reuse a lot of code in 1.0.0

vovagorodok · 2023-06-05T12:16:03Z

In stddef.h of UNO:

#ifndef __SIZE_TYPE__
#define __SIZE_TYPE__ long unsigned int
#endif
#if !(defined (__GNUG__) && defined (size_t))
typedef __SIZE_TYPE__ size_t;

RobTillaart · 2023-06-05T12:54:34Z

Quick test in my coffee break, disabled the deprecated functions and patched the test

6018 ==> 5894 versus 5184 (0.3.3)

vovagorodok · 2023-06-05T13:28:12Z

I think that for CRC.h will be a bit higher because we use algorithms from classes inside functions and class is linked when we use a function.

Binary size will be lower than 0.3.3 when we use functions and classes.

For this moment cant find solution to not duplicate code and not reuse classes. Have an idea?

vovagorodok · 2023-06-05T13:58:27Z

Not sure if we should consider CRC_reference_test result for that, because it uses all possible CRC functions in one test (binary). For CRC32_test binary size is 2450 (library overhaed ~2000 bytes) for ver 1.0.0. CRC32_test for ver 0.3.3 has binary size 2708

This reverts commit 5b48a07.

RobTillaart · 2023-06-06T09:09:17Z

Quick check latest version => CRC16 has improved a lot, others are (nearly)identical to previous 1.0.0

Good step forward

CRC_performance.ino
Calculating CRC of 100 bytes message

CRC8:	2
TIME:	800
CRC8:	3E
TIME:	964
=============================
CRC16:	6E99
TIME:	552     (was 1120)
CRC16:	7480
TIME:	724    (was 1288)
=============================
CRC32:	50FD78E4
TIME:	1820
CRC32:	1D73421
TIME:	2004
=============================
CRC64:	BFF3AA1C182ABDB1
TIME:	7384
CRC64:	451DF73FA9BFFA5A
TIME:	7616
=============================

vovagorodok · 2023-06-06T09:34:22Z

Next possible issue that I see:

void CRC32::add(const uint8_t *array, crc_size_t length)
{
  while (length--)
  {
    add(*array++);
  }
}

We call add() each time instead of direct calculations in 0.3.3. Method calling can cost

Maybe change:

void CRC32::add(uint8_t value)
{
  _count++;

  if (_reverseIn) value = reverse8bits(value);
  _crc ^= ((uint32_t)value) << 24;
  for (uint8_t i = 8; i; i--) 
  {
    if (_crc & (1UL << 31))
    {
      _crc <<= 1;
      _crc ^= _polynome;
    }
    else
    {
      _crc <<= 1;
    }
  }
}

as inline function?

vovagorodok · 2023-06-06T09:56:26Z

Lets prepare small check before. Changed reverse8bits as inline

This reverts commit 12abf47.

vovagorodok · 2023-06-24T13:09:43Z

I bought a board in order to check. Changing functions to inline doesn't help.
But in function uint32_t calcCRC32(...) when was changed from:

CRC32 crc(polynome, initial, xorOut, reverseIn, reverseOut);
crc.add(array, length);
return crc.calc();

to:

CRC32 crc(polynome, initial, xorOut, reverseIn, reverseOut);
while (length--)
{
  crc.add(*array++);
}
return crc.calc();

calculation time was decreased from:

=============================
CRC32:	50FD78E4
TIME:	1820
CRC32:	1D73421
TIME:	2004

to:

=============================
CRC32:  50FD78E4
TIME:   776
CRC32:  1D73421
TIME:   948

Hmm, crc.add(array, length); does the same job (only while with length decrementation). Don't know what happens for this moment. AVR compiler issue?

RobTillaart · 2023-06-24T13:20:25Z

Could be an AVR issue,
I expect to (squash and) merge your work coming weeks, there might be some loose ends but those can be handled separately.
Merging will get more feedback from other users, which is important.

RobTillaart · 2023-07-12T17:18:29Z

Lots of work piles up, so sorry this one does not get attention.
I might do an update to move the sources to a src folder.
That allows an easier compare of the sum of all your commits against the last version.

tsiselsk added 21 commits May 6, 2023 14:29

Changed folders structure

245e414

Rename fefault polinomes defines

b79fad5

Simplify CRC clases and reusing functions

409340b

Added yield functions

3bd8f07

Align tests

4f65264

Docs aligment

8dcd66e

Added YieldCRC32

6822fbd

New yield methods

07236a6

Added FastCRC32

5952740

Added more crc8 params

66219fc

Added more crc16 params

609447a

Added more crc32 params

3a05f41

Added more crc64 params

6081255

Added more crc64 params

28a6b8e

Fast reverse renaming

03b467d

Removed CRC16-CCITT from docs

2521d22

Change README

239cca8

Update keywords

d39a6a0

Added one more online calc

d821800

Text aligment

f807853

Rename to nonReversedCrc

203454a

vovagorodok mentioned this pull request May 8, 2023

Use little endian by default #31

Closed

Added EOF

f5bef02

vovagorodok commented May 8, 2023

View reviewed changes

test/unit_test_crc16.cpp Show resolved Hide resolved

tsiselsk added 2 commits May 9, 2023 10:44

Moved back from src

5cf7fa1

Revert "Moved back from src"

958b714

This reverts commit 5cf7fa1.

Fixed CRC64_test.ino

d9a4f61

Added crc_size_t in order to check performance

9d46901

Moved headers to cpp file

92b7186

Include test fix

5b48a07

vovagorodok added 4 commits June 5, 2023 16:08

Revert "Include test fix"

e0388b3

This reverts commit 5b48a07.

Moved headers back to h file

cbdeeed

Alligned header of CRC.h

f576849

Include test fix

47778ee

FastReverse functions as inline in order to check performance

12abf47

vovagorodok added 3 commits June 24, 2023 13:47

Added arduino nano

2fc2a9f

Revert "FastReverse functions as inline in order to check performance"

e526776

This reverts commit 12abf47.

Added monitor speed

c5149f3

Added private _add() method without conunt incrementation

bc0f1ac

RobTillaart merged commit 5d00a55 into RobTillaart:master Jul 13, 2023

vovagorodok mentioned this pull request Jul 24, 2023

Change CRC library vovagorodok/ArduinoBleOTA#11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reused parameters and algorithms #32

Reused parameters and algorithms #32

vovagorodok commented May 8, 2023 •

edited

Loading

RobTillaart commented May 9, 2023

RobTillaart commented May 9, 2023

vovagorodok commented May 9, 2023 •

edited

Loading

RobTillaart commented Jun 5, 2023

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023

RobTillaart commented Jun 5, 2023

vovagorodok commented Jun 5, 2023

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023

RobTillaart commented Jun 5, 2023

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023 •

edited

Loading

RobTillaart commented Jun 6, 2023

vovagorodok commented Jun 6, 2023 •

edited

Loading

vovagorodok commented Jun 6, 2023

vovagorodok commented Jun 24, 2023 •

edited

Loading

RobTillaart commented Jun 24, 2023

RobTillaart commented Jul 12, 2023 •

edited

Loading

Reused parameters and algorithms #32

Reused parameters and algorithms #32

Conversation

vovagorodok commented May 8, 2023 • edited Loading

RobTillaart commented May 9, 2023

RobTillaart commented May 9, 2023

vovagorodok commented May 9, 2023 • edited Loading

RobTillaart commented Jun 5, 2023

Arduino UNO (16 MHz)

Compile size

Runtimes

ESP32 (240 MHz)

Compile size

Runtimes

vovagorodok commented Jun 5, 2023 • edited Loading

vovagorodok commented Jun 5, 2023

RobTillaart commented Jun 5, 2023

vovagorodok commented Jun 5, 2023

vovagorodok commented Jun 5, 2023 • edited Loading

vovagorodok commented Jun 5, 2023

RobTillaart commented Jun 5, 2023

vovagorodok commented Jun 5, 2023 • edited Loading

vovagorodok commented Jun 5, 2023 • edited Loading

RobTillaart commented Jun 6, 2023

vovagorodok commented Jun 6, 2023 • edited Loading

vovagorodok commented Jun 6, 2023

vovagorodok commented Jun 24, 2023 • edited Loading

RobTillaart commented Jun 24, 2023

RobTillaart commented Jul 12, 2023 • edited Loading

vovagorodok commented May 8, 2023 •

edited

Loading

vovagorodok commented May 9, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 5, 2023 •

edited

Loading

vovagorodok commented Jun 6, 2023 •

edited

Loading

vovagorodok commented Jun 24, 2023 •

edited

Loading

RobTillaart commented Jul 12, 2023 •

edited

Loading