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

Add MemoryPool test. #4936

Merged
merged 1 commit into from Oct 30, 2017

Conversation

Projects
None yet
8 participants
@mprse
Member

mprse commented Aug 18, 2017

Description

Add test for MemoryPool.

Status

READY

Migrations

No

@bulislaw

There's couple of indentation errors. Also please split the PR into two commits: fixes to the original header and the tests.

rtos/MemoryPool.h Outdated
@@ -92,7 +96,7 @@ class MemoryPool : private mbed::NonCopyable<MemoryPool<T, pool_sz> > {
private:
osMemoryPoolId_t _id;
osMemoryPoolAttr_t _attr;
char _pool_mem[sizeof(T) * pool_sz];
char _pool_mem[ ((sizeof(T) + 3) & ~3) * pool_sz];

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

Can we have a short comment why we are doing that.

rtos/MemoryPool.h Outdated
@@ -83,7 +84,10 @@ class MemoryPool : private mbed::NonCopyable<MemoryPool<T, pool_sz> > {
/** Free a memory block.
@param block address of the allocated memory block to be freed.
@return status code that indicates the execution status of the function.
@return osOK on successful deallocation, osErrorParameter if parameter mp_id

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

what is mb_id in our case? From the perspective of C++ API it doesn't exist. I don't think we should be quoting it, instead we could say something like internal error for both.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
return verbose_test_setup_handler(number_of_cases);
}
Case cases[] = {

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

Please fix the indentation.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
* When free operation is executed on this block again.
* Then operation fails with osErrorResource status.
*/
template<typename T, const uint32_t numOfEntries, AllocType atype>

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

That's way too long can you split it in multiple tests.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
* When free operation is executed on this block again.
* Then operation fails with osErrorResource status.
*/
template<typename T, const uint32_t numOfEntries, AllocType atype>

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

that's way too long can you split it in multiple small tests.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
* Then operation fails with osErrorResource status.
*
*/
template<typename T, AllocType atype>

This comment has been minimized.

@bulislaw

bulislaw Aug 18, 2017

Member

That's too long can you please split it in multiple tests.

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch 7 times, most recently Aug 18, 2017

@mprse mprse changed the title from Add MemoryPool test and fixes in MemoryPool.h file. to Add MemoryPool test. Aug 21, 2017

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
*
* */
template<typename T, const uint32_t numOfEntries, AllocType atype>
void test_mem_pool_alloc_success() {

This comment has been minimized.

@0xc0170

This comment has been minimized.

@mprse

mprse Sep 11, 2017

Member

@0xc0170 codding style has been updated.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
T * p_blocks[numOfEntries];
uint32_t i;
/* Create a memory pool. */

This comment has been minimized.

@0xc0170

0xc0170 Aug 21, 2017

Member

It is good to have comments but I find a lot of them redundant . like create a memory pool, it is obvious from the code.

the comment on line 80 below for instance is fine.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
* Then deallocation fails with osErrorParameter error.
*
*/
void free_block_invalid_prarameter_null() {

This comment has been minimized.

@0xc0170

0xc0170 Aug 21, 2017

Member

typo - parameter

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch Aug 21, 2017

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
*
* Given MemoryPool object of the specified type and queue size has
* been successfully created.
* When max number of blocks is allocated form the pool.

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

typo -> from the pool

This comment has been minimized.

@mprse

mprse Aug 24, 2017

Member

Fixed.

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
T * p_blocks[numOfEntries];
uint32_t i;
p_mem_pool = new MemoryPool<T, numOfEntries>();

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

Why are you use dynamic allocation of the pool?

This comment has been minimized.

@mprse

mprse Aug 23, 2017

Member

The intention was to not allocate lots of data on stack. In case when for example block size is equal to 12 bytes and pool consists of 100 blocks. In this case we would need over 1kB of memory for such memory pool. But then we agreed that such large numbers do not need to be tested. On the other hand why using dynamic allocation is a problem?

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
utest::v1::status_t test_setup(const size_t number_of_cases)
{
GREENTEA_SETUP(45, "default_auto");

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

Can we go lower than 45 or is it actually as long as it takes?

This comment has been minimized.

@mprse

mprse Aug 24, 2017

Member

Timeout has been modified to 20 s.

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch Aug 24, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 25, 2017

@0xc0170 can you have a look

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 25, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1119

Build failed!

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Aug 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2017

I restarted Travis, should become green. In case not, please close/opne the PR to restart travis. It sometimes does not report the status back

TESTS/mbedmicro-rtos-mbed/MemoryPool/main.cpp Outdated
if(object->a == 0 &&
object->b == 0 &&
object->c == 0)
{

This comment has been minimized.

@0xc0170

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch 2 times, most recently Sep 11, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

@0xc0170 Could you review please?

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 21, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1353

Build failed!

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch Sep 22, 2017

@mprse

This comment has been minimized.

Member

mprse commented Sep 22, 2017

Output
mbed Build Number: 1353
Build failed!

One error on NUMAKER_PFM_NANO130 platform using IAR compiler: unable to allocate space for sections/blocks with a total estimated minimum size of 0x3b90 bytes (max align 0x8) in <[0x20000000-0x20003fff]> (total uncommitted space 0x3a00).

Test has been updated. Memory usage has been decreased.

@theotherjimmy could you please re-run morph test.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 22, 2017

There's couple of indentation errors. Also please split the PR into two commits: fixes to the original header and the tests.

Shall this be splitted as requested prior we run another CI?

@mprse

This comment has been minimized.

Member

mprse commented Sep 22, 2017

Shall this be splitted as requested prior we run another CI?

This was my first PR and it initially provided two features:

  • fix in memory pool header file, and
  • memory pool test.

As requested this has been splitted into two PRs. Currently this PR provides only the test.
The header file has been fixed here:
#4941

@adbridge adbridge added needs: CI and removed ready for merge labels Oct 16, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 16, 2017

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 18, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

@mprse Please look at the latest build, there's linker error for one device.

@mprse

This comment has been minimized.

Member

mprse commented Oct 19, 2017

Test : FAILURE

Build error on NUMAKER_PFM_NANO130/IAR:

Output: IAR ELF Linker V7.80.1.28/LNX for ARM
Output: Copyright 2007-2016 IAR Systems AB.
Output:
Output: 47 710 bytes of readonly code memory
Output: 4 481 bytes of readonly data memory
Output: 1 536 bytes of readwrite data memory
Output:
Output: Errors: 2
Output: Warnings: none
Output:
Output: Link time: 0.19 (CPU) 1.00 (elapsed)
Errors: Error[Lp011]: section placement failed
Errors: unable to allocate space for sections/blocks with a total estimated
Errors: minimum size of 0x3a28 bytes (max align 0x8) in
Errors: <[0x20000000-0x20003fff]> (total uncommitted space
Errors: 0x3a00).
Errors: Error[Lp021]: the destination for compressed initializer batch "P2-P3-1" is
Errors: placed at an address that is dependent on the size of the batch,
Errors: which is not allowed when using lz77 compression. Consider using
Errors: "initialize by copy with packing = zeros" (or none) instead.

Decreased memory usage by reducing number of test cases. Wrapper functions have been used to test alloc() and calloc() memory allocation types together (in one test case).
Hopefully this will solve the problem for time longer than two weeks.

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch 2 times, most recently Oct 20, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 24, 2017

/morph uvisor-test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 25, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 26, 2017

@bulislaw Can you rereview after the update, tests are now green

@@ -0,0 +1,608 @@
#include "mbed.h"

This comment has been minimized.

@0xc0170

0xc0170 Oct 26, 2017

Member

Can you please add the license header here, every file should have it

This comment has been minimized.

@mprse

mprse Oct 27, 2017

Member

Licence header has been added.

@bulislaw

Only the header is missing

Add MemoryPool test.
Add test for MemoryPool.

@mprse mprse force-pushed the mprse:memPool_test_and_fixes branch to b7e2776 Oct 27, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 27, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 27, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@adbridge adbridge added ready for merge and removed needs: CI labels Oct 30, 2017

@theotherjimmy theotherjimmy merged commit 55197e7 into ARMmbed:master Oct 30, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment