Low power timer needs to be reset when setting time #7849

TacoGrandeTX · 2018-08-21T17:46:38Z

Description

This PR addresses #7804.

When set_time() is called with DEVICE_LPTICKER the timer needs to be reset also, otherwise
the old _start value is incorrect and this taints the value returned by Timer::read().

Using the issue's test program, the minutes and seconds are now properly tracked after calling set_time():

resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018
Thu Aug 16 23:01:04 2018
Thu Aug 16 23:01:05 2018
Thu Aug 16 23:01:06 2018
Thu Aug 16 23:01:07 2018
Thu Aug 16 23:01:08 2018
Thu Aug 16 23:01:09 2018
Thu Aug 16 23:01:10 2018
resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018
Thu Aug 16 23:01:04 2018
Thu Aug 16 23:01:05 2018
Thu Aug 16 23:01:06 2018
Thu Aug 16 23:01:07 2018
Thu Aug 16 23:01:08 2018
Thu Aug 16 23:01:09 2018
Thu Aug 16 23:01:10 2018
resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018

Pull request type

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

TacoGrandeTX · 2018-08-21T17:50:44Z

Adding @kegilbert for comment/review

kegilbert

LGTM, needs someone closer to the lpticker source to sign off though. Added @c1728p9 . Feel free to add anyone else.

cmonr · 2018-08-22T01:55:20Z

@NeilMacMullen Would you mind confirming that this fixes the issue?

0xc0170 · 2018-08-22T12:39:22Z

@TacoGrandeTX Review travis failure, seems like this change fails to build

NeilMacMullen · 2018-08-22T12:47:23Z

@cmonr Unfortunately I'm on vacation for the next couple of weeks so can't confirm for a while but I think the original issue report should make a reasonable unit test.

bulislaw

We need tests to confirm the fix.

…you just end up adding to the time instead of setting it. ARMmbed#7849

RobMeades · 2018-09-05T09:37:45Z

Can this fix be progressed please? It's not good to have something as fundamental as set_time() broken for so long.

0xc0170 · 2018-09-05T12:11:17Z

@TacoGrandeTX Can you please add a test case ?

TacoGrandeTX · 2018-09-05T16:50:08Z

Neil MacMullen's test case is a good one:

#include "mbed.h"

int main()
{
    for (int hour = 23; hour >= 0; hour--)
    {
        for (int z = 0; z < 12; z++)
        {
            printf("resetting time using hour %d...\n", hour);

            struct tm t
            {
            };
            t.tm_year = 2018 - 1900;
            t.tm_mon = 8 - 1;
            t.tm_mday = 16;
            t.tm_hour = hour;
            t.tm_min = 1;
            t.tm_sec = 1;
            set_time(mktime(&t));

            for (int c = 0; c < 10; c++)
            {
                time_t seconds = time(NULL);
                printf("%s", ctime(&seconds));
                wait_ms(1000);
            }
        }
    }
    return 0;
}

Before (note how seconds are not reset):

resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018
Thu Aug 16 23:01:04 2018
Thu Aug 16 23:01:05 2018
Thu Aug 16 23:01:06 2018
Thu Aug 16 23:01:07 2018
Thu Aug 16 23:01:08 2018
Thu Aug 16 23:01:09 2018
Thu Aug 16 23:01:10 2018
resetting time using hour 23...
Thu Aug 16 23:01:11 2018
Thu Aug 16 23:01:12 2018
Thu Aug 16 23:01:13 2018
Thu Aug 16 23:01:14 2018

Now (seconds are reset after calling set_time()):

resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018
Thu Aug 16 23:01:04 2018
Thu Aug 16 23:01:05 2018
Thu Aug 16 23:01:06 2018
Thu Aug 16 23:01:07 2018
Thu Aug 16 23:01:08 2018
Thu Aug 16 23:01:09 2018
Thu Aug 16 23:01:10 2018
resetting time using hour 23...
Thu Aug 16 23:01:01 2018
Thu Aug 16 23:01:02 2018
Thu Aug 16 23:01:03 2018

Tested this on nRF52_DK and also MAX32620FTHR as there was a community post like RobMeades' about set_time() not working more than once on that target.

cmonr · 2018-09-06T01:26:26Z

@TacoGrandeTX Something looks odd with this PR's history. Could you take a look at it?

0xc0170 · 2018-09-06T13:10:45Z

@TacoGrandeTX What @bulislaw requested, we have hal tests for RTC. If new test case could be added to check to confirm the fix

TacoGrandeTX · 2018-09-11T22:42:27Z

I've just created #8094 as a HAL test case.

0xc0170 · 2018-09-17T14:43:55Z

I've just created #8094 as a HAL test case.

To my understanding the test case should be part of this PR ? Otherwise test case PR cant be tested (fails). Can you add a test to this PR?

TacoGrandeTX · 2018-09-18T18:13:01Z

@0xc0170 Understood - I've brought the HAL test into this PR and have closed #8094.

kegilbert · 2018-10-25T18:19:59Z

@kjbracey-arm Has a good point, RTC->RTOS->LPTICKER->None sounds pretty solid to me.

Should be fairly straight forward to add that ordering here: https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_rtc_time.cpp#L25-L74

TacoGrandeTX · 2018-10-31T18:54:34Z

@cmonr:

If RTC is off, then how are the current tests being run against any device?

As @kjbracey-arm pointed out, the tests in mbed_hal/rtc_time are only testing _rtc_maketime() and _rtc_localtime() which don't rely on having a running clock!

@kjbracey-arm:

Maybe the new test should have gone into "mbed_drivers/rtc", where the other time() tests are.

I think I'm going to have to do that. Thanks for pointing out the proper location. I'll try this again.

However, I would really like the time() call to work on those platforms. It would be good to have an alternative implementation for RTOS using Kernel::get_ms_count(), which means it would.

Like events now do, it's better to ask the RTOS for the time it already has than to have a separate timer, so I'd have it in precedence order "RTC->RTOS->LPTICKER->nothing".

OK - I'll check with @kegilbert about raising a new issue for this.

kegilbert · 2018-10-31T23:14:30Z

@TacoGrandeTX Yeah a new issue would be perfect, sorry should've clarified that as an aside and not blocking for this PR.

Reset _rtc_lp_timer only if DEVICE_LPTICKER is defined.

When DEVICE_LPTICKER is defined set_time() only works correctly on the first call. This test calls set_time() twice and ensures the time set by both calls is correct. This test only runs if DEVICE_RTC or DEVICE_LPTICKER is defined.

TacoGrandeTX · 2018-11-02T15:27:13Z

Met with @studavekar yesterday and he advised protecting the test with #if, so that's been done. Performed a merge to pick up the template test changes so this should now be good to go.

studavekar · 2018-11-02T15:33:15Z

We need to revisit 6d90c27 and check if we can enable RTC on disabled devices.

0xc0170 · 2018-11-06T13:50:05Z

We need to revisit 6d90c27 and check if we can enable RTC on disabled devices.

Who's doind this action? Is this blocking this PR?

studavekar · 2018-11-06T15:29:04Z

Who's doind this action? Is this blocking this PR?

enablement RTC on disabled devices is not blocking this PR.

cmonr · 2018-11-06T22:23:51Z

@studavekar Mind creating an issue? The comment made it sound like this needed to be addressed in this PR.

0xc0170 · 2018-11-07T12:47:48Z

This is now entering CI

/morph build

mbed-ci · 2018-11-07T15:15:00Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-11-07T23:29:37Z

Exporter Build : FAILURE

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

mbed-ci · 2018-11-08T01:49:32Z

Test : FAILURE

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

cmonr · 2018-11-08T04:38:29Z

Note: This PR is now a part of a rollup PR (#8675).

Jenkins CI export nodes experienced many drops throughout the day causing false failures. In an attempt to get those PRs through CI, while keeping CI load low, several PRs have been bundled into a single rollup PR.

If any more commits are made in this PR, this PR will remain open and have to go through CI on its own.

0xc0170 · 2018-11-08T14:16:01Z

/morph test
/morph export-build

mbed-ci · 2018-11-08T16:09:24Z

Test : SUCCESS

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

mbed-ci · 2018-11-08T16:13:30Z

Exporter Build : SUCCESS

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

TacoGrandeTX mentioned this pull request Aug 21, 2018

RF52 serious behavioural regressions in RTC #7804

Closed

kegilbert requested a review from c1728p9 August 21, 2018 19:13

kegilbert reviewed Aug 21, 2018

View reviewed changes

TacoGrandeTX changed the title ~~Low power timer needs to be reset when setting time.~~ Low power timer needs to be reset when setting time Aug 21, 2018

cmonr added the needs: review label Aug 22, 2018

0xc0170 requested a review from a team August 22, 2018 08:13

0xc0170 added needs: work and removed needs: review labels Aug 22, 2018

bulislaw previously requested changes Aug 22, 2018

View reviewed changes

TacoGrandeTX mentioned this pull request Sep 4, 2018

set_time() bizarre behaviour on NRF52832 (AKA "am I going mad") #7983

Closed

RobMeades added a commit to u-blox/mbed-os that referenced this pull request Sep 5, 2018

Add fix to reset low power timer when set_time() is called otherwise …

865bb14

…you just end up adding to the time instead of setting it. ARMmbed#7849

kegilbert mentioned this pull request Sep 5, 2018

set_time not working on LPC1768 #7380

Closed

TacoGrandeTX mentioned this pull request Sep 11, 2018

Add a HAL set_time() test #8094

Closed

cmonr added the needs: preceding PR label Sep 12, 2018

RobMeades mentioned this pull request Sep 17, 2018

NRF52832: time stops after 35 minutes (or am I going mad again?) #8155

Closed

0xc0170 added the release-version: 5.10.1 label Sep 17, 2018

cmonr added needs: review and removed needs: preceding PR labels Sep 19, 2018

RFulchiero added 2 commits November 1, 2018 17:11

Low power timer needs to be reset when setting time.

d82990c

Reset _rtc_lp_timer only if DEVICE_LPTICKER is defined.

Add a protected HAL set_time() test

0de5ab2

When DEVICE_LPTICKER is defined set_time() only works correctly on the first call. This test calls set_time() twice and ensures the time set by both calls is correct. This test only runs if DEVICE_RTC or DEVICE_LPTICKER is defined.

kjbracey approved these changes Nov 2, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Nov 7, 2018

cmonr added the release-version: 5.10.4 label Nov 7, 2018

cmonr assigned cmonr and unassigned cmonr Nov 8, 2018

cmonr added the rollup PR label Nov 8, 2018

cmonr mentioned this pull request Nov 8, 2018

Rollup PR: Aleviating intermittent export issues #8675

Closed

0xc0170 mentioned this pull request Nov 8, 2018

Rollup PR #8676

Merged

cmonr removed the rollup PR label Nov 8, 2018

cmonr merged commit e635613 into ARMmbed:master Nov 8, 2018

cmonr removed the needs: CI label Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Low power timer needs to be reset when setting time #7849

Low power timer needs to be reset when setting time #7849

TacoGrandeTX commented Aug 21, 2018

TacoGrandeTX commented Aug 21, 2018

kegilbert left a comment

cmonr commented Aug 22, 2018

0xc0170 commented Aug 22, 2018

NeilMacMullen commented Aug 22, 2018

bulislaw left a comment

RobMeades commented Sep 5, 2018

0xc0170 commented Sep 5, 2018

TacoGrandeTX commented Sep 5, 2018

cmonr commented Sep 6, 2018

0xc0170 commented Sep 6, 2018

TacoGrandeTX commented Sep 11, 2018

0xc0170 commented Sep 17, 2018

TacoGrandeTX commented Sep 18, 2018

kegilbert commented Oct 25, 2018

TacoGrandeTX commented Oct 31, 2018

kegilbert commented Oct 31, 2018

TacoGrandeTX commented Nov 2, 2018

studavekar commented Nov 2, 2018

0xc0170 commented Nov 6, 2018

studavekar commented Nov 6, 2018

cmonr commented Nov 6, 2018

0xc0170 commented Nov 7, 2018

mbed-ci commented Nov 7, 2018

mbed-ci commented Nov 7, 2018

mbed-ci commented Nov 8, 2018

cmonr commented Nov 8, 2018

0xc0170 commented Nov 8, 2018

mbed-ci commented Nov 8, 2018

mbed-ci commented Nov 8, 2018

Low power timer needs to be reset when setting time #7849

Low power timer needs to be reset when setting time #7849

Conversation

TacoGrandeTX commented Aug 21, 2018

Description

Pull request type

TacoGrandeTX commented Aug 21, 2018

kegilbert left a comment

Choose a reason for hiding this comment

cmonr commented Aug 22, 2018

0xc0170 commented Aug 22, 2018

NeilMacMullen commented Aug 22, 2018

bulislaw left a comment

Choose a reason for hiding this comment

RobMeades commented Sep 5, 2018

0xc0170 commented Sep 5, 2018

TacoGrandeTX commented Sep 5, 2018

cmonr commented Sep 6, 2018

0xc0170 commented Sep 6, 2018

TacoGrandeTX commented Sep 11, 2018

0xc0170 commented Sep 17, 2018

TacoGrandeTX commented Sep 18, 2018

kegilbert commented Oct 25, 2018

TacoGrandeTX commented Oct 31, 2018

kegilbert commented Oct 31, 2018

TacoGrandeTX commented Nov 2, 2018

studavekar commented Nov 2, 2018

0xc0170 commented Nov 6, 2018

studavekar commented Nov 6, 2018

cmonr commented Nov 6, 2018

0xc0170 commented Nov 7, 2018

mbed-ci commented Nov 7, 2018

Build : SUCCESS

Triggering tests

mbed-ci commented Nov 7, 2018

Exporter Build : FAILURE

mbed-ci commented Nov 8, 2018

Test : FAILURE

cmonr commented Nov 8, 2018

0xc0170 commented Nov 8, 2018

mbed-ci commented Nov 8, 2018

Test : SUCCESS

mbed-ci commented Nov 8, 2018

Exporter Build : SUCCESS