Ticker tests fix

[maciejbocianski]

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

[drewcassidy]

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]

@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]

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]

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

[drewcassidy]

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

[0xc0170]

/morph build

[mbed-ci]

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]

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]

/morph uvisor-test

[mbed-ci]

Test : SUCCESS

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

[mbed-ci]

Exporter Build : SUCCESS

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

[cmonr]

/morph uvisor-test

[cmonr]

/morph uvisor-test

[jamesbeyond]

looks good!