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 RTC : write RTC time while LPTICKER is enabled #8286

Merged
merged 1 commit into from Oct 9, 2018

Conversation

Projects
None yet
7 participants
@jeromecoutant
Contributor

jeromecoutant commented Oct 1, 2018

Description

During RTC register write, only the date and time counters are set,
the SubSecond part is reset.
In the previous implementation, there was a quite long loop (up to 1 second).

@LMESTM @c1728p9 @mattbrown015

Pull request type

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

@0xc0170 0xc0170 requested review from c1728p9 and ARMmbed/team-st-mcd Oct 1, 2018

struct tm timeinfo;
core_util_critical_section_enter();

This comment has been minimized.

@c1728p9

c1728p9 Oct 2, 2018

Contributor

Was something calling this outside a critical section?

This comment has been minimized.

@jeromecoutant

jeromecoutant Oct 4, 2018

Contributor

I added protection, as rtc_write and rtc_read_lp functions can update the same variables

@c1728p9

c1728p9 approved these changes Oct 2, 2018

@LMESTM

Request for few comments to be added

/* Save current SSR */
uint32_t Read_SubSeconds = (uint32_t)(RTC->SSR);
/* RTC SubSeconds counter is reset to PREDIV_S_VALUE */
LP_last_RTC_time = (timeStruct.Seconds + timeStruct.Minutes * 60 + timeStruct.Hours * 60 * 60) * PREDIV_S_VALUE;

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

Does this always fit into a uint32_t ? or would a uint64_t be required ?
Also is LP_last_RTC_time in real seconds ? or µs ?

/* Save current SSR */
uint32_t Read_SubSeconds = (uint32_t)(RTC->SSR);
/* RTC SubSeconds counter is reset to PREDIV_S_VALUE */

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

Not clear to me what the comment says and how it relates to the code below: where is Subsecond counter reset to PREDIV_S_VALUE ?

@@ -341,11 +334,16 @@ static void RTC_IRQHandler(void)
uint32_t rtc_read_lp(void)
{
/* LPTICKER time is reading only time RTC register (in second) and add the sub-second part

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

Where is the sub-second part retrieved from and how is it computed ?

/* LPTICKER time is reading only time RTC register (in second) and add the sub-second part
* In order to get a continuous time information (each 24h = 86400s = 0x15180),
* current time is added into an internal counter : LP_continuous_time
* In order to get spent time since last counter update, current RTC time is saved : LP_last_RTC_time

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

what is "last counter update" ? do you mean RTC_write (like application set_time()) ?
Does this comment apply here or in RTC_write function ?

@@ -358,17 +356,18 @@ uint32_t rtc_read_lp(void)
timeinfo.tm_min = RTC_Bcd2ToByte((uint8_t)((Read_time & (RTC_TR_MNT | RTC_TR_MNU)) >> 8));
timeinfo.tm_sec = RTC_Bcd2ToByte((uint8_t)((Read_time & (RTC_TR_ST | RTC_TR_SU)) >> 0));
uint32_t RTC_time_s = timeinfo.tm_sec + timeinfo.tm_min * 60 + timeinfo.tm_hour * 60 * 60; // Max 0x0001-517F => * 8191 + 8191 = 0x2A2E-AE80
uint32_t RTC_time_s = (timeinfo.tm_sec + timeinfo.tm_min * 60 + timeinfo.tm_hour * 60 * 60) * PREDIV_S_VALUE + PREDIV_S_VALUE - Read_SubSeconds; // Max 0x0001-517F * 8191 + 8191 = 0x2A2E-AE80

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

is RTC_time_s still in seconds ? the operation seems to mix it with sub_seconds. If so, need to rename the variable.

@@ -358,17 +356,18 @@ uint32_t rtc_read_lp(void)
timeinfo.tm_min = RTC_Bcd2ToByte((uint8_t)((Read_time & (RTC_TR_MNT | RTC_TR_MNU)) >> 8));
timeinfo.tm_sec = RTC_Bcd2ToByte((uint8_t)((Read_time & (RTC_TR_ST | RTC_TR_SU)) >> 0));
uint32_t RTC_time_s = timeinfo.tm_sec + timeinfo.tm_min * 60 + timeinfo.tm_hour * 60 * 60; // Max 0x0001-517F => * 8191 + 8191 = 0x2A2E-AE80
uint32_t RTC_time_s = (timeinfo.tm_sec + timeinfo.tm_min * 60 + timeinfo.tm_hour * 60 * 60) * PREDIV_S_VALUE + PREDIV_S_VALUE - Read_SubSeconds; // Max 0x0001-517F * 8191 + 8191 = 0x2A2E-AE80
if (LP_last_RTC_time <= RTC_time_s) {
LP_continuous_time += (RTC_time_s - LP_last_RTC_time);

This comment has been minimized.

@LMESTM

LMESTM Oct 2, 2018

Contributor

Would be great to explain in commit message how those 3 variables are used:
LP_continuous_time : is it the time in µsec since start-up ?
RTC_time_s: the current time that was just read from RTC register ?
LP_last_RTC_time: the last time read from RTC register, either in this function, or in RTC_write() ?)
I don't undertstand the "add 24h" case ...

STM32 RTC : write RTC time while LPTICKER is enabled
This fix avoid a long waiting loop in rtc_write function,
  which was not acceptable in TICKLESS context.

Implementation comments added.

Global variable name has been updated for easier maintenance:
- LPTICKER_counter is the U32 continuous tick counter
- LPTICKER_RTC_time is the RTC time used to get the time difference
   between rtc_read_lp() calls

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_RTCWRITE branch from 0362e53 to 83caa04 Oct 4, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 4, 2018

@LMESTM please check updates

@0xc0170

0xc0170 approved these changes Oct 4, 2018

@LMESTM

LMESTM approved these changes Oct 5, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 5, 2018

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Oct 7, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 9, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 18d6131 into ARMmbed:master Oct 9, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 626 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10201 cycles (+270 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 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Oct 9, 2018

@jeromecoutant jeromecoutant deleted the jeromecoutant:PR_RTCWRITE branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment