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

Provide fix for issue #5835 - Tickers update should be atomic #5889

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
6 participants
@mprse
Member

mprse commented Jan 19, 2018

Description

Perform update of the present time in critical section.

Status

READY

Migrations

NO

Steps to test or reproduce

While working on new lp ticker driver for NRF51_DK board (#5629), some problems with lp ticker has been detected.

Performed test:

volatile int cnt = 0;
void isr()
{
    cnt++;
}

void test()
{
    {
        Timer timer;
        LowPowerTicker ticker;
        timer.start();

        ticker.attach_us(isr, 1000);

        int val = 0;
        int buf = 0;

        while(val < 1) {
            val = timer.read();

            if(val != buf) {
                printf("--> val: %d \n", val);
                buf = val;
            }

        }
        ticker.detach();
        printf("---> cnt: %d \n", cnt);
    }
}

We expect that isr() function will be executed about 1000 times and exit from while loop is after one second:

val: 1
cnt: ¬1000

Test results without this fix NRF51_DK/GCC_ARM:

[1516363480.03][CONN][RXD] --> val: 3
[1516363480.05][CONN][RXD] ---> cnt: 3237

[1516363445.63][CONN][RXD] --> val: 3
[1516363445.65][CONN][RXD] ---> cnt: 3228

[1516362979.66][CONN][RXD] --> val: 3
[1516362979.67][CONN][RXD] ---> cnt: 3219

Test results with this fix NRF51_DK/GCC_ARM:

[1516363621.83][CONN][RXD] --> val: 1
[1516363621.86][CONN][RXD] ---> cnt: 1016

[1516363576.84][CONN][RXD] --> val: 1
[1516363576.86][CONN][RXD] ---> cnt: 1016

[1516362931.19][CONN][RXD] --> val: 1
[1516362931.20][CONN][RXD] ---> cnt: 1016

@0xc0170 0xc0170 requested review from pan- and c1728p9 Jan 19, 2018

@0xc0170

Thanks for the testing before/after 👍

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 19, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 19, 2018

Build : SUCCESS

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

Triggering tests

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

@pan-

pan- approved these changes Jan 19, 2018

If ever the time spent in critical section is too long, I suppose this can be rewritten in way where all the computation happen outside of the critical section.

The only parts that needs protection are data reads and data writes.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 19, 2018

/morph test

@cmonr cmonr added needs: CI and removed needs: review labels Jan 19, 2018

@mbed-ci

This comment has been minimized.

@c1728p9

c1728p9 approved these changes Jan 19, 2018 edited

One alternative you may want to consider is synchronizing only (and all) the public functions. Remaining function which could benefit from synchronization:

  • ticker_set_handler
  • ticker_irq_handler
  • ticker_read_us
  • ticker_read (implicitly synchronized if ticker_read_us is)
@mprse

This comment has been minimized.

Member

mprse commented Jan 22, 2018

Provided fix has influence on tests-mbed_drivers-ticker. This test now fails on NRF51_DK/GCC_ARM.
Probably time spent in critical section is too long.

Provide fix for issue #5835 - Tickers update should be atomic
Synchronise only (and all) the public functions.

@mprse mprse force-pushed the mprse:fix_issue_5835_ticker_update_should_be_atomic branch from c5bb736 to 6d3ba94 Jan 22, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jan 22, 2018

As suggested by @c1728p9 synchronized only (and all) the public functions.

Tested (tests-mbed_drivers-ticker) on the following platforms: NRF51_DK, K64F, NUCLEO_F070RB (all tool-chains) with positive results.

Please review.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 22, 2018

/morph build

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 22, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 467e25f into ARMmbed:master Jan 23, 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