From 25fad5d2a36eaa250cdf015508daebb25964db40 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Thu, 7 May 2020 17:17:55 +0300 Subject: [PATCH 1/2] Add Timeout rescheduling test 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 #12940, and should not have stability problems. --- TESTS/mbed_drivers/lp_timeout/main.cpp | 3 +++ TESTS/mbed_drivers/timeout/main.cpp | 3 +++ TESTS/mbed_drivers/timeout/timeout_tests.h | 21 +++++++++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/TESTS/mbed_drivers/lp_timeout/main.cpp b/TESTS/mbed_drivers/lp_timeout/main.cpp index 3d69ba22ed..696e2ce10d 100644 --- a/TESTS/mbed_drivers/lp_timeout/main.cpp +++ b/TESTS/mbed_drivers/lp_timeout/main.cpp @@ -52,6 +52,9 @@ Case cases[] = { Case("Zero delay (attach)", test_no_wait >), Case("Zero delay (attach_us)", test_no_wait >), + Case("Reschedule in callback (attach)", test_reschedule >), + Case("Reschedule in callback (attach_us)", test_reschedule >), + Case("10 ms delay accuracy (attach)", test_delay_accuracy, 10000, SHORT_DELTA_US>, greentea_failure_handler), Case("10 ms delay accuracy (attach_us)", test_delay_accuracy, 10000, SHORT_DELTA_US>, diff --git a/TESTS/mbed_drivers/timeout/main.cpp b/TESTS/mbed_drivers/timeout/main.cpp index 9ea301626d..cbf7b264dc 100644 --- a/TESTS/mbed_drivers/timeout/main.cpp +++ b/TESTS/mbed_drivers/timeout/main.cpp @@ -48,6 +48,9 @@ Case cases[] = { Case("Zero delay (attach)", test_no_wait >), Case("Zero delay (attach_us)", test_no_wait >), + Case("Reschedule in callback (attach)", test_reschedule >), + Case("Reschedule in callback (attach_us)", test_reschedule >), + Case("10 ms delay accuracy (attach)", test_delay_accuracy, 10000, SHORT_DELTA_US>, greentea_failure_handler), Case("10 ms delay accuracy (attach_us)", test_delay_accuracy, 10000, SHORT_DELTA_US>, diff --git a/TESTS/mbed_drivers/timeout/timeout_tests.h b/TESTS/mbed_drivers/timeout/timeout_tests.h index 98b7886261..27d29f8249 100644 --- a/TESTS/mbed_drivers/timeout/timeout_tests.h +++ b/TESTS/mbed_drivers/timeout/timeout_tests.h @@ -363,6 +363,27 @@ private: TimeoutTesterType _timeout; }; +/** Template for tests: rescheduling + * + * Given a Timeout object + * When a callback is attached with that reattaches itself, with @a attach() + * Then the callback is called repeatedly + * + * Given a Timeout object + * When a callback is attached with that reattaches itself, with @a attach_us() + * Then the callback is called repeatedly + */ +template +void test_reschedule(void) +{ + volatile uint32_t callback_count = 0; + TimeoutDriftTester timeout; + + timeout.reschedule_callback(); + ThisThread::sleep_for(TEST_DELAY_MS * 5); + TEST_ASSERT(timeout.get_callback_count() >= 3); +} + /** Template for tests: accuracy of timeout delay scheduled repeatedly * * Test time drift -- device part From 8a5f58bf553d62bf1756b79d57d7b490f0219619 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Thu, 7 May 2020 17:40:06 +0300 Subject: [PATCH 2/2] Convert Timeout test to Chrono Now tests only the Chrono `attach(duration)` method, not the deprecated `attach` and `attach_us` calls. --- TESTS/mbed_drivers/lp_timeout/main.cpp | 41 +++-------- TESTS/mbed_drivers/timeout/main.cpp | 37 +++------- TESTS/mbed_drivers/timeout/timeout_tests.h | 85 +++++++++------------- 3 files changed, 56 insertions(+), 107 deletions(-) diff --git a/TESTS/mbed_drivers/lp_timeout/main.cpp b/TESTS/mbed_drivers/lp_timeout/main.cpp index 696e2ce10d..30f303553c 100644 --- a/TESTS/mbed_drivers/lp_timeout/main.cpp +++ b/TESTS/mbed_drivers/lp_timeout/main.cpp @@ -37,54 +37,37 @@ utest::v1::status_t greentea_failure_handler(const Case *const source, const fai } Case cases[] = { - Case("Callback called once (attach)", test_single_call >), - Case("Callback called once (attach_us)", test_single_call >), + Case("Callback called once", test_single_call), - Case("Callback not called when cancelled (attach)", test_cancel >), - Case("Callback not called when cancelled (attach_us)", test_cancel >), + Case("Callback not called when cancelled", test_cancel), - Case("Callback override (attach)", test_override >), - Case("Callback override (attach_us)", test_override >), + Case("Callback override", test_override), - Case("Multiple timeouts running in parallel (attach)", test_multiple >), - Case("Multiple timeouts running in parallel (attach_us)", test_multiple >), + Case("Multiple timeouts running in parallel", test_multiple), - Case("Zero delay (attach)", test_no_wait >), - Case("Zero delay (attach_us)", test_no_wait >), + Case("Zero delay", test_no_wait), - Case("Reschedule in callback (attach)", test_reschedule >), - Case("Reschedule in callback (attach_us)", test_reschedule >), + Case("Reschedule in callback", test_reschedule), - Case("10 ms delay accuracy (attach)", test_delay_accuracy, 10000, SHORT_DELTA_US>, - greentea_failure_handler), - Case("10 ms delay accuracy (attach_us)", test_delay_accuracy, 10000, SHORT_DELTA_US>, + Case("10 ms delay accuracy", test_delay_accuracy, greentea_failure_handler), - Case("1 s delay accuracy (attach)", test_delay_accuracy, 1000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("1 s delay accuracy (attach_us)", test_delay_accuracy, 1000000, LONG_DELTA_US>, + Case("1 s delay accuracy (attach)", test_delay_accuracy, greentea_failure_handler), - Case("5 s delay accuracy (attach)", test_delay_accuracy, 5000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("5 s delay accuracy (attach_us)", test_delay_accuracy, 5000000, LONG_DELTA_US>, + Case("5 s delay accuracy (attach)", test_delay_accuracy, greentea_failure_handler), #if DEVICE_SLEEP - Case("1 s delay during sleep (attach)", test_sleep, 1000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("1 s delay during sleep (attach_us)", test_sleep, 1000000, LONG_DELTA_US>, + Case("1 s delay during sleep (attach)", test_sleep, greentea_failure_handler), - Case("1 s delay during deepsleep (attach)", test_deepsleep, 1000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("1 s delay during deepsleep (attach_us)", test_deepsleep, 1000000, LONG_DELTA_US>, + Case("1 s delay during deepsleep (attach)", test_deepsleep, greentea_failure_handler), #endif #if !defined(SKIP_TIME_DRIFT_TESTS) - Case("Timing drift (attach)", test_drift >), - Case("Timing drift (attach_us)", test_drift >), + Case("Timing drift (attach)", test_drift), #endif }; diff --git a/TESTS/mbed_drivers/timeout/main.cpp b/TESTS/mbed_drivers/timeout/main.cpp index cbf7b264dc..38bc095cfb 100644 --- a/TESTS/mbed_drivers/timeout/main.cpp +++ b/TESTS/mbed_drivers/timeout/main.cpp @@ -33,49 +33,34 @@ utest::v1::status_t greentea_failure_handler(const Case *const source, const fai } Case cases[] = { - Case("Callback called once (attach)", test_single_call >), - Case("Callback called once (attach_us)", test_single_call >), + Case("Callback called once", test_single_call), - Case("Callback not called when cancelled (attach)", test_cancel >), - Case("Callback not called when cancelled (attach_us)", test_cancel >), + Case("Callback not called when cancelled", test_cancel), - Case("Callback override (attach)", test_override >), - Case("Callback override (attach_us)", test_override >), + Case("Callback override", test_override), - Case("Multiple timeouts running in parallel (attach)", test_multiple >), - Case("Multiple timeouts running in parallel (attach_us)", test_multiple >), + Case("Multiple timeouts running in parallel", test_multiple), - Case("Zero delay (attach)", test_no_wait >), - Case("Zero delay (attach_us)", test_no_wait >), + Case("Zero delay", test_no_wait), - Case("Reschedule in callback (attach)", test_reschedule >), - Case("Reschedule in callback (attach_us)", test_reschedule >), + Case("Reschedule in callback", test_reschedule), - Case("10 ms delay accuracy (attach)", test_delay_accuracy, 10000, SHORT_DELTA_US>, - greentea_failure_handler), - Case("10 ms delay accuracy (attach_us)", test_delay_accuracy, 10000, SHORT_DELTA_US>, + Case("10 ms delay accuracy", test_delay_accuracy, greentea_failure_handler), - Case("1 s delay accuracy (attach)", test_delay_accuracy, 1000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("1 s delay accuracy (attach_us)", test_delay_accuracy, 1000000, LONG_DELTA_US>, + Case("1 s delay accuracy", test_delay_accuracy, greentea_failure_handler), - Case("5 s delay accuracy (attach)", test_delay_accuracy, 5000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("5 s delay accuracy (attach_us)", test_delay_accuracy, 5000000, LONG_DELTA_US>, + Case("5 s delay accuracy", test_delay_accuracy, greentea_failure_handler), #if DEVICE_SLEEP - Case("1 s delay during sleep (attach)", test_sleep, 1000000, LONG_DELTA_US>, - greentea_failure_handler), - Case("1 s delay during sleep (attach_us)", test_sleep, 1000000, LONG_DELTA_US>, + Case("1 s delay during sleep", test_sleep, greentea_failure_handler), #endif #if !defined(SKIP_TIME_DRIFT_TESTS) - Case("Timing drift (attach)", test_drift >), - Case("Timing drift (attach_us)", test_drift >), + Case("Timing drift", test_drift), #endif }; diff --git a/TESTS/mbed_drivers/timeout/timeout_tests.h b/TESTS/mbed_drivers/timeout/timeout_tests.h index 27d29f8249..f363deac45 100644 --- a/TESTS/mbed_drivers/timeout/timeout_tests.h +++ b/TESTS/mbed_drivers/timeout/timeout_tests.h @@ -20,12 +20,12 @@ #include "mbed.h" #include "unity/unity.h" -#define NUM_TIMEOUTS 16 -#define DRIFT_TEST_PERIOD_US 10000 +using namespace std::chrono; -const float TEST_DELAY_S = 0.01; -const uint32_t TEST_DELAY_MS = 1000.0F * TEST_DELAY_S; -const us_timestamp_t TEST_DELAY_US = 1000000.0F * TEST_DELAY_S; +#define NUM_TIMEOUTS 16 +const microseconds DRIFT_TEST_PERIOD = 10ms; + +const milliseconds TEST_DELAY = 10ms; /* Timeouts are quite arbitrary due to large number of boards with varying level of accuracy */ #define LONG_DELTA_US (100000) @@ -41,24 +41,6 @@ void cnt_callback(volatile uint32_t *cnt) (*cnt)++; } -template -class AttachTester: public TimeoutType { -public: - void attach_callback(Callback func, us_timestamp_t delay_us) - { - TimeoutType::attach(func, (float) delay_us / 1000000.0f); - } -}; - -template -class AttachUSTester: public TimeoutType { -public: - void attach_callback(Callback func, us_timestamp_t delay_us) - { - TimeoutType::attach_us(func, delay_us); - } -}; - /** Template for tests: callback called once * * Test callback called once @@ -77,15 +59,15 @@ void test_single_call(void) Semaphore sem(0, 1); T timeout; - timeout.attach_callback(mbed::callback(sem_callback, &sem), TEST_DELAY_US); + timeout.attach(mbed::callback(sem_callback, &sem), TEST_DELAY); bool acquired = sem.try_acquire(); TEST_ASSERT_FALSE(acquired); - acquired = sem.try_acquire_for(TEST_DELAY_MS + 2); + acquired = sem.try_acquire_for(TEST_DELAY + 2ms); TEST_ASSERT_TRUE(acquired); - acquired = sem.try_acquire_for(TEST_DELAY_MS + 2); + acquired = sem.try_acquire_for(TEST_DELAY + 2ms); TEST_ASSERT_FALSE(acquired); timeout.detach(); @@ -109,13 +91,13 @@ void test_cancel(void) Semaphore sem(0, 1); T timeout; - timeout.attach_callback(mbed::callback(sem_callback, &sem), 2.0f * TEST_DELAY_US); + timeout.attach(mbed::callback(sem_callback, &sem), 2 * TEST_DELAY); - bool acquired = sem.try_acquire_for(TEST_DELAY_MS); + bool acquired = sem.try_acquire_for(TEST_DELAY); TEST_ASSERT_FALSE(acquired); timeout.detach(); - acquired = sem.try_acquire_for(TEST_DELAY_MS + 2); + acquired = sem.try_acquire_for(TEST_DELAY + 2ms); TEST_ASSERT_FALSE(acquired); } @@ -142,13 +124,13 @@ void test_override(void) Semaphore sem2(0, 1); T timeout; - timeout.attach_callback(mbed::callback(sem_callback, &sem1), 2.0f * TEST_DELAY_US); + timeout.attach(mbed::callback(sem_callback, &sem1), 2 * TEST_DELAY); - bool acquired = sem1.try_acquire_for(TEST_DELAY_MS); + bool acquired = sem1.try_acquire_for(TEST_DELAY); TEST_ASSERT_FALSE(acquired); - timeout.attach_callback(mbed::callback(sem_callback, &sem2), 2.0f * TEST_DELAY_US); + timeout.attach(mbed::callback(sem_callback, &sem2), 2 * TEST_DELAY); - acquired = sem2.try_acquire_for(2 * TEST_DELAY_MS + 2); + acquired = sem2.try_acquire_for(2 * TEST_DELAY + 2ms); TEST_ASSERT_TRUE(acquired); acquired = sem1.try_acquire(); TEST_ASSERT_FALSE(acquired); @@ -176,9 +158,9 @@ void test_multiple(void) volatile uint32_t callback_count = 0; T timeouts[NUM_TIMEOUTS]; for (size_t i = 0; i < NUM_TIMEOUTS; i++) { - timeouts[i].attach_callback(mbed::callback(cnt_callback, &callback_count), TEST_DELAY_US); + timeouts[i].attach(mbed::callback(cnt_callback, &callback_count), TEST_DELAY); } - ThisThread::sleep_for(TEST_DELAY_MS + 2); + ThisThread::sleep_for(TEST_DELAY + 2ms); TEST_ASSERT_EQUAL(NUM_TIMEOUTS, callback_count); } @@ -200,7 +182,7 @@ void test_no_wait(void) for (int i = 0; i < 100; i++) { Semaphore sem(0, 1); T timeout; - timeout.attach_callback(mbed::callback(sem_callback, &sem), 0ULL); + timeout.attach(mbed::callback(sem_callback, &sem), 0s); bool sem_acquired = sem.try_acquire(); TEST_ASSERT_EQUAL(true, sem_acquired); timeout.detach(); @@ -227,11 +209,11 @@ void test_delay_accuracy(void) Timer timer; timer.start(); - timeout.attach_callback(mbed::callback(sem_callback, &sem), delay_us); + timeout.attach(mbed::callback(sem_callback, &sem), microseconds(delay_us)); sem.acquire(); timer.stop(); - TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.read_high_resolution_us()); + TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.elapsed_time().count()); timeout.detach(); } @@ -262,7 +244,7 @@ void test_sleep(void) sleep_manager_lock_deep_sleep(); timer.start(); - timeout.attach_callback(mbed::callback(sem_callback, &sem), delay_us); + timeout.attach(mbed::callback(sem_callback, &sem), microseconds(delay_us)); bool deep_sleep_allowed = sleep_manager_can_deep_sleep_test_check(); TEST_ASSERT_FALSE_MESSAGE(deep_sleep_allowed, "Deep sleep should be disallowed"); @@ -270,7 +252,7 @@ void test_sleep(void) timer.stop(); sleep_manager_unlock_deep_sleep(); - TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.read_high_resolution_us()); + TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.elapsed_time().count()); timeout.detach(); } @@ -316,17 +298,17 @@ void test_deepsleep(void) * hardware buffers are empty. However, such an API does not exist now, * so we'll use the ThisThread::sleep_for() function for now. */ - ThisThread::sleep_for(20); + ThisThread::sleep_for(20ms); timer.start(); - timeout.attach_callback(mbed::callback(sem_callback, &sem), delay_us); + timeout.attach(mbed::callback(sem_callback, &sem), microseconds(delay_us)); bool deep_sleep_allowed = sleep_manager_can_deep_sleep_test_check(); TEST_ASSERT_TRUE_MESSAGE(deep_sleep_allowed, "Deep sleep should be allowed"); sem.acquire(); timer.stop(); - TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.read_high_resolution_us()); + TEST_ASSERT_UINT64_WITHIN(delta_us, delay_us, timer.elapsed_time().count()); timeout.detach(); } @@ -336,14 +318,14 @@ void test_deepsleep(void) template class TimeoutDriftTester { public: - TimeoutDriftTester(us_timestamp_t period = 1000) : + TimeoutDriftTester(microseconds period = 1ms) : _callback_count(0), _period(period), _timeout() { } void reschedule_callback(void) { - _timeout.attach_callback(mbed::callback(this, &TimeoutDriftTester::reschedule_callback), _period); + _timeout.attach(mbed::callback(this, &TimeoutDriftTester::reschedule_callback), _period); _callback_count++; } @@ -359,7 +341,7 @@ public: private: volatile uint32_t _callback_count; - us_timestamp_t _period; + microseconds _period; TimeoutTesterType _timeout; }; @@ -376,11 +358,10 @@ private: template void test_reschedule(void) { - volatile uint32_t callback_count = 0; - TimeoutDriftTester timeout; + TimeoutDriftTester timeout(TEST_DELAY); timeout.reschedule_callback(); - ThisThread::sleep_for(TEST_DELAY_MS * 5); + ThisThread::sleep_for(TEST_DELAY * 5); TEST_ASSERT(timeout.get_callback_count() >= 3); } @@ -410,7 +391,7 @@ void test_drift(void) char _key[11] = { }; char _value[128] = { }; int expected_key = 1; - TimeoutDriftTester timeout(DRIFT_TEST_PERIOD_US); + TimeoutDriftTester timeout(DRIFT_TEST_PERIOD); greentea_send_kv("timing_drift_check_start", 0); timeout.reschedule_callback(); @@ -421,11 +402,11 @@ void test_drift(void) expected_key = strcmp(_key, "base_time"); } while (expected_key); - greentea_send_kv(_key, timeout.get_callback_count() * DRIFT_TEST_PERIOD_US); + greentea_send_kv(_key, timeout.get_callback_count() * DRIFT_TEST_PERIOD.count()); // wait for 2nd signal from host greentea_parse_kv(_key, _value, sizeof(_key), sizeof(_value)); - greentea_send_kv(_key, timeout.get_callback_count() * DRIFT_TEST_PERIOD_US); + greentea_send_kv(_key, timeout.get_callback_count() * DRIFT_TEST_PERIOD.count()); timeout.detach_callback();