Extends test set for Ticker class #5261

maciejbocianski · 2017-10-05T10:22:47Z

Description

New test suite for Ticker class with fixes

Fix for NRF51/NRF51 included - time measure accuracy improved
Alternative version of fix #5255 included

Fixes for fails on KL46Z / MAX32625MBED / MAX32630FTHR / NCS36510 not inclcuded

Problems with KL46Z / MAX32625MBED / MAX32630FTHR / NCS36510 looks similar to #5004

Not all callbacks executed and TEST_ASSERT_EQUAL(TICKER_COUNT, multi_counter); fails (see below code)

#define TICKER_COUNT 16

Ticker ticker[TICKER_COUNT];

multi_counter = 0;
for (int i = 0; i < TICKER_COUNT; i++) {
    ticker[i].attach_us(callback(wait_callback_multi), 100 * 1000);
}

Thread::wait(105);
for (int i = 0; i < TICKER_COUNT; i++) {
    ticker[i].detach();
}

TEST_ASSERT_EQUAL(TICKER_COUNT, multi_counter);

Status

HOLD

Related PRs

#5006 - reverted due to fails
#5255

Related issues:

#5004
#5291

Blocked by

#5309
#5307 - seems to be fixed by #5390 waitng for retest

pan- · 2017-10-05T13:28:43Z

TESTS/mbed_drivers/ticker/main.cpp

+/* CPU clock */
+extern uint32_t SystemCoreClock;
+
+#define TOLERANCE_FACTOR 15000.0f


Could you document why that value was chosen as the tolerance factor ?

pan- · 2017-10-05T14:21:47Z

TESTS/mbed_drivers/ticker/main.cpp

+    Ticker ticker;
+    int32_t ret;
+
+    ticker.attach(callback(wait_callback_sem), 0.1f);


The semaphore can be local and there is no need to create a new function to release the semaphore:

Semaphore sem(0, 1); ticker.attach(callback(&sem, &Semaphore::release), 0.1f);

but attach expects Callback<void()> func and Semaphore::release return osStatus

changed to local semaphore passed as pointer

Semaphore sem(0, 1); ticker.attach(callback(sem_release, &sem), 0.1f);

pan- · 2017-10-05T14:23:01Z

TESTS/mbed_drivers/ticker/main.cpp

+
+/** Test many tickers run one after the other
+
+    Given a 16 Tickers


Given a 16 Tickers

pan- · 2017-10-05T14:26:32Z

TESTS/mbed_drivers/ticker/main.cpp

+        ticker[i].attach_us(callback(wait_callback_multi), 100 * 1000);
+    }
+
+    Thread::wait(105);


Would be better if this value was not hard coded but computed from the time set in attach_us.

pan- · 2017-10-05T14:26:53Z

TESTS/mbed_drivers/ticker/main.cpp

+        ticker[i].attach_us(callback(wait_callback_multi), (100 + i) * 1000);
+    }
+
+    Thread::wait(120);


Would be better if this value was not hard coded but computed from the time set in attach_us.

pan- · 2017-10-05T14:32:30Z

TESTS/mbed_drivers/ticker/main.cpp

+        while(!ticker_callback_flag);
+        time_diff = gtimer.read_us();
+
+        TEST_ASSERT_UINT32_WITHIN(TOLERANCE, 100000, time_diff);


Please do not hardcode 100000. A local variable constant well named would help maintenance and readability.

pan- · 2017-10-05T14:36:32Z

TESTS/mbed_drivers/ticker/main.cpp

+    ticker_callback_flag = true;
+}
+
+void wait_callback_multi(void)


May we have a better naming ? increment_multi_counter for instance.

pan- · 2017-10-05T14:37:04Z

TESTS/mbed_drivers/ticker/main.cpp


+void wait_callback_flag(void)


May we have a better naming ? stop_gtimer for instance.

maciejbocianski · 2017-10-09T08:16:15Z

@pan- updated after review

0xc0170 · 2017-10-09T13:27:05Z

There was a fix merged recently that is causing this PR to be rebased

theotherjimmy · 2017-10-09T15:56:09Z

@pan- Are you happy with the new version?

theotherjimmy · 2017-10-09T15:57:07Z

Note to self, this should pass nightly 3 times in a row before going in.

mbed-ci · 2017-10-09T17:24:53Z

Build : SUCCESS

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

pan- · 2017-10-09T18:10:18Z

TESTS/mbed_drivers/ticker/main.cpp

@@ -85,7 +123,8 @@ void ticker_callback_2_switch_to_1(void) {
    ticker_callback_2_led();
 }

-void wait_and_print() {
+void wait_and_print()


This function is not used, would it be possible to remove it as well as symbols only used by it ?

pan- · 2017-10-09T18:22:00Z

TESTS/mbed_drivers/ticker/main.cpp

 volatile uint32_t callback_trigger_count = 0;
 static const int test_timeout = 240;
 static const int total_ticks = 10;

+extern uint32_t SystemCoreClock; /* CPU clock */


Is there a portable way to access to that declaration from an header ?

on most platforms osKernelGetSysTimerFreq works
but look #4986

Is there a portable way to access to that declaration from an header ?

I wish there was as this is part of cmsis, they declare it there as extern symbol in the cmsis startup files

I noticed lately that there was some changes and now osKernelGetSysTimerFreq return SystemCoreClock for all platforms. It uses osRtxSysTimerGetFreq and there is no platform overriding it so below default implementation is used for all targets

... osRtxInfo.kernel.sys_freq = SystemCoreClock; ... __WEAK uint32_t osRtxSysTimerGetFreq (void) { return osRtxInfo.kernel.sys_freq; }

So now we could use osKernelGetSysTimerFreq instead SystemCoreClock

pan- · 2017-10-09T18:25:17Z

TESTS/mbed_drivers/ticker/main.cpp

+
+
+volatile bool ticker_callback_flag;
+volatile uint32_t multi_counter;


Ideally those variables should be accessed atomically, volatile does not guarantee atomicity.

you mean increment/change atomically ?

Yes, use atomic operations we provide

there is no function: core_util_atomic_incr_u32(volatile uint32_t *valuePtr so we have to cast to remove volatile: core_util_atomic_incr_u32((uint32_t*)&multi_counter, 1);

Other option is to use just core_util_critical_section to protect multi_counter change
O mayby we should have volatile versions of core_util_atomic_ functions ?
@0xc0170 @pan- what do you think ?

volatile should not be required for atomic access because the memory order we use is similar to std::memory_order_seq_cst so data access should not be reordered.

pan- · 2017-10-09T18:27:26Z

TESTS/mbed_drivers/ticker/main.cpp


 volatile int ticker_count = 0;
 volatile bool print_tick = false;

 void ticker_callback_1_switch_to_2(void);
 void ticker_callback_2_switch_to_1(void);

-void ticker_callback_0(void) {
+void ticker_callback_0(void)


increment_ticker_counter ?

pan- · 2017-10-09T18:29:36Z

TESTS/mbed_drivers/ticker/main.cpp

    ++callback_trigger_count;
 }

-void ticker_callback_1_led(void) {
+void ticker_callback_1_led(void)


switch_led1_state ?

pan- · 2017-10-09T18:29:48Z

TESTS/mbed_drivers/ticker/main.cpp

    led1 = !led1;
 }

-void ticker_callback_2_led(void) {
+void ticker_callback_2_led(void)


switch_led2_state ?

0xc0170 · 2017-10-10T07:01:50Z

/morph test

mbed-ci · 2017-10-10T07:52:43Z

Build : FAILURE

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

mbed-bot · 2017-10-10T09:16:46Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1560

Example Build failed!

0xc0170 · 2017-10-10T12:20:42Z

/morph test

mbed-ci · 2017-10-10T14:20:56Z

Build : FAILURE

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

mbed-ci · 2017-10-11T12:30:04Z

Build : SUCCESS

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

Triggering tests

/test mbed-os

mbed-ci · 2017-10-12T02:38:35Z

Test : FAILURE

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

0xc0170 · 2017-10-12T07:44:40Z

Can you look at NCS36510 that fails ?

bulislaw · 2017-10-27T15:31:07Z

@maciejbocianski what's the status of that?

maciejbocianski · 2017-10-27T15:45:43Z

waiting for #5307 and #5309
In spite of that this devices are unplugged from CI the problem still exist

0xc0170 · 2017-11-13T08:52:03Z

/morph build

mbed-ci · 2017-11-13T10:09:34Z

Build : SUCCESS

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

Triggering tests

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

studavekar · 2017-11-16T06:06:22Z

/morph test
/morph export-build

mbed-ci · 2017-11-16T07:05:39Z

Exporter Build : SUCCESS

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

mbed-ci · 2017-11-16T07:09:15Z

Test : SUCCESS

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

maciejbocianski · 2017-11-16T08:06:42Z

@0xc0170 tests passed, looks that #5307 is fixed by #5390 so I will resolve #5307 as fixed ?

0xc0170 · 2017-11-16T11:31:25Z

@0xc0170 tests passed, looks that #5307 is fixed by #5390 so I will resolve #5307 as fixed ?

+1 , if you confirm that is fixed, should be !

0xc0170 · 2017-11-16T11:32:04Z

@pan- Can you please review the latest update?

0xc0170 added the needs: work label Oct 5, 2017

maciejbocianski force-pushed the ticker_tests2 branch from 25be029 to 9135ac8 Compare October 5, 2017 11:53

pan- reviewed Oct 5, 2017

View reviewed changes

maciejbocianski force-pushed the ticker_tests2 branch 4 times, most recently from 4a3f680 to 4243404 Compare October 9, 2017 08:10

maciejbocianski force-pushed the ticker_tests2 branch 2 times, most recently from c89f9e7 to 69f7fd8 Compare October 9, 2017 13:24

maciejbocianski force-pushed the ticker_tests2 branch 2 times, most recently from c17f0df to 423f131 Compare October 9, 2017 13:54

theotherjimmy added needs: review and removed needs: work labels Oct 9, 2017

pan- reviewed Oct 9, 2017

View reviewed changes

maciejbocianski force-pushed the ticker_tests2 branch from 423f131 to 5421213 Compare October 10, 2017 06:37

maciejbocianski force-pushed the ticker_tests2 branch from 5421213 to 5fb0759 Compare October 10, 2017 12:57

maciejbocianski force-pushed the ticker_tests2 branch from 5fb0759 to 4660555 Compare October 11, 2017 09:20

0xc0170 added the needs: preceding PR label Oct 19, 2017

Extends test set for Ticker class (round 2)

aaa15bc

maciejbocianski force-pushed the ticker_tests2 branch from 4660555 to aaa15bc Compare October 19, 2017 13:13

0xc0170 removed the needs: work label Oct 19, 2017

theotherjimmy added needs: review needs: work and removed needs: preceding PR needs: review labels Nov 13, 2017

0xc0170 added needs: review and removed needs: work labels Nov 16, 2017

pan- approved these changes Nov 16, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Nov 16, 2017

0xc0170 approved these changes Nov 16, 2017

View reviewed changes

0xc0170 changed the title ~~Extends test set for Ticker class (round 2)~~ Extends test set for Ticker class Nov 16, 2017

0xc0170 added the release-version: 5.6.5 label Nov 16, 2017

0xc0170 merged commit 56aa7c3 into ARMmbed:master Nov 16, 2017

0xc0170 removed the ready for merge label Nov 16, 2017

0xc0170 mentioned this pull request Nov 20, 2017

tests-mbed_drivers-ticker tests are failing on KL46Z #5529

Closed

maciejbocianski mentioned this pull request Nov 23, 2017

KL46Z: Inconsistent Ticker callback calls when scheduled many tickers at the same time #5564

Closed


		/** Test many tickers run one after the other

		Given a 16 Tickers



		volatile bool ticker_callback_flag;
		volatile uint32_t multi_counter;

Extends test set for Ticker class #5261

Extends test set for Ticker class #5261

Uh oh!

Conversation

maciejbocianski commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Related PRs

Related issues:

Blocked by

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!

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!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Oct 9, 2017

Uh oh!

0xc0170 commented Oct 9, 2017

Uh oh!

theotherjimmy commented Oct 9, 2017

Uh oh!

theotherjimmy commented Oct 9, 2017

Uh oh!

mbed-ci commented Oct 9, 2017

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!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski Oct 11, 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!

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!

maciejbocianski Oct 10, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Oct 5, 2017 •

edited

Loading

maciejbocianski Oct 11, 2017 •

edited

Loading

maciejbocianski Oct 10, 2017 •

edited

Loading