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

Ticker tests fix #5971

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
6 participants
@maciejbocianski
Member

maciejbocianski commented Jan 30, 2018

Description

This PR contains fix for mbed_drivers-ticker tests.

Despite the test was always green there was hidden bug in test_case_2x_callbacks subtest detected in #5955 and #5892.

In effect of this bug (source code 1):

  • ticker_callback_1_switch_to_2 was called only once
  • ticker2 was never been fired because it was repeatedly detached just before fire and attached again

Fix for this bug (source code 2) showed that time measure with repeatedly rescheduled ticker is not accurate. When we use ticker without rescheduling (like in test_case_1x_ticker) ticker callback is executed within ticker interval time - not extra time is added to ticker interval until the callback execution time is less then ticker interval time.

Test test_case_2x_callbacks (after fix - source code 2) at each interval(callback call) reschedules ticker (call detach() an attach_us()) what cause that the time between consecutive callback calls is not 1ms but 1ms + time needed to call the callback and attach new one.

So the cross-call fix, fixed the algorithm but caused the test inaccurate.

Finally the two tickers test was redesigned and now it just uses two tickers to update counter alternatively every 1ms without rescheduling

// source code 1
void ticker_callback_1_switch_to_2(void)
{
    ++callback_trigger_count;
    // If ticker is NULL then it is being or has been deleted
    if (ticker1) {
        ticker1->detach();
        ticker1->attach_us(ticker_callback_2_switch_to_1, ONE_MILLI_SEC);
    }
    switch_led1_state();
}

void ticker_callback_2_switch_to_1(void)
{
    ++callback_trigger_count;
    // If ticker is NULL then it is being or has been deleted
    if (ticker2) {
        ticker2->detach();
        ticker2->attach_us(ticker_callback_1_switch_to_2, ONE_MILLI_SEC);
    }
    switch_led2_state();
}

void test_case_2x_callbacks()
{
    ...
    ticker1->attach_us(ticker_callback_1_switch_to_2, ONE_MILLI_SEC);
    ...
}
// source code 2
void ticker_callback_1_switch_to_2(void)
{
    ++callback_trigger_count;
    // If ticker is NULL then it is being or has been deleted
    if (ticker1) {
        ticker1->detach();
    }
    if (ticker2) {
        ticker2->attach_us(ticker_callback_2_switch_to_1, ONE_MILLI_SEC);
    }
    switch_led1_state();
}

void ticker_callback_2_switch_to_1(void)
{
    ++callback_trigger_count;
    // If ticker is NULL then it is being or has been deleted
    if (ticker2) {
        ticker2->detach();
    }
    if (ticker1) {
        ticker1->attach_us(ticker_callback_1_switch_to_2, ONE_MILLI_SEC);
    }
    switch_led2_state();
}

Status

READY

Migrations

NO

maciejbocianski added some commits Jan 29, 2018

test-mbed_drivers-ticker: fix ticker cross attach
This commit fixes ticker cross-schedule bug in test_case_2x_callbacks subtest

In effect of this bug:
    ticker_callback_1_switch_to_2 was called only once
    ticker2 was never been fired because it was repeatedly detached just before fire and attached again
test-mbed_drivers-ticker: improve two ticker test accuracy
test_case_2x_callbacks test was redesigned to eliminate ticker rescheduling and improve time mesure accuracy.

Constant ticker rescheduling (detach()/attach_us() calls)
was causing the gap between consecutive callback calls was not exact 1ms
but 1ms + time needed to call the callback and attach new one.
New design just uses two tickers to update counter alternatively every 1ms without rescheduling them

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_tests_fix branch from 93bb31a to 1437651 Jan 30, 2018

@drewcassidy

This comment has been minimized.

Contributor

drewcassidy commented Jan 31, 2018

Great, this fixes the issue of test never completing.

only problem (which should probably go in its own issue) is that the tests will occasionally fail on nRF51 boards when using the internal RC (tested with an nRF51-DK). I wonder if the tolerance shouldnt be increased for those targets since the RC clock is never going to match the accuracy of the external crystal

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Jan 31, 2018

@drewcassidy usually tests are designed and run for default targets configuration so there is nothing unusual that test is failing if we change configuration. Especially when we use oscillator with lower accuracy then assumed by test designer.

If we increase test tolerance then more configurations will pass but test will be less reliable

@0xc0170 @bulislaw @jamesbeyond

@drewcassidy

This comment has been minimized.

Contributor

drewcassidy commented Jan 31, 2018

Log here: https://gist.github.com/anonymous/4873033bb990e29f3f321b5f61c1c9a4, usually the result is closer to the expected value. This is on an nRF51-DK with the RC clock selected with the app_config.json file

This should probably move back to #5892, which is attempting to add a target using the nRF51822 using the RC clock. I didnt mean to suggest increasing the tolerance universally, but as something based on the configuration. I'm not sure if thats set in the test and can just use an #ifdef or if it would require extra python tooling

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 31, 2018

Thanks @drewcassidy , we will look into this to find a way to set tolerance better

@cmonr cmonr added needs: work and removed needs: review labels Feb 1, 2018

@drewcassidy

This comment has been minimized.

Contributor

drewcassidy commented Feb 1, 2018

I checked the nRF51822 datasheet again and the internal RC clock SHOULD have an accuracy of ±250ppm, far less than the ±5% its currently failing to reach. Either the datasheet is wildly off, I have multiple defective chips, or there is something wrong with the way this test is being performed

@bulislaw bulislaw requested a review from jamesbeyond Feb 5, 2018

@0xc0170 0xc0170 referenced this pull request Feb 5, 2018

Merged

Adding LAIRD_BL600 MTB #5996

1 of 2 tasks complete

@drewcassidy drewcassidy referenced this pull request Feb 6, 2018

Merged

add OSHChip as an mbed target #5892

1 of 3 tasks complete
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 6, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

There is target addition in #5996, and having the issue with ticker, but solved with this PR.

This should probably move back to #5892, which is attempting to add a target using the nRF51822 using the RC clock. I didnt mean to suggest increasing the tolerance universally, but as something based on the configuration. I'm not sure if thats set in the test and can just use an #ifdef or if it would require extra python tooling

We will review this test fix, and we should still look at why it still fails for your setup, but as you mentioned, might move to 5892 (in case this goes in, you can review the test with your new target addition, and resolve the issue there).

Restarting

/morph uvisor-test

@0xc0170 0xc0170 added needs: review and removed needs: work labels Feb 6, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

/morph uvisor-test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

@0xc0170

0xc0170 approved these changes Feb 7, 2018

@jamesbeyond

looks good!

@cmonr cmonr merged commit 0978062 into ARMmbed:master Feb 8, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
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
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment