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

ST: Watchdog: Fix timeout registers value calculation #11178

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@Tharazi97
Copy link
Contributor

commented Aug 7, 2019

Description

Proposition of fixing: #11175
Changes only implementation of watchdog on ST devices, but problem applies mainly to the meaning of this test. I don't know if this is a right way of fixing that. @fkjagodzinski might help.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] 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 7, 2019

@ciarmcom

This comment has been minimized.

@@ -18,6 +18,10 @@
#error [NOT_SUPPORTED] Watchdog not supported for this target
#else

#ifndef MAX_IWDG_PR
#define MAX_IWDG_PR 0x6

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond Aug 7, 2019

Contributor

Why is the max pre-scaler defined as 6 ?

This comment has been minimized.

Copy link
@Tharazi97

Tharazi97 Aug 7, 2019

Author Contributor

Right, I didn't look to other targets than STM and assumed that it is used like this in all boards. I'll take a peek to other drivers and make it work on all devices.

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 7, 2019

Test run: FAILED

Summary: 3 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 8, 2019

// The watchdog should trigger at, or after the timeout value.
TEST_ASSERT(watchdog.get_timeout() >= max_timeout);
// The watchdog should trigger at, or after the timeout value - maximum time spend on prescaller.
uint32_t max_window = ceil((float)(4 << MAX_IWDG_PR) * 1000 / LSI_VALUE);

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 8, 2019

Member

this is also one MCU family specific value. See LSI_VALUE error in the build logs.

The test need to be generic and using driver functionality (=test).

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

I've changed the function in stm watchdog_api.c so it rounds the value of timeout up. This makes the test pass, but it still needs to be discussed if it's the right way of doing that. @fkjagodzinski might help in clarifying this problem.

@@ -225,7 +225,7 @@ void test_start_max_timeout()
Watchdog &watchdog = Watchdog::get_instance();
uint32_t max_timeout = watchdog.get_max_timeout();
TEST_ASSERT_TRUE(watchdog.start(max_timeout));
// The watchdog should trigger at, or after the timeout value.
// The watchdog should trigger at, or after the timeout value;

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 9, 2019

Member

this change is not related. the rest of the comments in this file has it as it was. please revert

@0xc0170 0xc0170 requested a review from ARMmbed/team-st-mcd Aug 9, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

This currently is touching only ST targets, this should be stated (title and updated comment about the scope of this fix)

@Tharazi97 Tharazi97 force-pushed the Tharazi97:watchdog_max_timeout branch from 6e9eafd to eb558de Aug 9, 2019

@Tharazi97 Tharazi97 changed the title proposition of fix watchdog start max timeout proposition of fix watchdog start max timeout (tweak ST implementation) Aug 9, 2019

@Tharazi97 Tharazi97 changed the title proposition of fix watchdog start max timeout (tweak ST implementation) proposition of fix watchdog start max timeout (tweak STM implementation) Aug 9, 2019

@Tharazi97 Tharazi97 force-pushed the Tharazi97:watchdog_max_timeout branch from eb558de to 302be42 Aug 12, 2019

@Tharazi97 Tharazi97 changed the title proposition of fix watchdog start max timeout (tweak STM implementation) fix watchdog start max timeout and test update config (tweak STM implementation) Aug 12, 2019

@fkjagodzinski
Copy link
Member

left a comment

  • Please clean up the commit history -- if you make an update and then revert it, this shouldn't be visible here.
  • Please update the commit messages -- e.g. change in stm watchdog doesn't say much. The commit message may be as long as you need it to be to explain why and what was updated. Take a look at our commit history for reference.
  • If the code needs an astyle update to pass the CI, please keep that in a separate commit.

// Convert Prescaler_divider bits (PR) of Prescaler_register (IWDG_PR) and a timeout value [ms]
// to Watchdog_counter_reload_value bits (RL) of Reload_register (IWDG_RLR)
#define PR_TIMEOUT_MS2RL(PR_BITS, TIMEOUT_MS) \
((TIMEOUT_MS) * (LSI_VALUE) / (PR2PRESCALER_DIV(PR_BITS)) / 1000UL)
((uint32_t)ceil((float)(TIMEOUT_MS) * (LSI_VALUE) / (PR2PRESCALER_DIV(PR_BITS)) / 1000UL))

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 14, 2019

Member

Avoid using floating point if possible. The code below will give same values as ceil().

Suggested change
((uint32_t)ceil((float)(TIMEOUT_MS) * (LSI_VALUE) / (PR2PRESCALER_DIV(PR_BITS)) / 1000UL))
(((TIMEOUT_MS) * (LSI_VALUE) / (PR2PRESCALER_DIV(PR_BITS)) + 999UL) / 1000UL)
const uint32_t rl = PR_TIMEOUT_MS2RL(pr, config->timeout_ms);
uint32_t rl = PR_TIMEOUT_MS2RL(pr, config->timeout_ms);
if (rl > MAX_IWDG_RL) {
rl = MAX_IWDG_RL;

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 14, 2019

Member

Isn't that a dead code? pr != INVALID_IWDG_PR should guarantee that rl is also valid.

@fkjagodzinski

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@Tharazi97 could you also update the name and the description of this PR? From what I see now, this could be as simple as ST: Watchdog: Fix timeout registers value calculation. ;)

@Tharazi97 Tharazi97 changed the title fix watchdog start max timeout and test update config (tweak STM implementation) ST: Watchdog: Fix timeout registers value calculation Aug 14, 2019

@Tharazi97 Tharazi97 force-pushed the Tharazi97:watchdog_max_timeout branch from 302be42 to a75d43b Aug 14, 2019

Tweak STM watchdog implementation
Change the calculation method of rl so it is rounded up.

@Tharazi97 Tharazi97 force-pushed the Tharazi97:watchdog_max_timeout branch from a75d43b to df43350 Aug 14, 2019

@jeromecoutant
Copy link
Contributor

left a comment

NUCLEO_L073RZ watchdog tests are now OK!
DISCO_L475VG_IOT01A watchdog tests are still OK.

Thx for the fix

@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 20, 2019

Test run: SUCCESS

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

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

25 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/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8651 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
@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@donatieng we fixed a watchdog issue related to NUCLEO_L073RZ and someother STM target, you might need to aware of this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.