Skip to content

tests-mbed_drivers-lp_timer: change delay method #7471

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

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Jul 11, 2018

Description

The test sometimes fails on NRF51_DK (test case: "Test: LowPowerTimer - time measurement 1 ms.") in morph tests.

The test verifies if LowPowerTimer class correctly counts elapsed time. Sometimes we got measured ~1600us for delay 1000 us (delta 550 us).
The delay is performed using wait_us() function which for delays greater than or equal to 1 ms (our case) calls Thread::wait((uint32_t)ms);. This causes rescheduling and potentially can put board into sleep (deep sleep mode is disabled by wait_us()). For our test purposes we don't need rescheduling/sleep since this actions takes extra time and have influence on the time measurement accuracy.
The solution is to implement function for delay which is based on busy loop and uses us ticker. It has been verified that this solves the problem. With this fix when test case is repeated 1000 times we got usually measured time equal to ~1080 us, and sometimes ~1300us (checked that this is caused by systick interrupt handling). Since this is test for drivers layer and the results are acceptable I decided to not disabling systick in the test).

Pull request type

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

The test sometimes fails on NRF51_DK (test case: "Test: LowPowerTimer - time measurement 1 ms.") in morph tests.

The test verifies if LowPowerTimer class correctly counts elapsed time. Sometimes we got measured ~1600 us for delay 1000 us (delta 550 us).
The delay is performed using `wait_us()` function which for delays greater than or equal to 1 ms (our case) calls `Thread::wait((uint32_t)ms);`. This causes rescheduling and potentially can put board into sleep (deep sleep mode is disabled by `wait_us()`). For our test purposes we don't need rescheduling/sleep since this actions takes extra time and have influence on the time measurement accuracy.
The solution is to implement function for delay which is based on busy loop and uses us ticker. It has been verified that this solves the problem. With this fix when measurement of 1 ms is repeated 1000 times we got usually measured time equal to ~1080 us, and sometimes ~1300us (checked that this is caused by systick interrupt handling). Since this is test for drivers layer and the results are acceptable I decided to not disabling systick in the test).
@mprse mprse mentioned this pull request Jul 11, 2018
@0xc0170 0xc0170 requested a review from a team July 11, 2018 11:18
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@cmonr
Copy link
Contributor

cmonr commented Jul 11, 2018

👍 👍 👍 For the description and commit message!

I'm good with the proposed fix and implementation.

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 11, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Jul 11, 2018

@mbed-ci
Copy link

mbed-ci commented Jul 12, 2018

Test : FAILURE

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

@jeromecoutant
Copy link
Collaborator

Hi
I checked the test failure issue.
I think this execution has been done after #7365 tests, and build before #7365 merge...
I suggest to restart morph build
Regards,

@mprse
Copy link
Contributor Author

mprse commented Jul 12, 2018

I'm confused with the result.
It looks like this change causes problem on NUCLEO_F746ZG. We have the following failed tests:

ARM tests-mbed_hal-lp_ticker (lp ticker sleep test)
tests-mbed_hal-sleep (deep-sleep - source of wake-up - lp ticker)
tests-mbed_drivers-rtc (Unit Test: attach stub RTC functions.)
tests-mbed_hal-rtc (RTC - init)
GCC_ARM tests-mbed_drivers-lp_timeout (1 s delay during deepsleep (attach))
IAR tests-mbed_hal-lp_ticker (lp ticker sleep test)
tests-mbed_hal-sleep (deep-sleep - source of wake-up - lp ticker)
tests-mbed_hal-rtc_reset (tests-mbed_hal-rtc_reset)
tests-mbed_drivers-lp_timeout (1 s delay during deepsleep (attach))

This PR only changes tests-mbed_drivers-lp_timer and it looks like patch works since this test has passed on all targets/compilers.
It looks like there is some problem with sleep and rtc on NUCLEO_F746ZG. Can we run /morph test again to check if the results are valid?

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 12, 2018

/morph test

@jeromecoutant
Copy link
Collaborator

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Jul 12, 2018

@cmonr
Copy link
Contributor

cmonr commented Jul 12, 2018

Exporter left too soon
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jul 12, 2018

@mbed-ci
Copy link

mbed-ci commented Jul 12, 2018

@cmonr cmonr merged commit 1cbfa00 into ARMmbed:master Jul 13, 2018
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
tests-mbed_drivers-lp_timer: change delay method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants