Skip to content

Stats - Update ticker read mechanism #7063

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Stats - Update ticker read mechanism #7063

wants to merge 1 commit into from

Conversation

deepikabhavnani
Copy link

Description

Stats collection in sleep affected other time critical tests, resulting into failures. Mechanism is added to disable stats collection, if required for sleep tests.

Pull request type

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

#7036

Add function pointer to ticker data for stats
0293619 
Function pointer will help in disabling extra logging if required
for other testing mechanism, or to change from low power ticker to
us ticker for testing
@deepikabhavnani
Copy link
Author

As per below information from @cmonr , test is updated and verified locally.

The test originally existed in a feature branch until it was brought in with the HAL omni-merge PR last weekend. Before then, the test had not been updated on the Master branch since late March: https://github.com/ARMmbed/mbed-os/commits/master/TESTS/mbed_hal/sleep/main.cpp

Four days ago, as a part of the HAL omni-merge PR, the test was disabled due to unstable performance: 817bf0b#diff-7d3d1c9063d94caad3b217e04e09fd05

bulislaw
bulislaw approved these changes May 30, 2018
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

@maciejbocianski please validate

@cmonr
Copy link
Contributor

cmonr commented May 30, 2018

/morph build

maciejbocianski
maciejbocianski approved these changes May 30, 2018
View reviewed changes
Copy link
Contributor

@maciejbocianski maciejbocianski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tested on NRF51_DK - works fine

tests-mbed_drivers-sleep_lock              | OK
tests-mbed_hal-sleep                       | OK
tests-mbed_hal-sleep_manager               | OK
tests-mbed_hal-sleep_manager_racecondition | OK

@cmonr cmonr mentioned this pull request May 30, 2018
Merged
@mbed-ci
Copy link

mbed-ci commented May 30, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 30, 2018

Test : FAILURE

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

@mbed-ci
Copy link

mbed-ci commented May 31, 2018

Exporter Build : SUCCESS

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

@maciejbocianski
Copy link
Contributor

maciejbocianski commented May 31, 2018 •
edited
Loading

tests-mbed_drivers-timer is failing randomly for NRF51_DK

1527720629.09][CONN][RXD] >>> Running case #3: 'Test: Timer (based on os ticker) - measured time accumulation.'... 
[1527720629.19][CONN][RXD] :409::FAIL: Expected 0.010000 Was 0.010769 [1527720629.19][CONN][INF] found KV pair in stream: {{__testcase_start;Test: Timer (based on os ticker) - measured

@ithinuel
Copy link
Member

ithinuel commented May 31, 2018 •
edited
Loading

This is due to the fact that the device does not have clock precise enough (NO_SYSTICK).
Is it ok to relax the precision requirement in such case ?
And if yes, how far can we go ?

EDIT: Note that the drift is not huge though : FAIL: Expected 0.010000 Was 0.010769
the current maximum tolerance given by DELTA_S(10) is 0.000700f second.

@adbridge
Copy link
Contributor

@bulislaw Can you comment on @ithinuel suggestion ?

@ithinuel ithinuel mentioned this pull request May 31, 2018
Merged
@maciejbocianski maciejbocianski mentioned this pull request May 31, 2018
Merged
@deepikabhavnani
Copy link
Author

Moved changes to PR #7036. Closing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bulislaw bulislaw bulislaw approved these changes

@maciejbocianski maciejbocianski maciejbocianski approved these changes

Assignees
No one assigned
Labels
needs: preceding PR needs: review release-version: 5.9.0-rc2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@deepikabhavnani @cmonr @mbed-ci @maciejbocianski @ithinuel @adbridge @bulislaw