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

tests-mbed_hal-sleep fix #7036

Merged
merged 3 commits into from Jun 15, 2018

Conversation

@maciejbocianski
Member

maciejbocianski commented May 28, 2018

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 0xc0170 requested a review from ARMmbed/mbed-os-hal May 28, 2018

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

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:hal_ticker_feature_merging branch from 4fe81b5 to 074a59d May 28, 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

This comment has been minimized.

@0xc0170

0xc0170 May 29, 2018

Member

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

This comment has been minimized.

@0xc0170

0xc0170 May 29, 2018

Member

To remove this, we need to have fix for MBED_ALL_STATS_ENABLED

@maciejbocianski Please provide details

This comment has been minimized.

@maciejbocianski

maciejbocianski May 29, 2018

Member

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

This comment has been minimized.

@bulislaw

bulislaw May 29, 2018

Member

Could we just unset the flag instead?

This comment has been minimized.

@maciejbocianski

maciejbocianski May 30, 2018

Member

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)

This comment has been minimized.

@0xc0170

0xc0170 May 30, 2018

Member

@studavekar Please review

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 29, 2018

@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?

@0xc0170 0xc0170 requested review from studavekar and SenRamakri May 29, 2018

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented May 29, 2018

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

@cmonr cmonr added the do not merge label May 30, 2018

@cmonr

Blocking until internal conversation has been resolved.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 30, 2018

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.

@cmonr cmonr dismissed stale reviews from 0xc0170 and themself May 30, 2018

Internal discussion appears complete

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented May 31, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

@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

This comment has been minimized.

Member

maciejbocianski commented May 31, 2018

#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

This comment has been minimized.

Contributor

deepikabhavnani commented May 31, 2018

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

Closing #7063

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:hal_ticker_feature_merging branch from 468b653 to 0504034 Jun 1, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 4, 2018

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.

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;

This comment has been minimized.

@c1728p9

c1728p9 Jun 4, 2018

Contributor

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.

This comment has been minimized.

@maciejbocianski

maciejbocianski Jun 4, 2018

Member

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);

This comment has been minimized.

@0xc0170

0xc0170 Jun 4, 2018

Member

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

@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 maciejbocianski force-pushed the maciejbocianski:hal_ticker_feature_merging branch from 0504034 to 48b9ad9 Jun 4, 2018

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Jun 4, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 7, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 7, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 7, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 8, 2018

/morph build

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 9, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 9, 2018

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

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 11, 2018

Looks like this has some real build failures.

fix and enable sleep_usticker_test
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 maciejbocianski dismissed stale reviews from 0xc0170 and fkjagodzinski via 53548e2 Jun 11, 2018

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:hal_ticker_feature_merging branch from 2d9a48f to 53548e2 Jun 11, 2018

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Jun 11, 2018

fix was already pushed
there was lp_ticker usage without DEVICE_LPTICKER guard

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 12, 2018

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

This comment has been minimized.

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 14, 2018

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

This comment has been minimized.

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

@adbridge adbridge merged commit d66dfcb into ARMmbed:master Jun 15, 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 9681 cycles (+852 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