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.

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

make watchdog kick reset test pass CI (LSI problem) #11172

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@Tharazi97
Copy link
Contributor

commented Aug 6, 2019

Description

This change makes the watchdog kick reset pass on device NUCLEO-L073RZ.

void test_kick_reset()
{
// 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_MS };
TEST_ASSERT_EQUAL(WATCHDOG_STATUS_OK, hal_watchdog_init(&config));
for (int i = 3; i; i--) {
// The reset is prevented as long as the watchdog is kicked
// anytime before the timeout.
wait_ms(TIMEOUT_MS - KICK_ADVANCE_MS);
hal_watchdog_kick();
}
if (send_reset_notification(&current_case, 2 * TIMEOUT_MS) == false) {
TEST_ASSERT_MESSAGE(0, "Dev-host communication error.");
return;
}
// Watchdog should fire before twice the timeout value.
wait_ms(2 * TIMEOUT_MS); // Device reset expected.
// 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.");
}

According to documentation the LSI oscillator frequency might vary from 26 kHz to 56 kHz. So the test might still don't pass on some devices, but in worst case watchdog might fire half time sooner than specified. We have relaxed this test earlier:

template<uint32_t timeout_ms>
void test_timing()
{
watchdog_features_t features = hal_watchdog_get_platform_features();
if (timeout_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.
// Verify the heartbeat time span sent by host is within given range:
// 1. The watchdog should trigger at, or after the timeout value.
// 2. The watchdog should trigger before twice the timeout value.
if (current_case.received_data != CASE_DATA_INVALID) {
// Provided the watchdog works as expected, the last timestamp received
// by the host will always be before the expected reset time. Because
// of that, the constraint no 1. is not verified.
TEST_ASSERT(current_case.received_data > 0);
TEST_ASSERT(current_case.received_data < 2 * timeout_ms);
current_case.received_data = CASE_DATA_INVALID;
return;
}

so it checks whether the watchdog restarts the device before 2*timeout. But we don't specify the lower limit. I think the lower limit should be checked too, but regarding to datasheets many devices have their LSI oscillators frequency differ very much. So it would be hard to choose good range of acceptable difference of timeouts.

I think it would be appropriate to write that test (watchdog kick reset) so it passes on all devices and write another test that checks the low limit of specific device.

Please review

Pull request type

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

Reviewers

@maciejbocianski @mprse @jamesbeyond @fkjagodzinski

Release Notes

@ciarmcom ciarmcom requested review from fkjagodzinski, jamesbeyond, maciejbocianski, mprse and ARMmbed/mbed-os-maintainers Aug 6, 2019

@ciarmcom

This comment has been minimized.

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

On our device watchdog kick reset test passes with 8ms window to timeout (KICK_ADVANCE_MS), but on CI it didn't pass with 10ms window. According to documentation it may be caused by the problem I mentioned above. The LSI value is defined in drivers here:

#if !defined (LSI_VALUE)
#define LSI_VALUE (37000U) /*!< LSI Typical Value in Hz*/
#endif /* LSI_VALUE */ /*!< Value of the Internal Low Speed oscillator in Hz

and here:
#if !defined (LSI_VALUE)
#define LSI_VALUE ((uint32_t)37000U) /*!< Value of the LSI oscillator in Hz */
#endif /* LSI_VALUE */

It's already strange that it is defined as 37 kHz, not 38 kHz as in documentation. On our board defining it as 40 kHz works very good what shows that the problem probably lays there.

@Tharazi97 Tharazi97 changed the title make test pass CI make test pass CI watchdog (LSI problem) Aug 7, 2019

@Tharazi97 Tharazi97 changed the title make test pass CI watchdog (LSI problem) make watchdog kick reset test pass CI (LSI problem) Aug 7, 2019

@jamesbeyond
Copy link
Contributor

left a comment

I am OK with the changes. I'd like to hear your opinion @fkjagodzinski since you started those watchdog tests

@0xc0170
0xc0170 approved these changes Aug 9, 2019

@Tharazi97 Tharazi97 force-pushed the Tharazi97:Watchdog_fail branch from 7afecfb to c4912c0 Aug 12, 2019

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I have added lower limit test of watchdog timeout. I think STM drivers have to be changed in that way so they pass the test and meet mbed os watchdog assumptions.
"The watchdog should trigger at or after the timeout value."

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I have added lower limit test of watchdog timeout. I think STM drivers have to be changed in that way so they pass the test and meet mbed os watchdog assumptions.
"The watchdog should trigger at or after the timeout value."

I don't follow. What is the next step? Is this PR ready?

cc @ARMmbed/team-st-mcd

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Last changes made NUCLEO-L073RZ pass the test. But ST devices still didn't keep the assumption specified in docs: "The watchdog should trigger at or after the timeout value."
So I've also added the test that checks this assumption. STM targets won't pass this test so I think there are 3 possible solutions now:

  1. Change the STM watchdog API so it meets assumptions specified in docs.
  2. 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).
  3. Relax assumptions.

@Tharazi97 Tharazi97 force-pushed the Tharazi97:Watchdog_fail branch from e913d2f to cf79e4c Aug 12, 2019

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

To clarify I splitted this PR into separate two PRs as it applies to two different problems. This PR makes the NUCLEO-L073RZ pass the test kick reset watchdog. KICK_ADVANCE_MS constant had to be changed to bigger value, because of this target LSI frequency variation. This change makes the test pass in the worst possible case.

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

I think this PR is ready for CI

@fkjagodzinski
Copy link
Member

left a comment

@Tharazi97 good job on the investigation! 👍

I'm fine with the patch, but please add comments to the code. Also, please update the commit message to be more informative.

@@ -26,7 +26,7 @@
#include "mbed.h"

#define TIMEOUT_MS 100UL
#define KICK_ADVANCE_MS 10UL
#define KICK_ADVANCE_MS 35UL

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 12, 2019

Member

Since this value is no longer arbitrary, and is now based on real data, we should explain how it was determined.

Suggested change
#define KICK_ADVANCE_MS 35UL
/* This value is used to calculate the time to kick the watchdog.
* Given the watchdog timeout is set to TIMEOUT_MS, the kick will be performed
* with a delay of (TIMEOUT_MS - KICK_ADVANCE_MS), after the init.
*
* It is common for the watchdog peripheral to use a low precision clock source,
* e.g. the LSI RC acts as a clock source for the IWDG on ST targets.
* According to the ST spec, the 37 kHz LSI is guaranteed to have a frequency
* around 37-38 kHz, but the actual frequency range guaranteed by the production
* tests is 26 kHz up to 56 kHz.
* Bearing that in mind, a 100 ms timeout value may actually last as long as 142 ms
* and as short as 66 ms.
* The value of 35 ms is used to cover the worst case scenario (66 ms).
*/
#define KICK_ADVANCE_MS 35UL
@@ -26,7 +26,7 @@
#include "mbed.h"

#define TIMEOUT_MS 100UL
#define KICK_ADVANCE_MS 10UL
#define KICK_ADVANCE_MS 35UL

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 12, 2019

Member

Same as above -- add a comment explaining why 35 ms is used.

tweak watchdog kick reset test
Change the value of KICK_ADVANCE_MS so targets pass the test in worst cases

@Tharazi97 Tharazi97 force-pushed the Tharazi97:Watchdog_fail branch from cf79e4c to da70c88 Aug 13, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 19, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 19, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 3b631fd into ARMmbed:master Aug 20, 2019

18 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8713 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.