Skip to content

Add thread safety to CRC class #7781

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Add thread safety to CRC class #7781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion TESTS/mbed_drivers/crc/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,41 @@ void test_any_polynomial()
}
}

void test_thread(void)
{
char test[] = "123456789";
uint32_t crc;
MbedCRC<POLY_32BIT_ANSI, 32> ct;
TEST_ASSERT_EQUAL(0, ct.compute((void *)test, strlen((const char*)test), &crc));
TEST_ASSERT_EQUAL(0xCBF43926, crc);
}

void test_thread_safety()
{
char test[] = "123456789";
uint32_t crc;

MbedCRC<POLY_16BIT_IBM, 16> ct;

TEST_ASSERT_EQUAL(0, ct.compute_partial_start(&crc));
TEST_ASSERT_EQUAL(0, ct.compute_partial((void *)&test, 4, &crc));

Thread t1(osPriorityNormal1, 320);
t1.start(callback(test_thread));
TEST_ASSERT_EQUAL(0, ct.compute_partial((void *)&test[4], 5, &crc));
TEST_ASSERT_EQUAL(0, ct.compute_partial_stop(&crc));
TEST_ASSERT_EQUAL(0xBB3D, crc);

// Wait for the thread to finish
t1.join();
}

Case cases[] = {
Case("Test supported polynomials", test_supported_polynomials),
Case("Test partial CRC", test_partial_crc),
Case("Test SD CRC polynomials", test_sd_crc),
Case("Test not supported polynomials", test_any_polynomial)
Case("Test not supported polynomials", test_any_polynomial),
Case("Test thread safety", test_thread_safety)
};

utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
Expand Down
2 changes: 2 additions & 0 deletions drivers/MbedCRC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ namespace mbed {
/** \addtogroup drivers */
/** @{*/

SingletonPtr<PlatformMutex> mbed_crc_mutex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a singleton ? It is not possible to have a mutex per MbedCRC instance ?
If the mutex is global then all crc calculations are serialized across threads; I'm not sure this is what we want.

Copy link
Author

@deepikabhavnani deepikabhavnani Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locking is needed for HW, since we will have one HW module performing CRC computation for all the instances. I understand this is an overhead for software only computations or combination of software and hardware.

We can have lock/unlock only in case of hardware, does that look like correct solution or will cause confusion?


/* Default values for different types of polynomials
*/
template<>
Expand Down
97 changes: 74 additions & 23 deletions drivers/MbedCRC.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "drivers/TableCRC.h"
#include "hal/crc_api.h"
#include "platform/mbed_assert.h"
#include "platform/SingletonPtr.h"
#include "platform/PlatformMutex.h"

/* This is invalid warning from the compiler for below section of code
if ((width < 8) && (NULL == _crc_table)) {
Expand All @@ -45,6 +47,7 @@ namespace mbed {
* ROM polynomial tables for supported polynomials (:: crc_polynomial_t) will be used for
* software CRC computation, if ROM tables are not available then CRC is computed runtime
* bit by bit for all data input.
* @note Synchronization level: Thread safe
*
* @tparam polynomial CRC polynomial value in hex
* @tparam width CRC polynomial width
Expand Down Expand Up @@ -79,21 +82,22 @@ namespace mbed {
* uint32_t crc = 0;
*
* printf("\nPolynomial = 0x%lx Width = %d \n", ct.get_polynomial(), ct.get_width());
*
* ct.compute_partial_start(&crc);
* ct.compute_partial((void *)&test, 4, &crc);
* ct.compute_partial((void *)&test[4], 5, &crc);
* ct.compute_partial_stop(&crc);
*
* printf("The CRC of data \"123456789\" is : 0x%lx\n", crc);
* return 0;
* }
* @endcode
* @ingroup drivers
*/

extern SingletonPtr<PlatformMutex> mbed_crc_mutex;

template <uint32_t polynomial = POLY_32BIT_ANSI, uint8_t width = 32>
class MbedCRC {

public:
enum CrcMode
{
Expand All @@ -104,7 +108,6 @@ class MbedCRC {
BITWISE
};

public:
typedef uint64_t crc_data_size_t;

/** Lifetime of CRC object
Expand All @@ -113,7 +116,7 @@ class MbedCRC {
* @param final_xor Final Xor value
* @param reflect_data
* @param reflect_remainder
* @note Default constructor without any arguments is valid only for supported CRC polynomials. :: crc_polynomial_t
* @note Default constructor without any arguments is valid only for supported CRC polynomials. :: crc_polynomial_t
* MbedCRC <POLY_7BIT_SD, 7> ct; --- Valid POLY_7BIT_SD
* MbedCRC <0x1021, 16> ct; --- Valid POLY_16BIT_CCITT
* MbedCRC <POLY_16BIT_CCITT, 32> ct; --- Invalid, compilation error
Expand All @@ -135,6 +138,8 @@ class MbedCRC {
}

/** Compute CRC for the data input
* Compute CRC performs the initialization, computation and collection of
* final CRC.
*
* @param buffer Data bytes
* @param size Size of data
Expand All @@ -144,52 +149,73 @@ class MbedCRC {
int32_t compute(void *buffer, crc_data_size_t size, uint32_t *crc)
{
MBED_ASSERT(crc != NULL);
int32_t status;
if (0 != (status = compute_partial_start(crc))) {
*crc = 0;
int32_t status = 0;

status = compute_partial_start(crc);
if (0 != status) {
unlock();
return status;
}
if (0 != (status = compute_partial(buffer, size, crc))) {
*crc = 0;

status = compute_partial(buffer, size, crc);
if (0 != status) {
unlock();
return status;
}
if (0 != (status = compute_partial_stop(crc))) {
*crc = 0;
return status;

status = compute_partial_stop(crc);
if (0 != status) {
*crc = 0;
}
return 0;

return status;

}

/** Compute partial CRC for the data input.
*
* CRC data if not available fully, CRC can be computed in parts with available data.
* Previous CRC output should be passed as argument to the current compute_partial call.
* @pre: Call \ref compute_partial_start to start the partial CRC calculation.
* @post: Call \ref compute_partial_stop to get the final CRC value.
*
* In case of hardware, intermediate values and states are saved by hardware and mutex
* locking is used to serialize access to hardware CRC.
*
* In case of software CRC, previous CRC output should be passed as argument to the
* current compute_partial call. Please note the intermediate CRC value is maintained by
* application and not the driver.
*
* @pre: Call `compute_partial_start` to start the partial CRC calculation.
* @post: Call `compute_partial_stop` to get the final CRC value.
*
* @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
* @note: CRC as output in compute_partial is not final CRC value, call @ref compute_partial_stop
* @note: CRC as output in compute_partial is not final CRC value, call `compute_partial_stop`
* to get final correct CRC value.
*/
int32_t compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc)
{
int32_t status = 0;

switch (_mode) {
#ifdef DEVICE_CRC
case HARDWARE:
hal_crc_compute_partial((uint8_t *)buffer, size);
*crc = 0;
return 0;
break;
#endif
case TABLE:
return table_compute_partial(buffer, size, crc);
status = table_compute_partial(buffer, size, crc);
break;
case BITWISE:
return bitwise_compute_partial(buffer, size, crc);
status = bitwise_compute_partial(buffer, size, crc);
break;
default:
status = -1;
break;
}

return -1;
return status;
}

/** Compute partial start, indicate start of partial computation
Expand All @@ -200,14 +226,15 @@ class MbedCRC {
* @param crc Initial CRC value set by the API
* @return 0 on success or a negative in case of failure
* @note: CRC is an out parameter and must be reused with compute_partial
* and compute_partial_stop without any modifications in application.
* and `compute_partial_stop` without any modifications in application.
*/
int32_t compute_partial_start(uint32_t *crc)
{
MBED_ASSERT(crc != NULL);

#ifdef DEVICE_CRC
if (_mode == HARDWARE) {
lock();
crc_mbed_config_t config;
config.polynomial = polynomial;
config.width = width;
Expand All @@ -218,7 +245,7 @@ class MbedCRC {

hal_crc_compute_partial_start(&config);
}
#endif // DEVICE_CRC
#endif

*crc = _initial_value;
return 0;
Expand All @@ -239,6 +266,7 @@ class MbedCRC {
#ifdef DEVICE_CRC
if (_mode == HARDWARE) {
*crc = hal_crc_get_result();
unlock();
return 0;
}
#endif
Expand All @@ -252,6 +280,7 @@ class MbedCRC {
} else {
*crc = (reflect_remainder(p_crc) ^ _final_xor) & get_crc_mask();
}
unlock();
return 0;
}

Expand Down Expand Up @@ -281,6 +310,28 @@ class MbedCRC {
uint32_t *_crc_table;
CrcMode _mode;

/** Acquire exclusive access to CRC hardware/software
*/
void lock()
{
#ifdef DEVICE_CRC
if (_mode == HARDWARE) {
mbed_crc_mutex->lock();
}
#endif
}

/** Release exclusive access to CRC hardware/software
*/
virtual void unlock()
{
#ifdef DEVICE_CRC
if (_mode == HARDWARE) {
mbed_crc_mutex->unlock();
}
#endif
}

/** Get the current CRC data size
*
* @return CRC data size in bytes
Expand Down