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 test improvement #6935

Merged
merged 1 commit into from Jun 13, 2018

Conversation

Projects
None yet
9 participants
@maciejbocianski
Member

maciejbocianski commented May 17, 2018

Description

Make multiticker test more reliable when scheduling very early interrupts.

When very early interrupt is scheduled (e.g now + 2[ticks]) then it's likely that it won't be fired in some circumstances and as a result there is overdue event in queue. That overdue event can be mistakenly scheduled again by Ticker::detach (detach calls schedule if head was removed). That's why we should check interrupts counter immediately after wait period and before detach loop

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented May 17, 2018

@mprse @c1728p9 could you review

@0xc0170 0xc0170 added the needs: CI label May 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 17, 2018

/morph build

@bulislaw

This comment has been minimized.

Member

bulislaw commented May 17, 2018

How does that fit the ticker feature branch? It's going to be merged soon so if it's not applicable to the new branch should we skip it to avoid conflicts?

@mbed-ci

This comment has been minimized.

mbed-ci commented May 17, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 18, 2018

@maciejbocianski Could you answer @bulislaw's question?

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 18, 2018

/morph test

@mbed-ci

This comment has been minimized.

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented May 18, 2018

@bulislaw it's not critical, could be postponed.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 18, 2018

How does that fit the ticker feature branch? It's going to be merged soon so if it's not applicable to the new branch should we skip it to avoid conflicts?

Setting this as needs preceding PR.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 29, 2018

@bulislaw Since HAL has landed, I take it this is clear to continue?

@cmonr cmonr added the needs: work label May 29, 2018

@maciejbocianski maciejbocianski dismissed stale reviews from 0xc0170 and mprse via 7a0a01c May 29, 2018

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_test_fix branch from 42dea31 to 7a0a01c May 29, 2018

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_test_fix branch from 7a0a01c to 45d295d May 30, 2018

@@ -91,9 +95,12 @@ void test_multi_ticker(void)
}

Thread::wait(MULTI_TICKER_TIME_MS + TICKER_COUNT + extra_wait);
multi_counter_snapshot = multi_counter;

This comment has been minimized.

@0xc0170

0xc0170 May 30, 2018

Member

When very early interrupt is scheduled (e.g now + 2[ticks]) then it's likely that it won't be fired in some circumstances and as a result there is overdue event in queue. That overdue event can be mistakenly scheduled again by Ticker::detach (detach calls schedule if head was removed). That's why we should check interrupts counter immediately after wait period and before detach loop

I don't follow the description and these changes here. can you elaborate how multicounter snapshot fixes it?

This comment has been minimized.

@maciejbocianski

maciejbocianski May 30, 2018

Member

Assume the following scenario (time is mesured in ticks)
Running test_multi_ticker with TICKER_COUNT == 3 (tickers t1, t2, t3) and ticker period 100
Then we get following events scheduled at time 0

event_queue: e1(100), e2(105), e3(105)
now == 0

at time 100 e1 is triggered (and next event for t1 is scheduled e11(200))
at time 103 e2 interrupt is set(scheduled) (now == 103)
for e2 interrupt will not be trigerred (trying to set (now + 2) what is less then (now + 3))
(on some targets to be sure that interrupt will be triggered we can't set interrupt earlier than (now + 3 ticks))

After wait instruction we have following state:

event_queue: e2(105), e3(105), e11(200)
multi_counter == 1 (e1 triggered)
now == 110
multi_counter_snapshot = multi_counter

In detach loop:
detach t1 events then e11 will be removed
detach t2 events then e2 will be removed
at this point since head was removed,
schedule_interrupt is called and since e3
is overdue fire_interrupt is called for e3
(and next event for t3 is scheduled e31(205))
detach t3 events then e31(205) will be removed

After detach loop we have following state:

event_queue: empty
multi_counter == 2 (e1 and e3 triggered)
now == 115

result when checking multi_counter: expected 3 is 2
result when checking multi_counter_snapshot: expected 3 is 1

So using multi_counter_snapshot we catch real test status, just after test end

@0xc0170 0xc0170 added needs: review and removed needs: work labels May 30, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Jun 6, 2018

@0xc0170 0xc0170 referenced this pull request Jun 7, 2018

Merged

NRF52_DK: SLEEP enable #7073

for (int i = 0; i < TICKER_COUNT; i++) {
ticker[i].detach();
}
TEST_ASSERT_EQUAL(TICKER_COUNT, multi_counter_snapshot);
TEST_ASSERT_EQUAL(TICKER_COUNT, multi_counter);

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 7, 2018

Contributor

So if multi_counter_snapshot catches real test status, why we still check multi_counter against TICKER_COUNT?

This comment has been minimized.

@maciejbocianski

maciejbocianski Jun 7, 2018

Member

Because detach calls schedule_interrupt in some circumstances (e.g. when head event is removed), so it's good to check if no more callbacks is raised during detaching.

BTW, now I see that the first ASSERT can be moved up, where the snapshot is taken
And will add some comment to second check

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 8, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 8, 2018

Build : SUCCESS

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

Triggering tests

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

ticker test improvement
Make multiticker test more reliable when scheduling very early interrupts.
When very early interrupt is scheduled (e.g now + 2[ticks]) then it's likely
that it won't be fired in some circumstances and as a result there is overdue
event in queue. That overdue event can be mistakenly scheduled again by
`Ticker::detach` (detach calls schedule if head was removed). That's why we
should check interrupts counter immediately after wait period and before detach loop

@maciejbocianski maciejbocianski dismissed stale reviews from c1728p9 and 0xc0170 via 27bf6cf Jun 8, 2018

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_test_fix branch from 45d295d to 27bf6cf Jun 8, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 8, 2018

Pausing CI until 5.9 RC3 completes CI. Will restart jobs when able.

@mbed-ci

This comment has been minimized.

@cmonr

cmonr approved these changes Jun 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 12, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jun 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 13, 2018

@cmonr cmonr merged commit 6999d25 into ARMmbed:master Jun 13, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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/astyle Passed, 920 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9607 cycles (+778 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment