Skip to content
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

Correct core RTOS sleep routine timing #12938

Merged
merged 1 commit into from
May 7, 2020
Merged

Correct core RTOS sleep routine timing #12938

merged 1 commit into from
May 7, 2020

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented May 7, 2020

Summary of changes

Chrono conversions inadvertantly changed the core timed sleep routine used by the RTOS idle to use OsTimer::update_and_get_tick() instead of OsTimer::get_tick().

Correct this, and expand/clarify documentation and naming to try to prevent recurrence.

Another minor fix observed while inspecting code - OsClock can't just use milliseconds, it should match the period of OsTimer, which theoretically can be different.

Fixes #12920.

Impact of changes

Migration actions required

Documentation

None.

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@JarkkoPaso, @jeromecoutant

@kjbracey
Correct core RTOS sleep routine timing
3f67eed 
Chrono conversions inadvertantly changed the core timed sleep routine
used by the RTOS idle to use `OsTimer::update_and_get_tick()` instead of
`OsTimer::get_tick()`.

Correct this, and expand/clarify documentation and naming to try to
prevent recurrence.

Another minor fix observed while inspecting code - `OsClock` can't just
use `milliseconds`, it should match the period of `OsTimer`, which
theoretically can be different.
@ciarmcom ciarmcom requested review from JarkkoPaso, jeromecoutant and a team May 7, 2020 09:00
@ciarmcom
Copy link
Member

ciarmcom commented May 7, 2020

@kjbracey-arm, thank you for your changes.
@JarkkoPaso @jeromecoutant @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@jeromecoutant
Copy link
Collaborator

Hi
I'm sorry but test is still failing:

[1588842154.66][CONN][RXD] >>> Running case #21: 'Timing drift (attach)'...
[1588842154.70][CONN][INF] found KV pair in stream: {{__testcase_start;Timing drift (attach)}}, queued...
[1588842154.73][CONN][INF] found KV pair in stream: {{timing_drift_check_start;0}}, queued...
[1588842154.75][SERI][TXD] {{base_time;0}}
[1588842154.80][CONN][INF] found KV pair in stream: {{base_time;20000}}, queued...
[1588842165.42][HTST][INF] Device base time 20000
[1588842165.42][HTST][INF] sleeping for 10.611820220947266 to measure drift accurately
[1588842165.44][SERI][TXD] {{final_time;0}}
[1588842165.48][CONN][INF] found KV pair in stream: {{final_time;20000}}, queued...
[1588842165.48][HTST][INF] Device final time 20000
[1588842165.48][HTST][INF] Compute host events
[1588842165.48][HTST][INF] Transport delay 0: 0.07014775276184082
[1588842165.48][HTST][INF] Transport delay 1: 0.060277462005615234
[1588842165.48][HTST][INF] DUT base time : 20000.0
[1588842165.48][HTST][INF] DUT end time  : 20000.0
[1588842165.48][HTST][INF] min_pass : 10.209194493293761 , max_pass : 11.146900069713594 for 5.0%%
[1588842165.48][HTST][INF] min_inconclusive : 10.085290539264678 , max_inconclusive : 11.283846545219422
[1588842165.48][HTST][INF] Time reported by device: 0.0
[1588842165.48][HTST][INF] Time outside of passing range. Timing drift seems to be present !!!
[1588842165.49][SERI][TXD] {{fail;0}}
mbedgt: :414::FAIL: Expected 'pass' Was 'fail'. Host script reported a failure
[1588842165.59][CONN][RXD] :414::FAIL: Expected 'pass' Was 'fail'. Host script reported a failure
[1588842165.64][CONN][INF] found KV pair in stream: {{__testcase_finish;Timing drift (attach);0;1}}, queued...
[1588842165.71][CONN][RXD] >>> 'Timing drift (attach)': 0 passed, 1 failed with reason 'Assertion Failed'

@kjbracey
Copy link
Contributor Author

kjbracey commented May 7, 2020 •
edited
Loading

Oh well. Not looked into your failures specifically yet, but was hoping it might be the same issue. Could you do me a favour and run a git bisect on the master/chrono-bisect branches on my fork? The topic branch isn't bisectable in the main tree, but I've made a bisectable version of that branch and its merge to master.

 git remote add kevin git@github.com:kjbracey-arm/mbed-os.git
 git remote update kevin
 git bisect start
 git bisect good kevin/master~
 git bisect bad kevin/master

and go through the bisection running the test.

(I'm only counting #12920 as the Wisun failure report - so far your issue is just a comment, so it might be about to get closed...)

I guess it couldn't have been the same issue, as you were seeing problems in non-tickless targets.

0xc0170
0xc0170 approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

TrivialClock is not trivial (me after reading the comments there).

@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2020

cc @kyle-cypress

@kjbracey
Copy link
Contributor Author

kjbracey commented May 7, 2020 •
edited
Loading

TrivialClock is not trivial (me after reading the comments there).

Tis a silly name. Trivial just means that its duration and time_point are simple types, and its now doesn't throw exceptions.

@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2020

Ci started

@JarkkoPaso
Copy link

Tested and fixes the original #12920 issue with Wi-SUN stack. So approval from me.

@jeromecoutant
Copy link
Collaborator

a140ddd is the first bad commit
commit a140ddd
Add Chrono support to Ticker et al

@jeromecoutant
Copy link
Collaborator

Note that tests are failing for DISCO_L475VG_IOT01A (tickless target) and NUCLEO_F429ZI (non tickless)

@mbed-ci
Copy link

mbed-ci commented May 7, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@kjbracey kjbracey mentioned this pull request May 7, 2020
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2020

I restarted travis to report a status back

@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2020

@jeromecoutant will work on finding the root case. Thanks for the bisect!

I'll merge this now.

@0xc0170 0xc0170 merged commit 7698b34 into ARMmbed:master May 7, 2020
@mergify mergify bot removed the ready for merge label May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@JarkkoPaso JarkkoPaso JarkkoPaso approved these changes

@jeromecoutant jeromecoutant Awaiting requested review from jeromecoutant

Assignees
No one assigned
Labels
release-version: 6.0.0-beta-1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timer issues with several targets
6 participants
@kjbracey @ciarmcom @jeromecoutant @0xc0170 @JarkkoPaso @mbed-ci