Reduce test overhead in preperation for CMSIS 5 #4317

c1728p9 · 2017-05-15T00:05:58Z

Reduce RAM consumption so all tests can still be built when using CMSIS/RTX5. Also reduce clutter by removing the per target stack size defines in the tests.

c1728p9 · 2017-05-15T01:35:40Z

/morph test-nightly

mbed-bot · 2017-05-15T05:05:32Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 216

Test failed!

0xc0170 · 2017-05-15T10:01:54Z

Looks like Testing parallel threads with child fails for some platforms, the change is in this patchset, please verify

bulislaw

Ah that seems to be connected with some other explicit or implicit changes to sizes that makes it 'just fit' on feature_cmisis5 branch.

pan- · 2017-05-15T10:48:34Z

@bulislaw @0xc0170 I'm trying to reproduce the issue but running the test locally on the NRF52_DK raise another issue: it timeouts in the test Testing single thread with wait.

If I run manually each test (skipping utest init part) with a debugger, then I got the same error as the one in the CI, the counter under test in the case Testing parallel threads with child is equal to 2 instead of 3.

bulislaw · 2017-05-15T10:49:56Z

Maybe it's a wast of time to do it if it's not an obvious fix, as it's part of bigger PR...

pan- · 2017-05-15T11:19:34Z

@bulislaw The issue is related to the configuration of RTX on those targets, they supports 6 concurrent tasks while the case Testing parallel threads with child requires up to 7 concurrent tasks.

bulislaw · 2017-05-15T11:20:49Z

Ok, but that changes with RTX5. We can try temporarily bumping supported task number?

pan- · 2017-05-15T11:28:20Z

Definitely, I suspect this failure was hidden for a long time (it depends a lot on thread scheduling....), it has to be addressed but it might be better to do it with the new version of RTX.

Affected boards: NRF51_DK, NRF52_DK, Beetle. This change is temporary and will be overwritten by RTX update. It's necessary for the boards to pass updated tests.

bulislaw · 2017-05-15T12:25:20Z

/morph test-nightly

pan- · 2017-05-15T12:26:07Z

@0xc0170 nRF and beetle.

The test requires refactoring to take the number of concurrent threads allowed into account.

0xc0170 · 2017-05-15T14:14:50Z

The test requires refactoring to take the number of concurrent threads allowed into account.

We can do that, update the test to consider this number?

bulislaw · 2017-05-15T14:24:09Z

It changes with RTX5, no much point addressing it now. For new RTX there's no global mem carveout for threads, it's purely down to available memory, on creation it's either success or fail.

bridadan

I think these changes are ok. Generally it's nice to have the some of those debug printf calls in the tests so if something goes wrong you have an idea of how far the test got. However, if it improves memory usage then that seems like a necessary trade off.

bridadan · 2017-05-15T18:07:19Z

TESTS/mbedmicro-rtos-mbed/basic/main.cpp

-#else
-#define STACK_SIZE DEFAULT_STACK_SIZE
-#endif
+#define TEST_STACK_SIZE 768


bridadan · 2017-05-15T18:21:29Z

@geky There's a few equeue test changes here you may be interested in.

mbed-bot · 2017-05-15T19:24:26Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 221

All builds and test passed!

geky

Don't remove all the printfs from the block device tests! That makes the test debatably useless since it gives the user no direction if it fails.

Until we have a better way to decide which tests fit on which platforms we should just filter out the storage tests (maybe an ifdef MBED_STORAGE_TESTS at the top like these?

It looks like we're also losing a bunch of useful information from the other tests. We shouldn't lose information. Perhaps we should have some flag for if we want more logging?

if (TEST_DEBUG) {
    printf("this thing happened\n");
}

If we want a more long-term solution, should we consider running tests in develop or release mode? Then we could adopt the debug function that is actually intended for this purpose.

geky · 2017-05-15T20:56:16Z

TESTS/mbedmicro-rtos-mbed/mail/main.cpp

@@ -54,8 +37,7 @@ void send_thread () {
 int main (void) {
    GREENTEA_SETUP(20, "default_auto");

-    Thread thread(osPriorityNormal, STACK_SIZE);
-    thread.start(send_thread);
+    Thread thread(send_thread, NULL, osPriorityNormal, STACK_SIZE);


Isn't this style of starting threads deprecated?

It is... I'm sure it's some merge weirdness. I'll revert that back to what it was.

geky · 2017-05-15T21:09:40Z

features/TESTS/filesystem/heap_block_device/main.cpp

-        for (int i = 0; i < 16; i++) {
-            printf("%02x", read_block[i]);
-        }
-        printf("...\n");


Uhm, wait a sec, these are used for determining the types of errors that occur during block read/write. Since this is the main test used for porting new block devices this is rather important.

Is pulling in printf and math really too much to run in CI?

If that's the case we really need to reconsider what sort of devices we are expecting to support storage. This test doesn't even have a filesystem yet!

Let me see if I can revert that, as I manage to shave some memory somewhere else, it may not be necessary. On the side note, this sort of prints in test shouldn't be necessary. We should be asserting things we care about in a way that when they fail we will know what failed from assert not from a print.

geky · 2017-05-15T21:31:46Z

Is anyone else getting red flags from how much we are having to reduce the tests? They have been quite clunky in some places, but surely there are applications that depend on us that are just as clunky.

theotherjimmy · 2017-05-15T21:35:11Z

@geky It does raise red flags. Are these tests unusually huge? in that case maybe we should pair them down to more functional/unit test style. Are these test just not fitting because mbed OS is large? We should re-consider how much flash we take up...

geky · 2017-05-15T21:33:07Z

TESTS/events/queue/main.cpp

@@ -7,6 +7,7 @@

 using namespace utest::v1;

+#define TEST_EQUEUE_SIZE 1024


@bridadan pointed this out to me. Looks great and matches the way we're handling stacks here 👍

geky · 2017-05-15T21:35:38Z

TESTS/mbed_drivers/stl_features/main.cpp

-    //std::for_each(floats_str, floats_str + TABLE_SIZE(floats_str), printString());
-    //printf("stl_transform::floats_transform: ");
-    //std::for_each(floats_transform, floats_transform + TABLE_SIZE(floats_transform), printFloat());
-    //printf("\n");

    TEST_ASSERT_TRUE(std::equal(floats_transform, floats_transform + TABLE_SIZE(floats_transform), floats));
 }

 void test_case_stl_sort_greater() {


I hope I'm not repeating a discussion, but have we considered if it's worthwhile to keep these stl tests?

You could spend a lifetime trying to add coverage to the stl....

We could remove them, shall be done via separate PR

c1728p9 · 2017-05-15T22:18:20Z

@bulislaw could you review these test changes and see if the removal of all the printfs are necessary and if not update the PR?

Revert removal of printfs. Make the block device size smaller.

bulislaw · 2017-05-16T09:55:19Z

/morph test-nightly

bulislaw · 2017-05-16T09:59:00Z

I think that our testing should use printf, we should have asserts that actually tell us what's going wrong. Anyway, I reverted removing printfs and made the heap device smaller for small targets. Also we have issues with small platforms like nrf51_dk which when you take the soft devices, 4k main stack, 1k ISR stack there's no that much memory left for all the threads (with stacks) or heap block devices.

New changes submitted.

pan- · 2017-05-16T10:42:46Z

@bulislaw RawSerial::printf doesn't do buffering and is lightweight compared to the regular printf. Maybe it can be used as a print replacement ?

bulislaw · 2017-05-16T10:44:13Z

Maybe we should consider of using it by default.

0xc0170 · 2017-05-16T10:50:19Z

Maybe we should consider of using it by default.

it is, if you use greentea (I believe api is in the client) that one uses rawserial (the default greentea console). Not sure now if this also is true for utest macros, but I believe yes.

pan- · 2017-05-16T12:16:24Z

@0xc0170 greentea and utest use RawSerial, the issue is for debug traces outputted directly with printf during the tests.

0xc0170

@geky I dont see here any removal of logging that should be kept in files (any crucial that a user could benefit from), I overlooked some? To the proposal, yes we should have logging in tests that should be disabled by default.

Thus this LGTM

0xc0170 · 2017-05-16T13:40:38Z

TESTS/mbed_drivers/stl_features/main.cpp

-    //std::for_each(floats_str, floats_str + TABLE_SIZE(floats_str), printString());
-    //printf("stl_transform::floats_transform: ");
-    //std::for_each(floats_transform, floats_transform + TABLE_SIZE(floats_transform), printFloat());
-    //printf("\n");

    TEST_ASSERT_TRUE(std::equal(floats_transform, floats_transform + TABLE_SIZE(floats_transform), floats));
 }

 void test_case_stl_sort_greater() {


We could remove them, shall be done via separate PR

geky · 2017-05-16T15:22:46Z

@0xc0170, yeah bulislaw reverted my main complaints. There's just the deprecated thread constructor and I'm happy

It sounds from discussions we should use a configurable logging function for tests that is normally compiled out (would the debug function work?).

I still think printfs/logging is important in tests. Consider the block device logs where we assert on errors in a 512 byte block write:

errors 0000:512 00000000000000000000000000000000 // good run, no errors
errors 0000:512 ffffffffffffffffffffffffffffffff // everything fails, probably issue with block device driver
errors 0000:512 0000000003000000000000000c500000 // random errors, probably noise?
errors 0000:512 80008000800080008000800080008000 // periodic errors, probably spi driver issue?

Unless you want to shove some sort of neural net into the poor microcontroller, I'm not sure how you could make these sort of predictions better than a human.

geky

Ah, you beat me too it XD

mbed-bot · 2017-05-16T23:29:29Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 233

All builds and test passed!

bulislaw · 2017-05-17T08:45:44Z

@0xc0170 can we merge this please.

c1728p9 force-pushed the reduce_test_overhead branch from 57a926f to 552d7ff Compare May 15, 2017 00:16

Reduce test overhead in preparation for CMSIS 5

6a0fcd3

Reduce RAM consumption so all tests can still be built when using CMSIS/RTX5. Also reduce clutter by removing the per target stack size defines in the tests.

c1728p9 force-pushed the reduce_test_overhead branch from 552d7ff to 6a0fcd3 Compare May 15, 2017 00:25

bulislaw approved these changes May 15, 2017

View reviewed changes

0xc0170 added the needs: work label May 15, 2017

bulislaw previously requested changes May 15, 2017

View reviewed changes

pan- approved these changes May 15, 2017

View reviewed changes

Temporarily increase max number of tasks for small boards

c82c40f

Affected boards: NRF51_DK, NRF52_DK, Beetle. This change is temporary and will be overwritten by RTX update. It's necessary for the boards to pass updated tests.

bridadan approved these changes May 15, 2017

View reviewed changes

TESTS/mbedmicro-rtos-mbed/basic/main.cpp

#else

#define STACK_SIZE DEFAULT_STACK_SIZE

#endif

#define TEST_STACK_SIZE 768

Copy link

Contributor

bridadan May 15, 2017

Choose a reason for hiding this comment

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

👍

geky suggested changes May 15, 2017

View reviewed changes

geky reviewed May 15, 2017

View reviewed changes

bulislaw added 2 commits May 16, 2017 10:45

Reshuffled memory usage for heap block device tests on small targets

4f3aabc

Revert removal of printfs. Make the block device size smaller.

Remove deprecated Thread constructor usage

07fc7ae

0xc0170 reviewed May 16, 2017

View reviewed changes

geky approved these changes May 16, 2017

View reviewed changes

geky mentioned this pull request May 16, 2017

bd: Add MBR block device for standard storage partitioning #3936

Merged

sg- added needs: CI and removed needs: work labels May 16, 2017

0xc0170 added ready for merge release-version: 5.5.0-rc1 and removed needs: CI ready for merge labels May 17, 2017

0xc0170 merged commit 8f29517 into ARMmbed:master May 17, 2017

betzw mentioned this pull request May 18, 2017

[NETSOCKET] TCPSocket & UDPSocket event handler cannot be run in IRQ context #4343

Closed

adbridge mentioned this pull request Jun 5, 2017

Reduce size of heap block device test #4367

Merged

1 task

		@@ -7,6 +7,7 @@

		using namespace utest::v1;

		#define TEST_EQUEUE_SIZE 1024

Reduce test overhead in preperation for CMSIS 5 #4317

Reduce test overhead in preperation for CMSIS 5 #4317

Uh oh!

Conversation

c1728p9 commented May 15, 2017

Uh oh!

c1728p9 commented May 15, 2017

Uh oh!

mbed-bot commented May 15, 2017

Output

Uh oh!

0xc0170 commented May 15, 2017

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

pan- commented May 15, 2017

Uh oh!

bulislaw commented May 15, 2017

Uh oh!

pan- commented May 15, 2017

Uh oh!

bulislaw commented May 15, 2017

Uh oh!

pan- commented May 15, 2017

Uh oh!

bulislaw commented May 15, 2017

Uh oh!

pan- commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 15, 2017

Uh oh!

bulislaw commented May 15, 2017

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan commented May 15, 2017

Uh oh!

mbed-bot commented May 15, 2017

Output

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented May 15, 2017

Uh oh!

theotherjimmy commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented May 15, 2017

Uh oh!

bulislaw commented May 16, 2017

Uh oh!

bulislaw commented May 16, 2017

Uh oh!

pan- commented May 16, 2017

Uh oh!

bulislaw commented May 16, 2017

Uh oh!

pan- commented May 15, 2017 •

edited

Loading

geky May 15, 2017 •

edited

Loading

0xc0170 May 16, 2017 •

edited

Loading

0xc0170 left a comment •

edited

Loading

0xc0170 May 16, 2017 •

edited

Loading