Add Timeout rescheduling test #12942
Conversation
The `Timeout` drift test uses rescheduling inside a callback, but it is currently disabled due to lack of stability. Rescheduling using relative timeouts inside the callback is a bad technique as it leads to drift, so I understand the test being disabled. It is better to use `Ticker` for a periodic callback or to use `Timeout::attach_absolute`. Add a simpler test that just ensures the callback is called repeatedly - this test would detect issue ARMmbed#12940, and should not have stability problems.
Now tests only the Chrono `attach(duration)` method, not the deprecated `attach` and `attach_us` calls.
|
@kjbracey-arm, thank you for your changes. |
|
Tests FAILED on top of master |
|
Thanks @jeromecoutant for testing |
The second commit is just switching from testing deprecated APIs to testing Chrono APIs. Not changing which functionality is tested. The unreliable drifting test is still there with the ifdef. Can make them separate PRs if you like. |
|
understood, do not use |
|
CI started |
Test run: SUCCESSSummary: 4 of 4 test jobs passed |
0xc0170
added
needs: review
release-version: 6.0.0-rc1
and removed
ready for merge
labels
|
@ARMmbed/mbed-os-core Please review |
|
Again does this actually need to go into 6.0 or can it go into the next release? |
|
No urgency at all. It's just an accompanying test for #12941, which didn't make it due to the rush to get that fix in. |
|
As this has passed the CI we might as well take it in |
Summary of changes
The
Timeoutdrift test uses rescheduling inside a callback, but it is currently disabled due to lack of stability. Rescheduling using relative timeouts inside the callback is a bad technique as it leads to drift, so I understand the test being disabled. It is better to useTickerfor a periodic callback or to useTimeout::attach_absolute.Add a simpler test that just ensures the callback is called repeatedly - this test would detect issue #12940, and should not have stability problems.
Convert Timeout test to Chrono
Now tests only the Chrono
attach(duration)method, not the deprecatedattachandattach_uscalls.Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers