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

STM32: Fix RTC test issue on targets using a 16-bit timer for us_ticker #7352

Merged
merged 5 commits into from Jul 9, 2018

Conversation

Projects
None yet
6 participants
@bcostm
Contributor

bcostm commented Jun 27, 2018

Description

This is a fix for Issue #7316

As proposed by @c1728p9 the us_ticker_read() must be used while the SDK is not ready.

Tests:

  • rtc, time, sleep and tick tests OK on NUCLEO_L073RZ, NUCLEO_F070RB + ARM
  • HAL_Delay function tested OK inside a basic application with NUCLEO_F103RB
  • Run rtc, time, sleep and tick tests on all boards

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/team-st-mcd Jun 27, 2018

return ticker_read_us(get_us_ticker_data()) / 1000; // 1 ms tick is required for ST HAL
// 1 ms tick is required for ST HAL driver
if (mbed_sdk_inited) {
return (ticker_read_us(get_us_ticker_data()) / 1000);

This comment has been minimized.

@c1728p9

c1728p9 Jun 27, 2018

Contributor

This is going to start counting from 0 so there will likely be a time jump after the SDK is initialized. As a workaround you may want to record the time returned by us_ticker_read just before mbed_sdk_inited is set and use it as an offset.

This comment has been minimized.

@bcostm

bcostm Jul 6, 2018

Contributor

Done in commit f785c23

return (ticker_read_us(get_us_ticker_data()) / 1000);
}
else {
return (us_ticker_read() / 1000);

This comment has been minimized.

@c1728p9

c1728p9 Jun 27, 2018

Contributor

With the 16 bit timer this will overflow in ~66ms. If anything in the SDK init takes longer than this it may get stuck as HAL_GetTick will never count higher than this during init. As a potential solution you could accumulate elapsed time rather than reading it directly. Something like:

uint32_t new_time = us_ticker_read();
elapsed_time += (new_time - prev_time) & 0xFFFF; // Only use the lower 16 bits
prev_time = new_time;
return elapsed_time / 1000;

Note - You'll need to make sure prev_time is reset in HAL_InitTick since this resets the timer count.

This comment has been minimized.

@bcostm

bcostm Jul 6, 2018

Contributor

Done in commit f785c23

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 28, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 2, 2018

Hi
Note that this PR should also solve all PWM failed tests with CI shield.

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jul 2, 2018

Today's status:

  • NUCLEO_F103RB: all RTC tests are OK with or without additional patch requested by Russ
  • NUCLEO_L073RZ, NUCLEO_F070RB, DISCO_L072CZ: tests-mbed_hal-rtc test is FAIL (RTC-Persist test case) with or without additional patch requested by Russ. On these 3 boards, the LPTICKER+LPTIM is used while no LPTICKER on F103RB. The fail appears after the device enters in deepsleep in RTC-sleep test case.

@bcostm bcostm force-pushed the bcostm:fix_rtc_ticker branch from 9c406ef to fcdd529 Jul 4, 2018

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jul 4, 2018

With commit fcdd529 (thanks @jeromecoutant ), the time, rtc, sleep, tick tests are OK on NUCLEO_L073RZ, NUCLEO_F070RB boards.

We are going to perform more tests on more boards but it looks good now.

@@ -161,6 +161,7 @@ void hal_deepsleep(void)
// Disable IRQs
core_util_critical_section_enter();
// Save the timer counter value in order to reprogram it after deepsleep

This comment has been minimized.

@jeromecoutant

jeromecoutant Jul 4, 2018

Contributor

I would prefer a new function in hal_tick_common.c which saves TIM_MST needed registers before going into deepsleep:

  • CNT
  • CCR1
  • DIER

This comment has been minimized.

@bcostm

bcostm Jul 4, 2018

Contributor

done in commit a838cb2

TIM_HandleTypeDef TimMasterHandle;
TimMasterHandle.Instance = TIM_MST;
__HAL_TIM_SET_COUNTER(&TimMasterHandle, EnterTimeUS);
__HAL_TIM_CLEAR_FLAG(&TimMasterHandle, TIM_FLAG_CC1);

This comment has been minimized.

@jeromecoutant

jeromecoutant Jul 4, 2018

Contributor

Call here a function to restore TIM_MST registers

This comment has been minimized.

@bcostm

bcostm Jul 4, 2018

Contributor

done in commit a838cb2

else {
new_time = us_ticker_read();
elapsed_time += (new_time - prev_time) & 0xFFFF; // Only use the lower 16 bits
prev_time = new_time;

This comment has been minimized.

@jeromecoutant

jeromecoutant Jul 4, 2018

Contributor

Is this really needed ?
What about 32b targets ?

This comment has been minimized.

@bcostm

bcostm Jul 4, 2018

Contributor

commit 9523bcf do this only for 16bits timer

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jul 5, 2018

Tests are OK now.

@cmonr cmonr added needs: CI and removed needs: work labels Jul 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

Weird. Trying again.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 6, 2018

Build : SUCCESS

Build number : 2547
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7352/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Jul 9, 2018

@cmonr

cmonr approved these changes Jul 9, 2018

@cmonr cmonr merged commit bcec185 into ARMmbed:master Jul 9, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10195 cycles (-20 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Jul 9, 2018

@bcostm bcostm deleted the bcostm:fix_rtc_ticker branch Jul 16, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7352 from bcostm/fix_rtc_ticker
STM32: Fix RTC test issue on targets using a 16-bit timer for us_ticker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment