Add support and tests for extended RTC #5363

mprse · 2017-10-23T13:27:50Z

Description

Add support for extended RTC.
Add tests for extended RTC.

Provide support to use whole 32-bit range (unsigned int) to hold time since UNIX epoch.
The supported time range is now from the 1st of January 1970 at 00:00:00 to the 7th of February 2106 at 06:28:15.
Add support for two types of RTC devices:

RTCs which handles all leap years in the mentioned year range correctly. Leap year is determined by checking if the year counter value is divisible by 400, 100, and 4.
RTCs which handles leap years correctly up to 2100. The RTC does a simple bit comparison to see if the two lowest order bits of the year counter are zero. In this case 2100 year will be considered incorrectly as a leap year, so the last valid point in time will be 28.02.2100 23:59:59 and next day will be 29.02.2100 (invalid). So after 28.02.2100 the day counter will be off by a day.

Status

READY

Migrations

YES

Modified API of _rtc_is_leap_year(), _rtc_mktime(), _rtc_localtime() functions.
Renamed _rtc_mktime() to _rtc_maketime().

Each function has additional parameter which informs if function is used for RTC which provides full leap year detection or not.

theotherjimmy · 2017-10-23T15:00:26Z

@c1728p9 Could you review?

bulislaw · 2017-10-26T19:28:58Z

@c1728p9 ping

c1728p9 · 2017-10-26T21:12:45Z

The functions functions _rtc_mktime and _rtc_localtime are intended to convert time reported in by RTC hardware in year, month, day, hour, minute and second to and from a 32-bit second value. Because of this, time ranges across the whole 32-bit range should be supported so these devices can comply to the RTC API.

The problem that needs to be addressed is that different devices calculate leap year differently. For example after year 2100 the LPC1768's counter will be off by a day, since it's hardware has a simpler leap year calculation. The RZ-A1H does not have this problem, as it factors in 100 and 400 year leap years. To account for these two types of behavior there should be two variants of each function, or an extra parameter of each function to choose this behavior at run time. A separate enhancement to the RTC HAL tests also needs to be made (in a separate PR), to test both 4 year leap year calculations and 100 year leap year handling on hardware.

For reference here is the RTC behavior of two separate devices:

LPC1768 User Manual:

27.6.4.1 Leap year calculation

The RTC does a simple bit comparison to see if the two lowest order bits of the year counter are zero. If true, then the RTC considers that year a leap year. The RTC considers all years evenly divisible by 4 as leap years. This algorithm is accurate from the year 1901 through the year 2099, but fails for the year 2100, which is not a leap year. The only effect of leap year on the RTC is to alter the length of the month of February for the month, day of month, and year counters.

RZ_A1H User Manual:

13.3.6 Day Counter (RDAYCNT)

A leap year is determined by checking if the year counter (RYRCNT) value is divisible by 400, 100, and 4.

adbridge · 2017-10-31T15:58:58Z

@mprse could you please address @c1728p9 comments ?

mprse · 2017-11-01T20:47:13Z

@c1728p9 Thank you for detailed objectives. I will come back to this PR next week.

Because of this, time ranges across the whole 32-bit range should be supported so these devices can comply to the RTC API.

We can use entire 32-bit range, but then we will not have value which indicate error. Can we modify the _rtc_mktime() function to return Boolean value (true on successful conversion) and calculated value of seconds via additional parameter? It would be consistent with _rtc_localtime() function. e.g. bool _rtc_mktime(const struct tm* time, time_t * timestamp);

mprse · 2017-11-02T14:07:04Z

@c1728p9 Could you please confirm if I correctly understand the problem:

For all RTC devices _rtc_mktime and _rtc_localtime shell support whole 32-bit range for count of elapsed seconds since the 1 of January 1970.
We have two types of RTC devices:
a) RTC devices which handle leap years properly for entire possible date range: from the 1st of January 1970 at 00:00:00 to the 7th of February 2106 at 06:28:15 (0 - 0xffff_ffff).
b) RTC devices which incorrectly recognise 2100 year as a leap year. So after 28.02.2100 23:59:59 RTC time will be invalid. On these devices next point of time will be 29.02.2100 0:0:0 which is invalid date.

I'm not sure how these invalid dates should be handled. Lets take an example:
Last valid point of time for both RTC types:
28.02.2100 23:59:59 <----> 4107542399
After a second we will have the following case:
a) 01.03.2100 0:0:0 <----> 4107542400
b) 29.02.2100 0:0:0 <----> 4107542400

Expected functions behaviour:
a) _rtc_mktime (01.03.2100 0:0:0) ----> 4107542400
_rtc_localtime (4107542400) ----> 01.03.2100 0:0:0
_rtc_is_leap_year(2100) ----> false

b) _rtc_mktime (29.02.2100 0:0:0) ----> 4107542400
_rtc_localtime (4107542400) ----> 29.02.2100 0:0:0
_rtc_is_leap_year(2100) ----> true

Questions:

Is that correct behaviour? Or maybe invalid dates should be fixed by the software?
It looks like to achieve such results modification of _rtc_is_leap_year function is sufficient. You wrote that "there should be two variants of each function, or an extra parameter of each function". Why we can not use special symbol for this purpose and preprocesor directives? e.g:

bool _rtc_is_leap_year(int year) {
#ifdef DEVICE_RTC_100
    if(year == 200) {
        return false; // 2100 is not a leap year
    }
#endif
    return (year) % 4 ? false : true;
}

c1728p9 · 2017-11-02T14:44:28Z

Could you please confirm if I correctly understand the problem

@mprse your understanding of this is correct based on the points you made.

bool _rtc_mktime(const struct tm* time, time_t * timestamp);

Adding a bool to this works for me. @pan- are you ok with this?

Why we can not use special symbol for this purpose and preprocesor directives?

A preprocessor directive should only be used as a last resort as it complicates the code (in this case the same function behaves differently) and prevents testing all configurations in the same build.

mprse · 2017-11-06T14:20:03Z

@c1728p9 This PR has been updated to meet the requirements.

c1728p9 · 2017-11-06T15:34:11Z

platform/mbed_mktime.h

+ */
+typedef enum {
+    RTC_FULL_LEAP_YEAR_SUPPORT,
+    RTC_PARTIAL_LEAP_YEAR_SUPPORT


To remove some ambiguity it may be good to rename this to something like RTC_4_YEAR_LEAP_YEAR_SUPPORT

Fixed as suggested.

c1728p9 · 2017-11-06T15:46:43Z

platform/mbed_mktime.c

+
+/* Macros which will be used to determine if we are within valid range. */
+#define EDGE_TIMESTAMP_FULL_LEAP_YEAR_SUPPORT 3220095     // 7th of February 1970 at 06:28:15
+#define EDGE_TIMESTAMP_PARTIAL_LEAP_YEAR_SUPPORT 3133695  // 6th of February 1970 at 06:28:15


Why was this date chosen? Also, why is it different for partial vs full?

This is a shortcut to check if calendar date provided to _rtc_mktime() function is within valid range. This is used to check upper limit. Function first only checks if the year range is valid. Then it calculates elapsed time since the beginning of the specified year. At this point we can check if provided date is within the range using these macros. If specified year is 2106 (last valid) and seconds since the beginning of the year are greater than edge timestamp, then we are out of range.
Different values are used for these two types of RTC devices. On devices which support full leap year detection last valid point of time is the 7th of February 2106 at 06:28:15 and on devices which have only 4 year leap year detection last valid date is 6th of February 2106 at 06:28:15 (at this point we are one day off). Both dates correspond to 0xffff_ffff number of seconds since the start of unix epoch.

bulislaw · 2017-11-08T15:15:03Z

@mprse ping

pan- · 2017-11-08T16:10:13Z

Adding a bool to this works for me. @pan- are you ok with this?

I would also rename the function to discriminate it from mktime which has a well defined semantic.

mprse · 2017-11-09T08:43:00Z

I would also rename the function to discriminate it from mktime which has a well defined semantic.

Changed mktime to maketime.

@mprse ping

All change requests are processed now.

bulislaw · 2017-11-10T13:26:01Z

@mprse can you fix the conflict please.

mprse · 2017-11-13T07:48:13Z

@mprse can you fix the conflict please.

Conflict has been resolved.

0xc0170 · 2017-11-13T09:46:59Z

@mprse Can you please check travis , the failures are related to this patch

mprse · 2017-11-13T10:32:25Z

@mprse Can you please check travis , the failures are related to this patch.

This PR modifies API of _rtc_is_leap_year(), _rtc_mktime(), _rtc_localtime() functions and renames _rtc_mktime() to _rtc_maketime(). Travis reports errors since Vendor's RTC drivers are not updated.

@c1728p9 @0xc0170 @bulislaw
I'm not sure how we handle such cases. We expect that Vendors will update their RTC drivers? So maybe in such case we need a feature branch for this?
Or maybe I should adjust also all RTC drivers in additional commit?

c1728p9 · 2017-11-13T17:55:23Z

@mprse I would recommend updating the targets RTC drivers. There should be < 10 files that need updating. This global update of targets was done when mktime was initially added here:
f880e44

mprse · 2017-11-14T12:43:22Z

Changed base of this PR from ARMmbed:master to ARMmbed:feature-hal-spec-rtc.

bulislaw · 2017-11-15T10:35:44Z

@mprse @0xc0170 What's the next step here? We need to merge it ASAP

mprse · 2017-12-04T16:32:59Z

Test : FAILURE
Build number : 469
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5363/469

Increased timeout for rtc_time_conv test.

0xc0170 · 2017-12-04T16:37:52Z

/morph build

mbed-ci · 2017-12-04T17:15:17Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2017-12-04T18:36:11Z

Exporter Build : ABORTED

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

mbed-ci · 2017-12-04T18:46:29Z

Test : FAILURE

Build number : 472
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5363/472

mprse · 2017-12-05T06:54:45Z

Increased timeout for rtc_time_conv test again.

0xc0170 · 2017-12-05T06:59:47Z

/morph build

0xc0170 · 2017-12-05T07:00:03Z

@mprse Please test locally if possible

mbed-ci · 2017-12-05T07:40:11Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2017-12-05T08:50:48Z

Exporter Build : SUCCESS

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

mbed-ci · 2017-12-05T09:12:51Z

Test : SUCCESS

Build number : 476
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5363/476

Although the extened RTC is supported by ARMmbed#5363 commit ("Add support and tests for extended RTC"), it seems that the changes was overlooked in RZ_A1H_api.c. So I added the changes with reference to other rtc_api.c.

Although the extened RTC is supported by #5363 commit ("Add support and tests for extended RTC"), it seems that the changes was overlooked in RZ_A1H_api.c. So I added the changes with reference to other rtc_api.c.

0xc0170 · 2018-01-02T19:04:58Z

@mprse We should review this As a result, even though it was tagged for 5.8, this is causing some new target dependencies that we should avoid.

I am thinking if this can be made backward compatible (it is internal API but affects targets) - having the functionality as it was, and the new one with suffix _extended in this case, . This would allow us to use old API for current patch releases, and would port it over to the new one for 5.8. Or just revert this changeset (simpler approach?), have feature branch and get this in right for 5.8 (5.7 would be as it was, and 5.8 would get the extended support in).

theotherjimmy added the needs: review label Oct 23, 2017

0xc0170 requested a review from c1728p9 October 26, 2017 19:41

adbridge added needs: work and removed needs: review labels Oct 31, 2017

mprse mentioned this pull request Nov 2, 2017

Update of RTC HAL API specification and tests. #5306

Merged

mprse force-pushed the extended_rtc branch from 8d0e121 to 062c2c9 Compare November 6, 2017 14:08

c1728p9 reviewed Nov 6, 2017

View reviewed changes

mprse force-pushed the extended_rtc branch from 062c2c9 to 0a31e3d Compare November 9, 2017 08:39

mprse force-pushed the extended_rtc branch from 0a31e3d to 1c5c203 Compare November 13, 2017 07:44

mprse force-pushed the extended_rtc branch from 1c5c203 to b0354bc Compare November 14, 2017 12:41

mprse changed the base branch from master to feature-hal-spec-rtc November 14, 2017 12:42

0xc0170 added the needs: review label Nov 15, 2017

mprse added 2 commits December 5, 2017 07:53

Add tests for extended RTC.

fcdaecc

Update RTC drivers for extended RTC.

1065616

mprse force-pushed the extended_rtc branch from 6137f63 to 1065616 Compare December 5, 2017 06:54

0xc0170 added ready for merge and removed needs: CI labels Dec 5, 2017

0xc0170 added the release-version: 5.8.0-rc1 label Dec 12, 2017

0xc0170 merged commit be52ba2 into ARMmbed:master Dec 12, 2017

0xc0170 removed the ready for merge label Dec 12, 2017

adbridge mentioned this pull request Jan 2, 2018

Release candidate for mbed-os-5.7.2 #5767

Merged

adbridge mentioned this pull request Jan 12, 2018

RTC time conversion test - reduce test execution time. #5802

Merged

adbridge mentioned this pull request Feb 23, 2018

Nuvoton: Rework RTC #6049

Merged

Add support and tests for extended RTC #5363

Add support and tests for extended RTC #5363

Uh oh!

Conversation

mprse commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Uh oh!

theotherjimmy commented Oct 23, 2017

Uh oh!

bulislaw commented Oct 26, 2017

Uh oh!

c1728p9 commented Oct 26, 2017

Uh oh!

adbridge commented Oct 31, 2017

Uh oh!

mprse commented Nov 1, 2017

Uh oh!

mprse commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 commented Nov 2, 2017

Uh oh!

mprse commented Nov 6, 2017

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!

bulislaw commented Nov 8, 2017

Uh oh!

pan- commented Nov 8, 2017

Uh oh!

mprse commented Nov 9, 2017

Uh oh!

bulislaw commented Nov 10, 2017

Uh oh!

mprse commented Nov 13, 2017

Uh oh!

0xc0170 commented Nov 13, 2017

Uh oh!

mprse commented Nov 13, 2017

Uh oh!

c1728p9 commented Nov 13, 2017

Uh oh!

mprse commented Nov 14, 2017

Uh oh!

bulislaw commented Nov 15, 2017

Uh oh!

mprse commented Dec 4, 2017

Uh oh!

0xc0170 commented Dec 4, 2017

Uh oh!

mbed-ci commented Dec 4, 2017

Triggering tests

Uh oh!

mbed-ci commented Dec 4, 2017

Uh oh!

mbed-ci commented Dec 4, 2017

Uh oh!

mprse commented Dec 5, 2017

Uh oh!

0xc0170 commented Dec 5, 2017

Uh oh!

0xc0170 commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Dec 5, 2017

Triggering tests

Uh oh!

mbed-ci commented Dec 5, 2017

Uh oh!

mbed-ci commented Dec 5, 2017

Uh oh!

mprse commented Oct 23, 2017 •

edited

Loading

mprse commented Nov 2, 2017 •

edited

Loading

0xc0170 commented Dec 5, 2017 •

edited

Loading