CRC feature implementation #5669

deepikabhavnani · 2017-12-06T20:37:48Z

CRC class implementation

CRC class BaseCRC.h is created to support hardware/software CRCs

BitwiseCRC.h contains software CRC implementation for bitwise CRC computation which will be somewhat slow compared to other techniques as bit by bit computation is performed in software.

FastCRC.h contains software CRC implementation using lookup table method. Table is generated during init step.

HalfByteCRC.h constains software implementation for CRC32 using 16-entry technique to have limited table size (64bytes) and is fast compared to bitwise CRC computation. Table is generated runtime during init call.

Usage example:
https://gist.github.com/deepikabhavnani/0dacf5572c0b4d87b8fbfbcfd9246a95

geky · 2017-12-06T22:33:14Z

features/crc/mbed_crc.h

+
+#define GET_DATA_SIZE(x) ((x) <= 8 ? 1 : ((x) <= 16 ? 2 : 4))
+#define TOP_BIT(x)       ((x) < 8 ? (1u << 7) : (1u << (x - 1)))
+#define DATA_MASK(x)     ((1u << x) - 1)


Can these defines be moved into the C++ file?

Yes, if we had any :-) . Software CRC classes are template type hence we have just header file for them. Also, defines are generic for Slow/Fast CRC and even hardware CRC driver can use these defines.

If we keep this as defines (I would do local inline functions helpers to get this would have the same result and might not pollute symbols), then use prefix as for above enums (CRC_).

There are two uses of x that are not enclosed in parentheses - could you add those?

geky · 2017-12-06T22:36:48Z

features/crc/soft/FastCRC.h

+            case CRC_7BIT_SD:
+                _polynomial = 0x9;             // x7+x3+1;
+                _width = 7;
+                break;


Why not just provide polynomial/width/initial_value/reflect_data/reflect_remainder/final_xor all as constructor parameters with reasonable defaults?

If you want to get creative, you could do something like this:

// Presets for standard 32 bit CRC #define CRC_32BIT 0x04C11DB7, 32, ~0, true, true, ~0 // Declare 32 bit CRC SlowCRC<uint32_t> crc(CRC_32BIT); // Declare non-standard 32 bit CRC SlowCRC<uint32_t> crc(0x04C11DB7, 32, 0);

Because not all of them are verified, I don't want to give impression that all CRC computations are implemented without verifying each of them. Ones which are used most are implemented and verified. Deviation from standard ones will give incorrect result.

Deviation from standard ones will give incorrect result.

They're all backed by the same algorithm though aren't they?

You could still say only the CRCs with matching defines are supported, and assert for invalid configurations in the constructor.

geky · 2017-12-06T22:40:15Z

features/crc/mbed_crc.h

+/** @{*/
+#include <stdint.h>
+
+typedef uint64_t data_size_t;


Should this be crc_data_size_t? or just crc_t?

data_size_t is ambiguous

crc_data_size_t 👍 . Will change it

geky · 2017-12-06T22:42:50Z

features/crc/mbed_crc.h

+     *
+     *  @param  buffer  Data bytes     
+     *  @param  size  Size of data
+     *  @param  crc  CRC 


Thought: you should document what the initial state of crc is expected to be.

Can I crc data without storing it all in memory at once? This is a requirement for littlefs.

Initial state of CRC is not picked from this argument, we only return computed CRC , Initial value is don;t care.
Inital XOR values for CRC, vary based on the CRC algorithm selected, which is set as part of constructor.

Do you think we could have a compute_partial or something similar to continue a CRC without initializing the value?

Partial CRC, yes we can have support for that in future. But partial CRC computation should be separate API and we will need proper locking mechanism with that. Partial CRC method is used to compute entire CRC in parts in parallel.

Oh no, not for parallel computation. Just for computing a CRC when the data being CRC'd doesn't fit in RAM. The data can be streamed in, but only if the CRC can be computed partially.

This can be another pr if you'd like, it would just be needed before this class can be adopted everywhere.

geky · 2017-12-06T22:53:23Z

features/crc/soft/FastCRC.h

+#include "mbed.h"
+
+template <typename T>
+class FastCRC : public CRC


It may be worth documenting this template parameter. I'm not entirely sure what value it adds to the class (the internal table uses T as elements? Does the resulting CRC size depend on T or the polynomial argument?)

Resulting CRC size will always be uint32_t (masked based on CRC width). Kept it generic in CRC class

Does this need to be a template then? you can extend an instantiated template:

class FastCRC : public CRC<uint32_t>

CRC class is generic and will be used for hardware CRC as well. If you check hardware CRC implementation of target devices, they all return 32-bit CRC (masked) based on polynomial. Could be because of C implementation, but in order to have consistent behavior it would be good to not have template in base class.

Any inputs from HAL team?

geky · 2017-12-06T22:57:20Z

Is there a CRC implementation that sits between FastCRC and SlowCRC in terms of ROM vs runtime?

I'm curious which CRC option best matches littlefs's:
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/littlefs/littlefs/lfs_util.c#L21-L35

Also is it safe to assume this opens up the opportunity to add hardware accelerated CRC?

deepikabhavnani · 2017-12-07T06:22:22Z

Intermediate version is to use smaller tables (16-entry table) if we have space constraints. The method which you have in littlefs.
http://create.stephan-brumme.com/crc32/

0xc0170

@deepikabhavnani We should ask to review this addition with mbedtls team. I'll tag them here

0xc0170 · 2017-12-07T07:58:19Z

features/crc/mbed_crc.h

@@ -0,0 +1,102 @@
+/* mbed Microcontroller Library


location - features/crc ? this is not part of platform folder? - just thinking why feature/ location was chosen

Current CRC use case is block device and filesystem, hence selected feature.
In future mbed_crc.h class can be used as base class for hardware CRC, hence we can have it HAL layer as well.
Inputs from core+hal team will be appreciated for this.

I would assume it should go to drivers/ straight away.

move entire CRC implementation to drivers? Or just mbed_crc.h.
Naming convention in driver folder does not have mbed prefix. Should I rename file to be CRC.h?
If everything is moved inside driver, should we have folder structure for software implementation or everything should be at same level?

I would say all CRC files and we should match the convention for the drivers and I don't think we need directory structure for it.

0xc0170 · 2017-12-07T08:01:31Z

features/crc/mbed_crc.h

+
+#define GET_DATA_SIZE(x) ((x) <= 8 ? 1 : ((x) <= 16 ? 2 : 4))
+#define TOP_BIT(x)       ((x) < 8 ? (1u << 7) : (1u << (x - 1)))
+#define DATA_MASK(x)     ((1u << x) - 1)


If we keep this as defines (I would do local inline functions helpers to get this would have the same result and might not pollute symbols), then use prefix as for above enums (CRC_).

0xc0170 · 2017-12-07T08:02:41Z

features/crc/soft/FastCRC.h

+#define MBED_FAST_CRC_H
+
+#include "mbed_crc.h"
+#include "mbed.h"


This should not be included within our code base (include everything is not an answer to what symbol is missing), please include what is needed here.

Good catch 👍

Sounds like we should have a script to catch these... 🤔

0xc0170 · 2017-12-07T08:03:12Z

features/crc/soft/FastCRC.h

+     *
+     * @return  0 on success, negative error code on failure
+     */
+    virtual int32_t deinit() {


style as in the above file { on the new line for body functions

0xc0170 · 2017-12-07T08:06:03Z

@mazimkhan @hanno-arm you might be interested in this addition, please review if that is the case

ciarmcom · 2017-12-07T08:50:31Z

ARM Internal Ref: IOTSSL-1938

deepikabhavnani · 2017-12-07T22:33:18Z

features/crc/soft/HalfByteCRC.h

+ */
+
+#ifndef MBED_HALF_BYTE_CRC_H
+#define MBED_HALF_BYTE_CRC_H


@geky - Please check if this implementation will suffice the CRC32 requirement for littlefs.
Tested with: https://gist.github.com/deepikabhavnani/ff6fc74c4150cff36d398c1d2a4e7f59

Looks good to me! Only thought is that the name HalfByteCRC is a bit confusing. What about MediumCRC? Another thought, what if we renamed SlowCRC to SmallCRC. Just tossing out ideas, it's up to you.

other terribles names:

ModestCRC

PracticalCRC

KindaFastKindaSlowCRC

BestCRC

Actually, thinking more about it. Maybe we should just leave out HalfByteCRC and force LittleFS to use FastCRC. It's probably better to have fewer options so there's more chances for the implementation to get reused.

We can also provide a config option for LittleFS to use SlowCRC (SmallCRC?) in the cases the size penalty is too much.

Thoughts?

name HalfByteCRC is a bit confusing

Naming would be good based on speed/technique.
Since we have so many different variations we can change from fast/slow to technique based.
Fast CRC - lookupTable or StandardTable CRC
Slow CRC - bitwiseCRC or tableless CRC.
HalfByteCRC - Half-byte/Nibble/16-byte entry table are different options for

Actually, thinking more about it. Maybe we should just leave out HalfByteCRC and force LittleFS to use FastCRC

We can have default as FastCRC and switch to SlowCRC option in targets.json. Selection will be one-time mostly based on target and not application. Correct?
No harm in providing half-byte option as well, we just need to finalize one default.

simonbutcher · 2017-12-11T22:09:44Z

Hi @0xc0170,

This isn't security or crypto related, so doesn't require a review from @hanno-arm or @mazimkhan, although I don't object to them reviewing it.

Could you please remove the TLS label? Thanks.

geky · 2017-12-11T22:23:21Z

features/crc/soft/HalfByteCRC.h

+        MBED_ASSERT(buffer != NULL);
+
+        uint8_t *data = static_cast<uint8_t *>(buffer);
+        uint32_t remainder = p_crc == 0x0 ? _inital_value : ~p_crc;


p_crc == 0x0 might be a valid intermediary value for the crc, this would break if that lined up with a compute_partial block. You can expect the user to either initialize it themselves, or call compute first

Asking user to set initial value will be better then calling compute first. Will update this.

geky · 2017-12-11T22:23:54Z

features/crc/soft/HalfByteCRC.h

+        uint8_t *data = static_cast<uint8_t *>(buffer);
+        uint32_t remainder = _inital_value;
+
+        for (crc_data_size_t byte = 0; byte < size; byte++) {


Can compute just call compute_partial now?

geky · 2017-12-11T22:32:40Z

features/crc/soft/FastCRC.h

+     */
+    virtual int32_t compute(void *buffer, crc_data_size_t size, uint32_t *crc)
+    {
+        MBED_ASSERT(crc != NULL);


Now that these aren't templates, could the implementations be moved back into C++ files?

FastCRC implementation is template based

geky · 2017-12-11T22:37:18Z

features/crc/soft/HalfByteCRC.h

+            remainder = _crc_table[((remainder ^ (data[byte] >> 4)) & 0x0F)] ^ (remainder >> 4);
+        }
+
+        *crc = (remainder ^ _final_xor);


XORing with _final_xor will also cause problems in partial computes. Not sure the best way to handle this. You could have compute_partial_start and compute_partial_stop for the initializing/finalizing. In LittleFS's case it's just handling the initializing/finalizing itself so you could also just get by without the initializing/finalizing for now.

SenRamakri · 2017-12-13T18:47:51Z

features/crc/soft/FastCRC.h

+private:
+    uint32_t _polynomial;
+    uint8_t _width;
+    T _crc_table[256];


Shouldn't the CRC table be same for same polynomial? In that case, have you thought of sharing the table for same polynomial types across multiple instances. That can help save lot of memory and will avoid computing the table for same polynomial type in init. May be the assumption here is to use same CRC object across the module(For E.g.: fs, block device), in that case should we protect the interfaces for thread-safety?

Having common CRC table across modules will require proper thread-safety, but I am not sure if any application will have this use case. Can however think of it in future

SenRamakri · 2017-12-13T18:48:15Z

features/crc/soft/FastCRC.h

+     *  @note CRC calculations are done byte wise.
+     */
+    virtual int32_t compute(void *buffer, data_size_t size, uint32_t *crc) {
+        MBED_ASSERT(crc != NULL);


Isn't MBED_ASSERTs removed for non-debug builds? In that case its better to return an error when you get an invalid parameter instead of working with invalid data(works for release builds).

In that case its better to return an error when you get an invalid parameter

== more code size

In this case we'll hit a hard fault either way, which is probably clear enough as to what went wrong IMO.

As of now hitting Hard Fault will put the application into an unrecoverable state, where as if we return an error the application can handle that more gracefully, like logging/reporting the error to the user. And hitting hardfault is not a scenario easily debuggable in a customer/deployed scenario and without a debugger. But I do agree that returning error code will slightly increase the code size.

But this case is a usage error. The correct way to handle this error is to not pass in NULL in the first place.

Point taken for making it easier to debug in the customer scenario.

But I have a hard time imagining the case where someone propagates error codes correctly, but passes NULL into this function.

Just food for thought, I don't think this conversation should block this pr

pan-

Would it be possible to use pre initialized look up tables for FastCRC and HalfByteCRC (in addition of existing methods) ? It would help users to deport memory cost of Fast CRC from RAM to ROM.

I'm personally not completely sold on the template + polymorphic approach taken on FastCRC and SlowCRC: In the parent class, the CRC is always passed as an uint32_t value but in the template class it is also the template type that parametrize the class; both might differ from one to another.

pan- · 2017-12-19T08:54:26Z

features/crc/mbed_crc.h

+
+/** CRC Polynomial type
+ *
+ * Differnet polynomials supported


Typo: Different

pan- · 2017-12-19T08:57:50Z

features/crc/mbed_crc.h

+/** @{*/
+#include <stdint.h>
+
+typedef uint64_t crc_data_size_t;


This may be part of the CRC class.

You mean add this define to every derived class?

In that context derived classes would have access to the typedef; no reason to define it in every class. Inside CRC is enough.

pan- · 2017-12-19T08:58:32Z

features/crc/mbed_crc.h

+/** \addtogroup crc */
+/** @{*/
+#include <stdint.h>
+


It would be nice if the CRC declarations were protected by a namespace.

pan- · 2017-12-19T09:00:32Z

features/crc/mbed_crc.h

+    CRC_32 = 32,
+} crc_width_t;
+
+#define CRC_GET_DATA_SIZE(x) ((x) <= 8 ? 1 : ((x) <= 16 ? 2 : 4))


Would it be possible to use functions rather than macros and document those ?

pan- · 2017-12-19T09:05:30Z

features/crc/mbed_crc.h

+     *
+     * @return  Polynomial value
+     */
+    virtual uint32_t get_polynomial(void) const = 0;


It may be interesting to add a typedef for the polynomial.

pan- · 2017-12-19T09:43:57Z

features/crc/mbed_crc.h

+     *  @param  crc  CRC value (intermediate CRC)
+     *  @return  0  on success or a negative error code on failure
+     */
+    virtual int32_t compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc)


buffer may be const; the function itself too.

pan- · 2017-12-19T09:51:35Z

features/crc/mbed_crc.h

+     *  This API is used to compute final chunk of CRC, else user should perform 
+     *  the final XOR operation on intermediate computed value to get correct CRC.
+     *
+     *  @param  buffer  Data bytes


buffer, size and p_crc are not part of the parameters.

pan- · 2017-12-19T09:52:01Z

features/crc/mbed_crc.h

+     *  @param  buffer  Data bytes
+     *  @param  size  Size of data
+     *  @param  p_crc  Previous CRC to continue CRC computation, should be 0 for first part
+     *  @param  crc  CRC result


Could you indicate that CRC is an in/out parameter ?

pan- · 2017-12-19T09:52:21Z

features/crc/mbed_crc.h

+     *  @param  p_crc  Previous CRC to continue CRC computation, should be 0 for first part
+     *  @param  crc  CRC result
+     */
+    virtual void compute_partial_stop(uint32_t *crc)


The function may be const.

pan- · 2017-12-19T09:59:46Z

features/crc/soft/FastCRC.h

+
+/** Templated FastCRC Class
+  *
+  *  @note typename must be consistent with polynomial length.


Wouldn't it be better to parametrize the template class on crc_polynomial_t ?
That would avoid user mistakes.

Yeah, I would assume it should.

0xc0170 · 2018-01-03T10:56:26Z

drivers/FastCRC.cpp

+    }
+}
+
+FastCRC::~FastCRC() {


{ new line

0xc0170 · 2018-01-03T10:57:24Z

drivers/FastCRC.cpp

+    deinit();
+}
+
+int32_t FastCRC::init(void)


What is the intention with init/deinit methods, instead of doing this in ctor/dtor (dtor does call deinit, but init is postponed).

Yes init is not mandatory for all techniques, we create table during init and allocate memory, which may result into error.
Also, some hardware CRCs require special init.

0xc0170 · 2018-01-03T11:00:01Z

drivers/BaseCRC.h

+     *  @param  buffer  Data bytes
+     *  @param  size  Size of data
+     *  @param  crc  CRC value is intermediate CRC value filled by API.
+     *  @return  0  on success or a negative error code on failure


Shall we define negative error codes, or -1 does it all?

-1 is the only error code in Software CRC implementation. However, I can use "Not supported" as additional error code, if we have common error codes, but nothing special at present.

1. Added default CRC case RawCRC 2. Renamed SlowCRC to BitwiseCRC class 3. Resolved doxygen errors 4. Resolved name clashes with different targets

deepikabhavnani · 2018-01-04T20:47:56Z

@bulislaw @0xc0170 @geky @pan- @SenRamakri - Please approve if all your comments are addressed. Thanks

CRC final computation and shift operation was done in compute partial API, which resulted in incorrect results for 8-bit CRC. Renamed 32-bit CRC, to 32-bit ANSI CRC to have more 32-bit CRC types in future.

0xc0170 · 2018-01-09T09:17:56Z

/morph build

mbed-ci · 2018-01-09T09:53:33Z

Build : SUCCESS

Build number : 827
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5669/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-01-09T12:06:50Z

Exporter Build : SUCCESS

Build number : 484
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5669/

mbed-ci · 2018-01-09T13:29:13Z

Test : FAILURE

Build number : 667
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5669/667

cmonr · 2018-02-01T22:44:24Z

@deepikabhavnani Will 5911 supersede this PR?

deepikabhavnani · 2018-02-01T23:13:13Z

@cmonr - yes it will. We can close this once 5911 is approved.

deepikabhavnani · 2018-02-05T16:06:00Z

Closing this, as PR #5911 will be merged after approvals

deepikabhavnani mentioned this pull request Dec 6, 2017

CRC support added PelionIoT/sd-driver#75

Merged

geky self-requested a review December 6, 2017 22:37

geky reviewed Dec 6, 2017

View reviewed changes

0xc0170 reviewed Dec 7, 2017

View reviewed changes

0xc0170 added component: security tracking labels Dec 7, 2017

0xc0170 requested review from mazimkhan and hanno-becker December 7, 2017 08:05

0xc0170 added the needs: review label Dec 7, 2017

ciarmcom added the mirrored label Dec 7, 2017

deepikabhavnani commented Dec 7, 2017

View reviewed changes

deepikabhavnani force-pushed the CRC_class branch from 213a53d to 487a8bc Compare December 7, 2017 22:48

SenRamakri requested a review from bulislaw December 8, 2017 19:06

geky reviewed Dec 11, 2017

View reviewed changes

SenRamakri reviewed Dec 13, 2017

View reviewed changes

0xc0170 removed component: security mirrored tracking labels Dec 18, 2017

pan- reviewed Dec 19, 2017

View reviewed changes

0xc0170 removed request for mazimkhan and hanno-becker December 19, 2017 10:42

deepikabhavnani force-pushed the CRC_class branch 2 times, most recently from 100b8e9 to e520d64 Compare January 2, 2018 19:39

0xc0170 reviewed Jan 3, 2018

View reviewed changes

Default Class and few fixes

64bb607

1. Added default CRC case RawCRC 2. Renamed SlowCRC to BitwiseCRC class 3. Resolved doxygen errors 4. Resolved name clashes with different targets

deepikabhavnani force-pushed the CRC_class branch from e520d64 to 64bb607 Compare January 3, 2018 16:36

deepikabhavnani added needs: CI needs: review and removed needs: work labels Jan 4, 2018

bulislaw approved these changes Jan 5, 2018

View reviewed changes

0xc0170 approved these changes Jan 5, 2018

View reviewed changes

0xc0170 added needs: review and removed needs: review needs: CI labels Jan 5, 2018

Partial computation results were incorrect for 8-bit CRC

cf6208a

CRC final computation and shift operation was done in compute partial API, which resulted in incorrect results for 8-bit CRC. Renamed 32-bit CRC, to 32-bit ANSI CRC to have more 32-bit CRC types in future.

deepikabhavnani added needs: work and removed needs: review labels Jan 11, 2018

deepikabhavnani mentioned this pull request Jan 24, 2018

CRC class implementation #5911

Merged

cmonr added the do not merge label Feb 1, 2018

deepikabhavnani closed this Feb 5, 2018

sg- removed do not merge needs: work labels Feb 5, 2018

deepikabhavnani deleted the CRC_class branch March 13, 2018 04:11

CRC feature implementation #5669

CRC feature implementation #5669

Conversation

deepikabhavnani commented Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

deepikabhavnani Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Dec 6, 2017

deepikabhavnani commented Dec 7, 2017 • edited Loading

0xc0170 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepikabhavnani Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Dec 7, 2017

ciarmcom commented Dec 7, 2017

Choose a reason for hiding this comment

geky Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepikabhavnani commented Jan 4, 2018 • edited Loading

deepikabhavnani commented Dec 6, 2017 •

edited

Loading

deepikabhavnani Dec 6, 2017 •

edited

Loading

deepikabhavnani commented Dec 7, 2017 •

edited

Loading

deepikabhavnani Dec 7, 2017 •

edited

Loading

geky Dec 11, 2017 •

edited

Loading

deepikabhavnani commented Jan 4, 2018 •

edited

Loading