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

Extends test set for Ticker class #5261

Merged
merged 1 commit into from Nov 16, 2017

Conversation

Projects
None yet
8 participants
@maciejbocianski
Member

maciejbocianski commented Oct 5, 2017

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

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

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch Oct 5, 2017

TESTS/mbed_drivers/ticker/main.cpp Outdated
/* CPU clock */
extern uint32_t SystemCoreClock;
#define TOLERANCE_FACTOR 15000.0f

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

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

TESTS/mbed_drivers/ticker/main.cpp Outdated
Ticker ticker;
int32_t ret;
ticker.attach(callback(wait_callback_sem), 0.1f);

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

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);

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 9, 2017

Member

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

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 9, 2017

Member

changed to local semaphore passed as pointer

Semaphore sem(0, 1);
ticker.attach(callback(sem_release, &sem), 0.1f);
TESTS/mbed_drivers/ticker/main.cpp Outdated
/** Test many tickers run one after the other
Given a 16 Tickers

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

Given a 16 Tickers

TESTS/mbed_drivers/ticker/main.cpp Outdated
ticker[i].attach_us(callback(wait_callback_multi), 100 * 1000);
}
Thread::wait(105);

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

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

TESTS/mbed_drivers/ticker/main.cpp Outdated
ticker[i].attach_us(callback(wait_callback_multi), (100 + i) * 1000);
}
Thread::wait(120);

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

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

TESTS/mbed_drivers/ticker/main.cpp Outdated
while(!ticker_callback_flag);
time_diff = gtimer.read_us();
TEST_ASSERT_UINT32_WITHIN(TOLERANCE, 100000, time_diff);

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

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

TESTS/mbed_drivers/ticker/main.cpp Outdated
ticker_callback_flag = true;
}
void wait_callback_multi(void)

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

May we have a better naming ? increment_multi_counter for instance.

TESTS/mbed_drivers/ticker/main.cpp Outdated
void wait_callback_flag(void)

This comment has been minimized.

@pan-

pan- Oct 5, 2017

Member

May we have a better naming ? stop_gtimer for instance.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch 4 times, most recently Oct 6, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 9, 2017

@pan- updated after review

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch 2 times, most recently Oct 9, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2017

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

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch 2 times, most recently Oct 9, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 9, 2017

@pan- Are you happy with the new version?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 9, 2017

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 9, 2017

TESTS/mbed_drivers/ticker/main.cpp Outdated
@@ -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 comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

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

This comment has been minimized.

@maciejbocianski
TESTS/mbed_drivers/ticker/main.cpp Outdated
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 */

This comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

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

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 9, 2017

Member

on most platforms osKernelGetSysTimerFreq works
but look #4986

This comment has been minimized.

@0xc0170

0xc0170 Oct 10, 2017

Member

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

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 11, 2017

Member

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

TESTS/mbed_drivers/ticker/main.cpp Outdated
volatile bool ticker_callback_flag;
volatile uint32_t multi_counter;

This comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

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

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 10, 2017

Member

you mean increment/change atomically ?

This comment has been minimized.

@0xc0170

0xc0170 Oct 10, 2017

Member

Yes, use atomic operations we provide

This comment has been minimized.

@maciejbocianski

maciejbocianski Oct 10, 2017

Member

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 ?

This comment has been minimized.

@pan-

pan- Oct 10, 2017

Member

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.

TESTS/mbed_drivers/ticker/main.cpp Outdated
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)

This comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

increment_ticker_counter ?

This comment has been minimized.

@maciejbocianski
TESTS/mbed_drivers/ticker/main.cpp Outdated
++callback_trigger_count;
}
void ticker_callback_1_led(void) {
void ticker_callback_1_led(void)

This comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

switch_led1_state ?

This comment has been minimized.

@maciejbocianski
TESTS/mbed_drivers/ticker/main.cpp Outdated
led1 = !led1;
}
void ticker_callback_2_led(void) {
void ticker_callback_2_led(void)

This comment has been minimized.

@pan-

pan- Oct 9, 2017

Member

switch_led2_state ?

This comment has been minimized.

@maciejbocianski

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch Oct 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

/morph test

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 10, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1560

Example Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

/morph test

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch Oct 10, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch Oct 11, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 12, 2017

Can you look at NCS36510 that fails ?

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests2 branch to aaa15bc Oct 19, 2017

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

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 27, 2017

@maciejbocianski what's the status of that?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 27, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 13, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 13, 2017

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

This comment has been minimized.

Collaborator

studavekar commented Nov 16, 2017

/morph test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Nov 16, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

@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 0xc0170 added needs: review and removed needs: work labels Nov 16, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

@pan- Can you please review the latest update?

@pan-

pan- approved these changes Nov 16, 2017

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

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

6 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter 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