Add watchdog lower limit timeout test #11203

Tharazi97 · 2019-08-12T14:38:12Z

Description

Currently there is no test that checks if watchdog on the device meets the assumption specified in docs: "The watchdog should trigger at or after the timeout value.".
This PR adds one to the base of watchdog reset tests.
It checks whether the watchdog did not restart the device before the window left to the end of timeout. If watchdog restarted the device, the test will time out.

Targets with not calibrated watchdog clock won't pass this test. This is the case for some STM MCU, default calibration is not accurate at all, so we should probably:

Change the watchdog API of these targets so it meets assumptions specified in docs.
Make this test optional (specify that these targets won't have their watchdog trigger at or after timeout, but in between half value of timeout to 2 times timeout value).
Relax assumptions.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@fkjagodzinski @0xc0170 @maciejbocianski @jamesbeyond

Release Notes

watchdog_features_t extended by two uint32_t members: clock_typical_frequency and clock_max_frequency. These values can be used to determine the accuracy of an uncalibrated watchdog clock.

ciarmcom · 2019-08-12T15:00:35Z

@Tharazi97, thank you for your changes.
@maciejbocianski @jamesbeyond @fkjagodzinski @0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2019-08-20T11:54:35Z

STM targets won't pass this test, so we should probably:
Change the STM watchdog API so it meets assumptions specified in docs.
Make this test optional (specify that STM targets won't have their watchdog trigger at or after timeout, but in between half value of timeout to 2 times timeout value).
Relax assumptions.

@Tharazi97 if we can fix it, would be great. Just need more details.
@ARMmbed/team-st-mcd Please review

0xc0170 · 2019-08-20T11:57:15Z

I reduced reviewers. Please review

Tharazi97 · 2019-08-20T12:20:56Z

@0xc0170
According to documentation watchdog should trigger at or after timeout value. (https://os.mbed.com/docs/mbed-os/v5.13/porting/watchdog-port.html)
STM targets have their watchdog clocked by LSI oscillator and its frequency may vary very much. So if we program watchdog to trigger at, or after 100 ms timeout it may physically trigger i. e. even after 70ms. So assumption that "The watchdog should trigger at or after the timeout value." is incorrect.

Test that I added checks if watchdog is physically triggering after the timeout value, if not it chcecks how close we are to this value.

Tharazi97 · 2019-08-20T12:25:09Z

We can change the assumption to: "The watchdog should trigger at or after the 75% timeout value.", or: "The watchdog should trigger at or after the timeout value (not applicable STM devices)."

jeromecoutant · 2019-08-20T12:46:56Z

STM targets won't pass this test

I would prefer to change this line...
Targets with not calibrated watchdog clock won't pass this test.
This is the case for some STM MCU, default calibration is not accurate at all.

Thx

jeromecoutant · 2019-08-20T13:42:19Z

Maybe we could add some frequency accuracy parameter in watchdog_features_t that test can check ?

Tharazi97 · 2019-08-20T13:54:07Z

@jeromecoutant
I think that could work. Add this parameter, change the test and change the assumption to: The watchdog should trigger at or after the timeout value times frequency accuracy. Or something like that. What do you think @jamesbeyond @fkjagodzinski?

fkjagodzinski · 2019-08-22T07:47:33Z

TESTS/mbed_drivers/watchdog_reset/Watchdog_reset_tests.h

@@ -64,6 +64,16 @@ void test_restart_reset();
 */
 void test_kick_reset();

+/** Test Watchdog timeout


Is there any reason to duplicate this test for the driver layer? As I see it, this test simply checks the actual hardware performance, right? Both HAL and driver APIs are verified by other test cases.

fkjagodzinski · 2019-08-22T07:49:48Z

TESTS/mbed_hal/watchdog_reset/main.cpp

@@ -264,6 +266,48 @@ void test_kick_reset()
    TEST_ASSERT_MESSAGE(0, "Watchdog did not reset the device as expected.");
 }

+template<uint32_t window>
+void test_timeout()


Since this is a timing test, I think it should be moved to TESTS/mbed_hal/watchdog_timing.

fkjagodzinski · 2019-08-22T08:01:35Z

TESTS/mbed_hal/watchdog_reset/main.cpp

@@ -42,6 +42,8 @@
 */
 #define KICK_ADVANCE_MS 35UL

+#define TIMEOUT_LOWER_LIMIT 1000UL


Please keep the convention of adding a unit suffix.

Suggested change

#define TIMEOUT_LOWER_LIMIT 1000UL

#define TIMEOUT_LOWER_LIMIT_MS 1000UL

fkjagodzinski · 2019-08-22T08:26:48Z

TESTS/mbed_hal/watchdog_reset/main.cpp

@@ -310,6 +354,12 @@ Case cases[] = {
 #endif
    Case("Watchdog started again", case_setup, test_restart_reset),
    Case("Kicking the Watchdog prevents reset", case_setup, test_kick_reset),
+    Case("timeout accuracy 20%", case_setup, test_timeout<200UL>),


As we know, for some targets these tests will fail for sure if the watchdog clock is left uncalibrated. I think we should add #if MBED_EXTENDED_TESTS here, to skip these tests on the CI.

fkjagodzinski · 2019-08-22T08:40:33Z

TESTS/mbed_hal/watchdog_reset/main.cpp

+
+    // Kick watchdog before timeout.
+    // Watchdog should not trigger before timeout.
+    // If device restarts while waiting for the kick, test fails.


Currently this produces an ERROR test case result instead of FAIL. Could you update that?

fkjagodzinski · 2019-08-22T09:30:44Z

Maybe we could add some frequency accuracy parameter in watchdog_features_t that test can check ?

That seems useful; would help the user to determine the watchdog timing capabilities. @donatieng, @0xc0170, what do you think?

0xc0170 · 2019-08-22T12:26:24Z

What would this number define and how it would be used? Illustration might help

Tharazi97 · 2019-09-02T07:30:26Z

So for example this is the documentation of STM32L073RZ. As we can see typical frequency is 38 kHz, but it may vary from 26 to 56 kHz. In its implementation of the watchdog LSI value is defined as 38 kHz, so ratio of defined value to max value is equal to 67% (rounding down).

I would name this parameter clock_accuracy and it would mean how accurate is not calibrated watchdog clock. The bigger value, the watchdog clock is more precise.

Practical use of this value would be to determine when to kick watchdog when it is not calibrated, so the device would not restart when not expected.

With this value, the test would look something like this:

void test_timeout()
{
    watchdog_features_t features = hal_watchdog_get_platform_features();
    if (TIMEOUT_LOWER_LIMIT_MS > features.max_timeout) {
        TEST_IGNORE_MESSAGE("Requested timeout value not supported for this target -- ignoring test case.");
        return;
    }

    // Phase 2. -- verify the test results.
    if (current_case.received_data != CASE_DATA_INVALID) {
        TEST_ASSERT_EQUAL(CASE_DATA_PHASE2_OK, current_case.received_data);
        current_case.received_data = CASE_DATA_INVALID;
        return;
    }

    // Phase 1. -- run the test code.
    watchdog_config_t config = { TIMEOUT_LOWER_LIMIT_MS };
    TEST_ASSERT_EQUAL(WATCHDOG_STATUS_OK, hal_watchdog_init(&config));

    // Kick watchdog before timeout.
    // Watchdog should not trigger before timeout * clock accuracy.
    // If device restarts while waiting for the kick, test fails.
    wait_ms(TIMEOUT_LOWER_LIMIT_MS * features.clock_accuracy / 100);
    hal_watchdog_kick();

    if (send_reset_notification(&current_case, 2 * TIMEOUT_MS) == false) {
        TEST_ASSERT_MESSAGE(0, "Dev-host communication error.");
        return;
    }
    hal_watchdog_kick();

    // Watchdog should fire before twice the timeout value.
    wait_ms(2 * TIMEOUT_LOWER_LIMIT_MS);

    // Watchdog reset should have occurred during that wait() above;

    hal_watchdog_kick();  // Just to buy some time for testsuite failure handling.
    TEST_ASSERT_MESSAGE(0, "Watchdog did not reset the device as expected.");
}

0xc0170

clock accuracy - is this in percentage (0-100) ? Not documented in hal header file

0xc0170 · 2019-09-02T10:56:39Z

targets/TARGET_STM/TARGET_STM32L0/device/stm32l0xx_hal_conf.h

@@ -136,6 +136,9 @@
 #endif /* LSI_VALUE */                      /*!< Value of the Internal Low Speed oscillator in Hz
                                             The real value may vary depending on the variations
                                             in voltage and temperature.*/
+#if !defined  (LSI_ACCURACY)


we should not be touching these device files as they get updated from upstream. This could be defined in our codebase (config for instance), and used in the watchdog HAL

@ARMmbed/team-st-mcd please review

I agree, don't patch files in device directory (except bugs)
Thx

0xc0170 · 2019-09-02T10:57:50Z

@ARMmbed/team-st-mcd Please review

fkjagodzinski · 2019-10-15T14:17:13Z

Silabs build errors are fixed now. As for the release notes -- I couldn't edit the original PR description. @adbridge, @0xc0170, could you paste this note there?:

Release Notes

watchdog_features_t extended by two uint32_t members: clock_typical_frequency and clock_max_frequency. These values can be used to determine the accuracy of an uncalibrated watchdog clock.

fkjagodzinski · 2019-10-15T14:40:54Z

Minor Watchdog docs update in ARMmbed/mbed-os-5-docs#1155.

Tharazi97 · 2019-10-15T18:57:57Z

Hi, sorry for not responding. Thank you @fkjagodzinski for taking care of this functionality.

0xc0170 · 2019-10-18T10:22:56Z

👍 for the update, will schedule CI soon

0xc0170 · 2019-10-21T07:14:00Z

CI started

mbed-ci · 2019-10-21T08:16:55Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

0xc0170 · 2019-10-21T08:21:09Z

Failures related, tests-mbed_hal-watchdog_timing failing

0xc0170 · 2019-10-28T12:32:01Z

@Tharazi97 Any update?

fkjagodzinski · 2019-10-28T12:41:24Z

@0xc0170, I'm looking into this failure right now. Will post an update shortly.

0xc0170 · 2019-10-30T15:05:45Z

CI restarted

fkjagodzinski · 2019-10-30T15:10:18Z

I updated the new test case (added in this PR) so that sleep or deepsleep mode will not be active during the wait for the watchdog reset.

Testing a watchdog performance in both sleep modes is taken care of in a different test suite. As for the K64F, I've created an issue for the deepsleep watchdog case -- #11774.

mbed-ci · 2019-10-30T16:09:55Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

0xc0170 · 2019-10-31T09:27:11Z

Travis has not reported back, I restarted it

ciarmcom requested review from 0xc0170, fkjagodzinski, jamesbeyond, maciejbocianski and a team August 12, 2019 15:00

ciarmcom added the needs: review label Aug 12, 2019

Tharazi97 force-pushed the Watchdog_lower_limit_timeout_test branch from 2611bba to bb32361 Compare August 20, 2019 11:52

0xc0170 removed request for a team August 20, 2019 11:57

fkjagodzinski suggested changes Aug 22, 2019

View reviewed changes

Tharazi97 force-pushed the Watchdog_lower_limit_timeout_test branch from bb32361 to 944e8f2 Compare September 2, 2019 10:14

0xc0170 requested a review from fkjagodzinski September 2, 2019 10:46

0xc0170 requested changes Sep 2, 2019

View reviewed changes

0xc0170 requested a review from a team September 2, 2019 10:57

0xc0170 removed the needs: review label Sep 2, 2019

Silicon Labs: Fix watchdog features build errors

fe68506

fkjagodzinski mentioned this pull request Oct 15, 2019

Update the watchdog timing requirements ARMmbed/mbed-os-5-docs#1155

Merged

0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 needs: CI and removed needs: work labels Oct 18, 2019

0xc0170 added needs: work and removed needs: CI labels Oct 21, 2019

Prevent sleep during watchdog lower limit test

5dcd0f7

0xc0170 added needs: CI ready for merge and removed needs: work needs: CI labels Oct 31, 2019

0xc0170 merged commit eea8300 into ARMmbed:master Oct 31, 2019

0xc0170 removed the ready for merge label Oct 31, 2019

fkjagodzinski deleted the Watchdog_lower_limit_timeout_test branch October 31, 2019 14:39

0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

	#define TIMEOUT_LOWER_LIMIT 1000UL
	#define TIMEOUT_LOWER_LIMIT_MS 1000UL

Add watchdog lower limit timeout test #11203

Add watchdog lower limit timeout test #11203

Uh oh!

Conversation

Tharazi97 commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Aug 12, 2019

Uh oh!

0xc0170 commented Aug 20, 2019

Uh oh!

0xc0170 commented Aug 20, 2019

Uh oh!

Tharazi97 commented Aug 20, 2019

Uh oh!

Tharazi97 commented Aug 20, 2019

Uh oh!

jeromecoutant commented Aug 20, 2019

Uh oh!

jeromecoutant commented Aug 20, 2019

Uh oh!

Tharazi97 commented Aug 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkjagodzinski commented Aug 22, 2019

Uh oh!

0xc0170 commented Aug 22, 2019

Uh oh!

Tharazi97 commented Sep 2, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 2, 2019

Uh oh!

fkjagodzinski commented Oct 15, 2019

Release Notes

Uh oh!

fkjagodzinski commented Oct 15, 2019

Uh oh!

Tharazi97 commented Oct 15, 2019

Uh oh!

0xc0170 commented Oct 18, 2019

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

mbed-ci commented Oct 21, 2019

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

0xc0170 commented Oct 28, 2019

Uh oh!

fkjagodzinski commented Oct 28, 2019

Uh oh!

0xc0170 commented Oct 30, 2019

Tharazi97 commented Aug 12, 2019 •

edited

Loading

fkjagodzinski commented Oct 30, 2019 •

edited

Loading