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

TimerEvent tests #5046

Merged
merged 4 commits into from Feb 26, 2018

Conversation

Projects
None yet
@fkjagodzinski
Member

fkjagodzinski commented Sep 7, 2017

Description

TimerEvent:

  • updated docs in the header file,
  • added unit tests,
  • added a fix from @0xc0170 (with an update from @maciejbocianski) for multiple boards failing to pass new test ceses, fixes #5284.

Status

READY

@bulislaw

In multiple tests we are checking internal state of other objects (not under test), I think that will lead to possible issues when the internals will change. I will say we should keep more to the APIs.

TESTS/mbed_drivers/timerevent/main.cpp Outdated
virtual ~TestTimerEvent() {
}
void insert(timestamp_t ts) {

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

Why are we overriding the calls without changing anything? Can't we use them directly from TimerEvent?

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 8, 2017

Member

I needed them to be publicly accessible for test purposes. I've found a more elegant way to achieve that, so I'll update this code anyway.

TESTS/mbed_drivers/timerevent/main.cpp Outdated
* Then @a ticker_queue is set properly
*/
template<typename T>
void test_ticker_queue(void) {

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

I would say this test goes to deep into the internals and tests other components. Problem with this approach is that internals can change and API should be stable. TimerEvent only calls Tickers API so I would only test it to this depth.

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 11, 2017

Member

Fixed -- after the update tests don't check any inernal structures.

TESTS/mbed_drivers/timerevent/main.cpp Outdated
T tte;
tte.sem_wait(0);
tte.set_future_timestamp(TEST_DELAY_US);

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

maybe we could try that just after the call the semaphore is not incremented

TESTS/mbed_drivers/timerevent/main.cpp Outdated
template<typename T>
void test_insert(void) {
T tte;
tte.sem_wait(0);

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

Why are we decrementing semaphore here? You can just initialise the semaphore with count of 0.

TESTS/mbed_drivers/timerevent/main.cpp Outdated
tte.insert_absolute(tte.ticker_read_us() - 1ULL);
int32_t sem_slots = tte.sem_wait(0);
TEST_ASSERT_EQUAL(1, sem_slots);

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 8, 2017

Member

Until #5051 is solved this assertion will fail on NUCLEO_F070RB & EFM32GG_STK3700.

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:timerevent_tests branch Sep 11, 2017

TESTS/mbed_drivers/timerevent/main.cpp Outdated
Case("Test remove after insert_absolute", test_remove<TestTimerEventAbsolute>),
Case("Test insert zero", test_insert_zero<TestTimerEventRelative>),
Case("Test insert_absolute zero", test_insert_zero<TestTimerEventAbsolute>),

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 11, 2017

Member

This test will fail on NUCLEO_F070RB and EFM32GG_STK3700 boards until #5051 is fixed.

TESTS/mbed_drivers/timerevent/main.cpp Outdated
Case("Test insert_absolute zero", test_insert_zero<TestTimerEventAbsolute>),
Case("Test insert timestamp from the past", test_insert_past<TestTimerEventRelative>),
Case("Test insert_absolute timestamp from the past", test_insert_past<TestTimerEventAbsolute>),

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 11, 2017

Member

This test will fail on NUCLEO_F070RB and EFM32GG_STK3700 boards until #5051 is fixed.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 25, 2017

@fkjagodzinski @bulislaw What's the status of this?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 28, 2017

bump

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Sep 29, 2017

[...] What's the status of this?

@theotherjimmy @0xc0170 @bulislaw
This PR has to wait for PR #5176, which fixes issue #5051.

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Oct 2, 2017

Ready for CI -- preceding PR #5176 merged.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 2, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 2, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1491

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 2, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 3, 2017

Result: ABORTED

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

/morph test-nightly

Output

mbed Build Number: 1497

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 5, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 6, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1533

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

Please look at failures, some of the new test cases fail

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Oct 6, 2017

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Oct 6, 2017

Test case named "Test insert_absolute zero" fails on several devices with an assertion:

[1507236309.66][CONN][RXD] >>> Running case #6: 'Test insert_absolute zero'...
[1507236309.71][CONN][INF] found KV pair in stream: {{__testcase_start;Test insert_absolute zero}}, queued...
[1507236309.74][CONN][RXD] :183::FAIL: Expected 1 Was 0

which means the event handler was not called instantly after event insertion.

@0xc0170
Looks like the issue #5051 affects more boards than initially listed. Shall I reopen it or report a new one?

I could only check NRF51_DK and that seems to be a different case -- the handler is called eventually, but with a delay (~91 us) which looks more like #5159.

To sum up, all failing boards don't comply with expected behaviour, which is:

Test insert_absolute zero
Given an instance of @a TimerEvent subclass
When a timestamp of 0 us is set with @a insert_absolute()
Then an event handler is called instantly
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

I looked at two devices, and could noticed some bugs in there. I can send a patch, but would like you to test them. I'll send a new PR shortly or can send you patch via email, you can ammend it here, to make tests pass. what do you think?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

@fkjagodzinski this commit fixes all failures in this PR - I dont have all devices with me now to retest, can you cherry-pick my commit here and we retest?

0xc0170@e514c9d

These failures emphasize how important tests are !

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:timerevent_tests branch Oct 6, 2017

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Oct 6, 2017

Update:

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

Please ammend the last commit, NRF5 should have

void us_ticker_fire_interrupt(void)
{
    uint32_t closest_safe_compare = common_rtc_32bit_ticks_get() + 2;

    nrf_rtc_cc_set(COMMON_RTC_INSTANCE, US_TICKER_CC_CHANNEL, RTC_WRAP(closest_safe_compare));
    nrf_rtc_event_enable(COMMON_RTC_INSTANCE, US_TICKER_INT_MASK);
}

I removed timestamp from the equation, should not be there

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:timerevent_tests branch Oct 6, 2017

0xc0170 and others added some commits Oct 6, 2017

us ticker: fix fire interrupt handling
Few targets need more than just pending IRQ set. They include some flags to be
set that are checked in IRQ handler. This is the case for targets in this
commit.

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:timerevent_tests branch from d006eaa to cd539e3 Feb 23, 2018

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Feb 23, 2018

Update:

Local tests show all is ok now; fingers crossed for the CI tests to pass this time. :)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 23, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 23, 2018

Build : SUCCESS

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

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.

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Feb 23, 2018

Looks like this PR is finally ready for merge! Unless anyone has comments of course, @0xc0170 @cmonr?

adbridge added a commit that referenced this pull request Feb 23, 2018

sleep_manager_racecondition: fix for slow devices
sleep_manager_racecondition test fix for devices with low CPU clock

This RP contains fix for sleep_manager_racecondition test
for very slow devices (like NRF51). It fixes the test itself
as well as side effects of fix introduced in
 #5046 (us ticker: fix fire interrupt handling)

The idea of the test was to test race condition between main thread
and interrupt handler calling the same function.
To efficiently test this, each handler call should interrupt
main thread to make race more likely.
On very slow devices (like NRF51) when we set very low ticker period
(e.g less then 1000us for NRF51) there is no much time for thread scheduling.
On such slow devices, setting period to 500 us cause that
main thread is scheduled very rarely and only handler is
constantly called making test unreliable.
Fix introduced in #5046 (us ticker: fix fire interrupt handling)
changed fire_interrupt function implementation causing more
interrupt tailing thus even less time for main thread scheduling.
After introduction of #5046 (us ticker: fix fire interrupt handling)
when running sleep_manager_racecondition test on NRF51
(with ticker1.attach_us(&sleep_manager_locking_irq_test, 500);)
test is failing with timeout due to the fact that interrupt
handler is constantly called and main thread is never scheduled.

cmonr added a commit that referenced this pull request Feb 23, 2018

sleep_manager_racecondition: fix for slow devices
sleep_manager_racecondition test fix for devices with low CPU clock

This RP contains fix for sleep_manager_racecondition test
for very slow devices (like NRF51). It fixes the test itself
as well as side effects of fix introduced in
 #5046 (us ticker: fix fire interrupt handling)

The idea of the test was to test race condition between main thread
and interrupt handler calling the same function.
To efficiently test this, each handler call should interrupt
main thread to make race more likely.
On very slow devices (like NRF51) when we set very low ticker period
(e.g less then 1000us for NRF51) there is no much time for thread scheduling.
On such slow devices, setting period to 500 us cause that
main thread is scheduled very rarely and only handler is
constantly called making test unreliable.
Fix introduced in #5046 (us ticker: fix fire interrupt handling)
changed fire_interrupt function implementation causing more
interrupt tailing thus even less time for main thread scheduling.
After introduction of #5046 (us ticker: fix fire interrupt handling)
when running sleep_manager_racecondition test on NRF51
(with ticker1.attach_us(&sleep_manager_locking_irq_test, 500);)
test is failing with timeout due to the fact that interrupt
handler is constantly called and main thread is never scheduled.

@cmonr cmonr merged commit 3d37d81 into ARMmbed:master Feb 26, 2018

18 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
mbed-ci-generic Build finished.
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-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 26, 2018

@fkjagodzinski fkjagodzinski deleted the fkjagodzinski:timerevent_tests branch Feb 28, 2018

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Mar 6, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Mar 6, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Mar 26, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Mar 29, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Apr 4, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Apr 10, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Apr 19, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

davidsaada added a commit to davidsaada/mbed-os that referenced this pull request May 8, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

davidsaada added a commit to davidsaada/mbed-os that referenced this pull request May 8, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046

kegilbert added a commit to kegilbert/mbed-os that referenced this pull request May 17, 2018

NRF52: fix us_ticker IRQ handler
Applying PR to NRF52 SDK 14.2:
ARMmbed#5046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment