-
Notifications
You must be signed in to change notification settings - Fork 3k
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
CRC class implementation #5911
CRC class implementation #5911
Conversation
801650e
to
ec16b02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are differences between these 2 implementations ? Why would I choose this one instead of the earlier one? +/- ?
drivers/TableCRC.h
Outdated
/** \addtogroup drivers */ | ||
/** @{*/ | ||
|
||
extern const uint8_t _Table_CRC_7Bit_SD[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
global symbol like this starting with _
? I would just leave it without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xc0170 It is prohibited to have an identifier name starting by a single underscore and followed by an uppercase letter. These are reserved for the C and C++ library implementation and compiler intrinsic (_Bool
is an example).
drivers/MbedCRC.h
Outdated
#define MBED_CRC_API_H | ||
|
||
#include <stdint.h> | ||
#include <TableCRC.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"TableCRC.h"
drivers/MbedCRC.h
Outdated
*/ | ||
MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder); | ||
MbedCRC(); | ||
virtual ~MbedCRC() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepikabhavnani Good submission, I have few questions:
- Could you detail how hardware CRC will be supported and how it fit with the
MbedCRC
template class ? - I'm not sure to understand the polymorphic model of
MbedCRC
,compute
is virtual butinit
,deinit
and all the partial computations aren't. Could you explain howMbedCRC
should be used in polymorphic context and how it should be subclassed ? - The member variables if
MbedCRC
are set at initialization not mutated afterwards. Is there a reason why these variables are not const (or part of the template list) ?
I've also noticed that most code can be completely extracted out of the class (as plain old functions). It may be interesting to extract it to avoid code bloat.
drivers/MbedCRC.c
Outdated
|
||
#if DEVICE_CRC | ||
|
||
namespace mbed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace
is not a valid C keyword. Is it a C file or a C++ one ?
drivers/TableCRC.h
Outdated
/** \addtogroup drivers */ | ||
/** @{*/ | ||
|
||
extern const uint8_t _Table_CRC_7Bit_SD[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xc0170 It is prohibited to have an identifier name starting by a single underscore and followed by an uppercase letter. These are reserved for the C and C++ library implementation and compiler intrinsic (_Bool
is an example).
drivers/MbedCRC.h
Outdated
|
||
/* Default values for different types of polynomials | ||
*/ | ||
template <uint32_t polynomial, uint8_t width> MbedCRC<polynomial, width>::MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you split into two lines the template parameter list and the function signature ?
drivers/MbedCRC.h
Outdated
} | ||
|
||
private: | ||
uint8_t _width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that _polynomial
and _width
are required ? They are already part of the template definition.
drivers/MbedCRC.h
Outdated
* @param data final crc value | ||
* @return Reflected CRC value | ||
*/ | ||
uint32_t reflect_remainder(uint32_t data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be marked as const
.
drivers/MbedCRC.h
Outdated
* @param crc CRC value is filled in, but the value is not the final | ||
* @return 0 on success or a negative error code on failure | ||
*/ | ||
int32_t bitwise_compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be marked as const
. buffer
can be const.
drivers/MbedCRC.h
Outdated
MBED_ASSERT(crc != NULL); | ||
MBED_ASSERT(buffer != NULL); | ||
|
||
uint8_t *data = static_cast<uint8_t *>(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
drivers/MbedCRC.h
Outdated
* @return 0 on success or a negative error code on failure | ||
*/ | ||
|
||
int32_t table_compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be marked as const
. buffer
can be const.
drivers/MbedCRC.h
Outdated
*/ | ||
template <uint32_t polynomial, uint8_t width> MbedCRC<polynomial, width>::MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder): | ||
_inital_value(intial_xor), _final_xor(final_xor), _reflect_data(reflect_data), _reflect_remainder(reflect_remainder) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you initialize the class member in the constructor initialization list ?
drivers/MbedCRC.h
Outdated
* @param reflect_data | ||
* @param reflect_remainder | ||
*/ | ||
MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: intial_xor
Previous implementation had multiple classes, and support for different CRC techniques, all of which were not required. According to @sg- it will be good to have HW as default and Table based if HW does not support. Bitwise only when HW and ROM table is not present in code. Also, he suggested to have single template class and no inheritance as it was confusing and usage was not that good. First implementation can support all techniques, but little confusing for user. Second one is user friendly with basic functionality. Adding support of other techniques like HalfByteCRC will be difficult in new implementation, but on other hand HAL / HW support will be easy. |
For hardware crc, HAL should have API to check polynomial supported by specific target. In constructor, same API will be used to check is if polynomial is supported by hardware, and set the private bool member "_isHW" accordingly. In rest of the member functions, decision will be based on private member "isHW" and HAL functions will be added with proper locking. MbedCRC.c file was added for HAL, it is not required at present (missed to remove it :-( ) |
Sorry for the confusion, none of the member functions should be virtual (I missed to remove). We will have single class to support CRC and no-inheritance. Hope this answers your query. |
I am not sure if I understood this question properly, can you help me understand which member variable you are taking about? |
Whoops I should have read my sentences before submitting them. Member variables of the |
@pan- Agree all variables (passed to constructor) are constant and set once. But using them as part of template will not be user friendly and we will have hell lot of combinations for specialization case where we use ROM tables. |
ec16b02
to
23a5c21
Compare
CRC class `MbedCRC.h` is templated class created to support hardware/software CRCs. Default CRC will be hardware CRC when support for HAL is available. Polynomial tables are available for 8/16 bit CCITT, 7/16 bit for SD card and 32-bit ANSI. Polynomial table implementation will be used if Hardware CRC is not available. In case device does not have hardware CRC and polynomial table is not supported, CRC is still available and is computed runtime bit by bit for all data input.
23a5c21
to
afe8834
Compare
drivers/MbedCRC.h
Outdated
*/ | ||
MbedCRC(uint32_t initial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder); | ||
MbedCRC(); | ||
virtual ~MbedCRC() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the virtual specifier if the class is not meant to be used in a polymorphic way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the driver files have destructor and mutex as virtual functions, was not sure why so tried to be consistent with that. With HW implementation, mutex will be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification; it makes more sense if the class embeds lock/unlock functions that can be overridden.
drivers/MbedCRC.h
Outdated
* | ||
* @return 0 on success or a negative error code on failure | ||
*/ | ||
int32_t init(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this will do something when hardware CRC will be available. Is it a correct assumption ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that, it's an interface, I wouldn't like people to use it:
MbedCRC crc(...);
crc.init();
crc.compute(...);
crc.deinit();
Can't we just use constructor/destructor for (de)init code?
*/ | ||
void mbed_crc_ctor(void) const | ||
{ | ||
MBED_STATIC_ASSERT(width <= 32, "Max 32-bit CRC supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use MBED_STATIC_ASSERT
at the class scope. It is not necessary to wrap it into a specific function called in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it intentionally over there, so that we have common constructor function, where hardware CRC check and other HW CRC related stuff can be done else I will have to keep empty function. Can we move it when we add support for Hardware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Could you share your thoughts on how to integrate HW CRC into this?
drivers/MbedCRC.h
Outdated
|
||
/** Lifetime of CRC object | ||
* | ||
* @param initial_xor Inital value/seed to Xor (Default ~0x0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no default parameter for the initial_xor
and final_xor
so saying here that default is ~0x0
is confusing.
POLY_32BIT_ANSI = 0x04C11DB7, // x32+x26+x23+x22+x16+x12+x11+x10+x8+x7+x5+x4+x2+x+1 | ||
} crc_polynomial_t; | ||
|
||
/** CRC object provides CRC generation through hardware/software |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get some short example of the usage here for both compute and partial? Also could we get some worlds about algorithm used (ignoring hw for now).
drivers/MbedCRC.h
Outdated
* | ||
* @return 0 on success or a negative error code on failure | ||
*/ | ||
int32_t init(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that, it's an interface, I wouldn't like people to use it:
MbedCRC crc(...);
crc.init();
crc.compute(...);
crc.deinit();
Can't we just use constructor/destructor for (de)init code?
drivers/MbedCRC.h
Outdated
*/ | ||
int32_t compute(void *buffer, crc_data_size_t size, uint32_t *crc) | ||
{ | ||
int32_t status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert on crc being null?
drivers/MbedCRC.h
Outdated
int32_t compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc) | ||
{ | ||
if (NULL == _crc_table) { | ||
// Compute Slow CRC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that official name of the algorithm? If not can we not use slow
as it sounds bad.
drivers/MbedCRC.h
Outdated
|
||
#if defined ( __CC_ARM ) | ||
#elif defined ( __GNUC__ ) | ||
#pragma GCC diagnostic pop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment about what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am disabling warnings reported as "Shift count is negative", which is kind of invalid warning.
".\mbed-os\drivers/MbedCRC.h(231): warning: #62-D: shift count is negative
detected during instantiation of "std::int32_t mbed::MbedCRC<polynomial, width>::compute_partial_stop(std::uint32_t *) [with polynomial=79764919U, width=(std::uint8_t)' ']" at line 15 of "main.cpp""
Code
if ((width < 8) && (NULL == _crc_table)) {
p_crc = (uint32_t)(p_crc << (8 - width));
}
Compiler warns of the shift operation with width
as it is width=(std::uint8_t), but we check for ( width < 8) before performing shift, so it should not be an issue.
Do we add such detailed information in code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just add one sentence description, it will save combined hours of googling over couple of years ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's switching it off... ignore me
For hardware crc, HAL should have API to check polynomial supported by specific target. In constructor, same API will be used to check is if polynomial is supported by hardware, and set the private bool member "_isHW" accordingly. In rest of the member functions, decision will be based on private member "isHW" and HAL functions will be added with proper locking.
We can use constructor/destructor for software CRC as there is no failure case during initialization. But in case of hardware, initialization in init will help in case of failures and will be consistent with other drivers. |
drivers/TableCRC.h
Outdated
/** \addtogroup drivers */ | ||
/** @{*/ | ||
|
||
const uint8_t Table_CRC_7Bit_SD[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All definitions should go into a cpp file.
#define MBED_CRC_API_H | ||
|
||
#include <stdint.h> | ||
#include "drivers/TableCRC.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mbed_assert.h header is missing.
6fbf725
to
013b09c
Compare
013b09c
to
8e53711
Compare
how does this relates to #5669 ? Should we try to unify it (aka close one)? |
If I'm allowed to make one comment...: I think the crc table should be #ifdef'ed with a macro which tells the compiler if the current target has hw crc support ... Or else this table will just waste flash space when it is not used due to hw crc capability. (for example add 'CRC' to the 'device_has'-parts in the targets/targets.json file, which will result in a preprocessor macro "DEVICE_CRC") This should be done as soon as the HAL is specified and implemented. |
Template specialized functions are moved to cpp file, to add MbedCRC.h in mbed.h. Else linker shall multiple definition error.
c7d18a0
to
af09822
Compare
/morph build |
Build : SUCCESSBuild number : 1191 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 865 |
Test : SUCCESSBuild number : 991 |
@bulislaw Can you review after changes? |
Three-week old review is stale, and no updated comments in two weeks. Re-review requested.
Holding off on merging this PR to wait for @bulislaw's and @SenRamakri's reviews. Not critical for 5.7.6, but will likely need to be completed by End of Next Week. |
drivers/TableCRC.cpp
Outdated
0x220, 0x8225, 0x822f, 0x22a, 0x823b, 0x23e, 0x234, 0x8231, 0x8213, 0x216, 0x21c, 0x8219 | ||
}; | ||
|
||
uint32_t Table_CRC_32bit_ANSI[256] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at these tables in this code file, they need to reside in RAM (we change them - I could find that we use them for calculations but never store anything in these) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to be stored in ROM I believe they need to also be marked const
. The compiler doesn't know if they could be modified otherwise.
drivers/MbedCRC.h
Outdated
#include "drivers/TableCRC.h" | ||
#include "platform/mbed_assert.h" | ||
|
||
#if defined ( __CC_ARM ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant find a reference for these lines here. why push/pop is gere for GCC and that negative shift warning ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am disabling warnings reported as "Shift count is negative", which is kind of invalid warning.
Warning is from below code section.
https://github.com/ARMmbed/mbed-os/pull/5911/files/af0982295f010f3dd45ade36ddc9155a5f86289e#diff-a353a07f9612be80e818a21f61ec8409R210
if ((width < 8) && (NULL == _crc_table)) {
p_crc = (uint32_t)(p_crc << (8 - width));
}
Compiler warns of the shift operation with width as it is width=(std::uint8_t), but we check for ( width < 8) before performing shift, so it should not be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is useful to know, please leave either in the code short description or commit message.
drivers/MbedCRC.h
Outdated
*/ | ||
uint32_t get_crc_mask(void) const | ||
{ | ||
return (width < 8 ? ((1u << 8)-1) : (uint32_t)((uint64_t)(1ull << width) - 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
((1u << 8)-1)
spaces around -
drivers/TableCRC.cpp
Outdated
@@ -107,7 +107,7 @@ uint16_t Table_CRC_16bit_IBM[256] = { | |||
0x220, 0x8225, 0x822f, 0x22a, 0x823b, 0x23e, 0x234, 0x8231, 0x8213, 0x216, 0x21c, 0x8219 | |||
}; | |||
|
|||
uint32_t Table_CRC_32bit_ANSI[256] = { | |||
extern const uint32_t Table_CRC_32bit_ANSI[256] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern is useless here as it's definition, external linkage by default .
One question related, you have sizes in definition and declaration. You could set these to a macro, or a declaration does not need the size, thus if t his changes, no need to change the declaration.
Declaration can be: extern const uint32_t Table_CRC_32bit_ANSI[];
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external linkage is default in C, but with C++ if array constant is inside the namespace default linkage is internal.
I will replace 256 to a macro, but will keep the size in declaration as well.
3910f1f
to
b60eb1d
Compare
/morph build |
@0xc0170 - Please review the changes, and let me know if your comments are not addressed properly |
Build : SUCCESSBuild number : 1296 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 957 |
Test : SUCCESSBuild number : 1082 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since it looks like this PR has addressed all outdated comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
*/ | ||
template <uint32_t polynomial, uint8_t width> | ||
MbedCRC<polynomial, width>::MbedCRC(uint32_t initial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder): | ||
_initial_value(initial_xor), _final_xor(final_xor), _reflect_data(reflect_data), _reflect_remainder(reflect_remainder), _crc_table(NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add line break before _crc_table to align with other declarations style.
Alternate CRC implementation to #5669.
CRC class
MbedCRC.h
is templated class created to support hardware/software CRCs. Default CRC will be hardware CRC when support for HAL is available.Polynomial tables are available for 8/16 bit CCITT, 7/16 bit for SD card and 32-bit ANSI. Polynomial table implementation will be used if Hardware CRC is not available.
In case device does not have hardware CRC and polynomial table is not supported, CRC is still available and is computed runtime bit by bit for all data input.
Here is the example for new implementation. https://gist.github.com/deepikabhavnani/44737819d512c86de49ceeabf8867ef7
Default constructor without any arguments is valid only for supported CRC polynomials.
MbedCRC<POLY_7BIT_SD, 7> ct; --- Valid
MbedCRC<0x07, 7> ct; --- Valid
MbedCRC<0x07, 7> ct(a,b,c,d) -- Valid
- Note: Though inital/final/.... values are different, they do not affect the table hence ROM table will be used.MbedCRC<0x7, 8> ct; --- Invalid, compilation error
If a user passed non supported polynomial, then it will give compilation error same as we have in call backs.
For non-supported polynomials, user need to provide other constructor args as well.
MbedCRC<0x7, 8> ct(a, b, c, d); -- Valid
- Note we do not have ROM table for non-supported polynomials, hence it will be either HW or Bitwise.