tests-mbed_hal-sleep fix #7036

maciejbocianski · 2018-05-28T13:02:27Z

Description

fix for deepsleep_lpticker_test (from tests-mbed_hal-sleep)
enable tests-mbed_hal-sleep test

Pull request type

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

0xc0170 · 2018-05-29T08:02:45Z

TESTS/mbed_hal/sleep/main.cpp

@@ -105,6 +105,11 @@ void sleep_usticker_test()

    const ticker_irq_handler_type us_ticker_irq_handler_org = set_us_ticker_irq_handler(us_ticker_isr);

+    // call ticker_read_us to initialize ticker upper layer


looking at the lines above, there's if #0 - should also be removed

To remove this, we need to have fix for MBED_ALL_STATS_ENABLED

@maciejbocianski Please provide details

When MBED_ALL_STATS_ENABLED is enable then sleep_manager_sleep_auto function calls some extra logging code, what makes return from sleep takes more time than expected

To enable sleep_usticker_test test
We could disable this flag globally for all test, or create mechanism to selectively mark tests which should be running without extra log/stats flags

Could we just unset the flag instead?

impossible to do, this flag is set globally for all tests

Jenkins code:
mbed_test_build_cmd=(mbed test --compile -m ${target} -t ${toolchain} -DMBED_HEAP_STATS_ENABLED=1 -DMBED_STACK_STATS_ENABLED=1 -DMBED_TRAP_ERRORS_ENABLED=1 -DMBED_ALL_STATS_ENABLED)

@studavekar Please review

0xc0170 · 2018-05-29T09:03:49Z

@studavekar @OPpuolitaival @SenRamakri Please review #7036 (comment)

Having all stats on might cause some time increase. In this case, as we expect a target to be running back from sleep in a specific period of time, adding extra stats adds delay and causing our tests to be failing.

I provide an example: a target from sleep should be running lets say <10us. If we enable stats, and they affect this functionality (adding some computation to the sleep functionality), this increases the time to be awake from 10us to higher numbers - not for all targets. This is a grey area (I dont think we got documented how stats affects functionality - shall we ?). If I know that lets say CPU stats can increase sleep times, then this should be stated in the documentation. How shall we fix this type of test?

OPpuolitaival · 2018-05-29T13:09:36Z

@0xc0170 I am not enough familiar with this that I can review

cmonr

Blocking until internal conversation has been resolved.

cmonr · 2018-05-30T22:06:57Z

After internal discussion, the CI flag will remain, and a mechanism will be added to bypass the delay causes by printing stats. This PR will need to be rebased once #7063 is in.

Internal discussion appears complete

maciejbocianski · 2018-05-31T07:08:19Z

In my opinion this PR should be merged first.
It contains fix for sleep test, that #7063 enables without fixing it

cmonr · 2018-05-31T16:02:03Z

@maciejbocianski Making sure I understand correctly. I thought that #7063 was going to add a mechanism to bypass the additional time delay, and then this PR would use that mechanism to run the test with proper timings.

Is this not correct?

maciejbocianski · 2018-05-31T16:49:18Z

#7063 main purpose is to fix sleep_usticker_test but it additionally turns on whole tests-mbed_hal-sleep test, but deepsleep_lpticker_test requires fix which is in #7036

sleep_usticker_test is disabled here, so can be merged before #7063

deepikabhavnani · 2018-05-31T18:42:20Z

@maciejbocianski @cmonr - Added changes from #7063 in this PR itself will save us some time in CI and sequencing.

Closing #7063

0xc0170 · 2018-06-04T09:51:26Z

This fixes the problem (the commit 0504034), not on board with the adding new hook to just overwrite the read functionality - having stats on but just return 0. Flag MBED_CPU_STATS_ENABLED vs this new function?
Both have the same effect.

The use for this API? One use case we have when a test is checking time requirement that stats break.

c1728p9 · 2018-06-04T14:10:18Z

TESTS/mbed_hal/sleep/main.cpp

 utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
 {
    GREENTEA_SETUP(60, "default_auto");
    us_ticker_init();
 #if DEVICE_LPTICKER
    lp_ticker_init();
 #endif
+    mbed_stats_ticker_read = disable_stats_read;


Rather than disabling the stats ticker, would it be possible the test to gracefully handle this case? This could be done either by decreasing the tolerance as in #7070 or by increasing delay time so the overhead of the stats read is proportionately less.

Can be done, delta has to be increased as follows (additional 10-15us to cover worst case: nrf51):

/* Used for regular sleep mode, a target should be awake within 10 us. Define us delta value as follows: * delta = default 10 us + worst ticker resolution + extra time for code execution + extra time for sleep time logging*/ static const uint32_t sleep_mode_delta_us = (10 + 4 + 5 + 10);

Looks good for now (I still would expect one day to test 10us period - no logging included - "release")

cmonr · 2018-06-04T15:53:00Z

@maciejbocianski Go ahead with the tolerance change, and once this PR is updated and re-approved, we can start CI.

As a reminder, we need to get this in before #7073 can progress, and we need it in before we can generate RC2.

maciejbocianski · 2018-06-04T16:19:20Z

@deepika's changes has been removed
tolerance value was increased for sleep_usticker_test to cover extra time needed for cpu stats computation

OPpuolitaival · 2018-06-07T13:16:15Z

build retriggered

cmonr · 2018-06-07T14:32:57Z

Looks like build completed successfully, but didn't report back. Going to retrigger.

cmonr · 2018-06-07T14:33:00Z

/morph build

cmonr · 2018-06-07T16:43:28Z

Halting CI builds until RC3 PRs are completed. Will resume after.

0xc0170 · 2018-06-08T08:15:54Z

/morph build

0xc0170 · 2018-06-09T06:42:27Z

/morph build

mbed-ci · 2018-06-09T10:47:11Z

Build : FAILURE

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

kjbracey · 2018-06-11T07:39:43Z

Looks like this has some real build failures.

Increases tolerance value for sleep_usticker_test to cover extra time needed for cpu stats computation (for more details see MBED_CPU_STATS_ENABLED). Prevent scheduling interrupt during ticker initialization (in lp_ticker_init) while test execution.

maciejbocianski · 2018-06-11T08:09:07Z

fix was already pushed
there was lp_ticker usage without DEVICE_LPTICKER guard

0xc0170 · 2018-06-12T11:10:24Z

/morph build

mbed-ci · 2018-06-12T12:09:12Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-06-12T14:03:28Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-06-13T21:05:59Z

Test : FAILURE

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

cmonr · 2018-06-14T02:34:05Z

Going to restart this test since the NRF51 appears to have timed out quite a bit, but there might be legitimate errors with the K82F and KW41Z building with GCC ('Low power ticker frequency test' failed for both targets).

/morph test

mbed-ci · 2018-06-15T06:54:48Z

Test : SUCCESS

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

0xc0170 requested a review from a team May 28, 2018 13:03

0xc0170 added needs: review release-version: 5.9.0-rc2 labels May 28, 2018

maciejbocianski changed the title ~~Hal ticker feature merging~~ tests-mbed_hal-sleep fix May 28, 2018

maciejbocianski force-pushed the hal_ticker_feature_merging branch from 4fe81b5 to 074a59d Compare May 28, 2018 16:54

0xc0170 previously requested changes May 29, 2018

View reviewed changes

0xc0170 requested review from studavekar and SenRamakri May 29, 2018 09:04

cmonr added the do not merge label May 30, 2018

cmonr previously requested changes May 30, 2018

View reviewed changes

deepikabhavnani mentioned this pull request May 30, 2018

Stats - Update ticker read mechanism #7063

Closed

cmonr added needs: preceding PR and removed do not merge labels May 30, 2018

maciejbocianski mentioned this pull request May 31, 2018

NRF52_DK: SLEEP enable #7073

Merged

maciejbocianski force-pushed the hal_ticker_feature_merging branch from 468b653 to 0504034 Compare June 1, 2018 11:46

cmonr removed the needs: preceding PR label Jun 4, 2018

c1728p9 reviewed Jun 4, 2018

View reviewed changes

maciejbocianski force-pushed the hal_ticker_feature_merging branch from 0504034 to 48b9ad9 Compare June 4, 2018 16:15

kjbracey added needs: work and removed needs: CI labels Jun 11, 2018

fix and enable sleep_usticker_test

53548e2

Increases tolerance value for sleep_usticker_test to cover extra time needed for cpu stats computation (for more details see MBED_CPU_STATS_ENABLED). Prevent scheduling interrupt during ticker initialization (in lp_ticker_init) while test execution.

maciejbocianski dismissed stale reviews from 0xc0170 and fkjagodzinski via 53548e2 June 11, 2018 08:07

maciejbocianski force-pushed the hal_ticker_feature_merging branch from 2d9a48f to 53548e2 Compare June 11, 2018 08:07

kjbracey added needs: review and removed needs: work labels Jun 11, 2018

0xc0170 approved these changes Jun 11, 2018

View reviewed changes

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

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

adbridge merged commit d66dfcb into ARMmbed:master Jun 15, 2018

0xc0170 removed the ready for merge label Jun 15, 2018

		@@ -105,6 +105,11 @@ void sleep_usticker_test()

		const ticker_irq_handler_type us_ticker_irq_handler_org = set_us_ticker_irq_handler(us_ticker_isr);

		// call ticker_read_us to initialize ticker upper layer

tests-mbed_hal-sleep fix #7036

tests-mbed_hal-sleep fix #7036

Uh oh!

Conversation

maciejbocianski commented May 28, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OPpuolitaival commented May 29, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented May 30, 2018

Uh oh!

maciejbocianski commented May 31, 2018

Uh oh!

cmonr commented May 31, 2018

Uh oh!

maciejbocianski commented May 31, 2018

Uh oh!

deepikabhavnani commented May 31, 2018

Uh oh!

0xc0170 commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jun 4, 2018

Uh oh!

maciejbocianski commented Jun 4, 2018

Uh oh!

OPpuolitaival commented Jun 7, 2018

Uh oh!

cmonr commented Jun 7, 2018

Uh oh!

cmonr commented Jun 7, 2018

Uh oh!

cmonr commented Jun 7, 2018

Uh oh!

0xc0170 commented Jun 8, 2018

Uh oh!

0xc0170 commented Jun 9, 2018

Uh oh!

mbed-ci commented Jun 9, 2018

Uh oh!

kjbracey commented Jun 11, 2018

Uh oh!

maciejbocianski commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jun 12, 2018

Uh oh!

mbed-ci commented Jun 12, 2018

Triggering tests

Uh oh!

mbed-ci commented Jun 12, 2018

Uh oh!

maciejbocianski May 29, 2018 •

edited

Loading

0xc0170 commented May 29, 2018 •

edited

Loading

maciejbocianski Jun 4, 2018 •

edited

Loading

maciejbocianski commented Jun 11, 2018 •

edited

Loading