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

RF52 serious behavioural regressions in RTC #7804

Closed
NeilMacMullen opened this Issue Aug 16, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@NeilMacMullen

NeilMacMullen commented Aug 16, 2018

GCC_ARM
nRF52 platform.
mbed-os-5.9.4

See #7763 for the likely breaking change.

The following code demonstrates that the seconds and minutes are NOT being correctly reset when calling set_time.

Further, other strange behaviour includes that described in #7763 where setting the clock (backwards?) can sometimes cause it to 'pause' for 35 minutes (1<<31 us).

I'm pretty sure I've also seen the clock drift significantly (several minutes over the course of an hour).

#include "mbed.h"

#include "segger_rtt.h"

int main()
{
    for (int hour = 23; hour >= 0; hour--)
    {
        for (int z = 0; z < 12; z++)
        {
            SEGGER_RTT_printf(0, "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);
                SEGGER_RTT_printf(0, "%s", ctime(&seconds));
                wait_ms(1000);
            }
        }
    }
    return 0;
}

Here's typical output. Note that the seconds/minutes are not being reset but hours are. I've confirmed the behaviour is correct on 5.8.5

0> resetting time using hour 23...
0> Thu Aug 16 23:02:41 2018
0> Thu Aug 16 23:02:41 2018
0> Thu Aug 16 23:02:42 2018
0> Thu Aug 16 23:02:43 2018
0> Thu Aug 16 23:02:44 2018
0> Thu Aug 16 23:02:45 2018
0> Thu Aug 16 23:02:46 2018
0> Thu Aug 16 23:02:47 2018
0> Thu Aug 16 23:02:48 2018
0> Thu Aug 16 23:02:49 2018
0> resetting time using hour 23...
0> Thu Aug 16 23:02:51 2018
0> Thu Aug 16 23:02:51 2018
0> Thu Aug 16 23:02:52 2018
0> Thu Aug 16 23:02:53 2018
0> Thu Aug 16 23:02:54 2018
0> Thu Aug 16 23:02:55 2018
0> Thu Aug 16 23:02:56 2018
0> Thu Aug 16 23:02:57 2018
0> Thu Aug 16 23:02:58 2018
0> Thu Aug 16 23:02:59 2018
0> resetting time using hour 22...
0> Thu Aug 16 22:03:01 2018
0> Thu Aug 16 22:03:01 2018
0> Thu Aug 16 22:03:02 2018
0> Thu Aug 16 22:03:03 2018
0> Thu Aug 16 22:03:04 2018
0> Thu Aug 16 22:03:05 2018
0> Thu Aug 16 22:03:06 2018
0> Thu Aug 16 22:03:07 2018
0> Thu Aug 16 22:03:08 2018
0> Thu Aug 16 22:03:09 2018
0> resetting time using hour 22...
0> Thu Aug 16 22:03:11 2018
0> Thu Aug 16 22:03:11 2018
0> Thu Aug 16 22:03:12 2018
0> Thu Aug 16 22:03:13 2018
0> Thu Aug 16 22:03:14 2018
0> Thu Aug 16 22:03:15 2018
0> Thu Aug 16 22:03:16 2018
0> Thu Aug 16 22:03:17 2018
0> Thu Aug 16 22:03:18 2018
0> Thu Aug 16 22:03:19 2018

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 16, 2018

@MarceloSalazar @pan- @mprse
[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 16, 2018

set_time seems to have been broken between 5.8.6 ( expected behaviour) and 5.9.0 (incorrect)
[Mirrored to Jira]

@ciarmcom ciarmcom added the mirrored label Aug 16, 2018

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 16, 2018

For good measure I verified it is also broken on 5.9.5
[Mirrored to Jira]

@TacoGrandeTX

This comment has been minimized.

Contributor

TacoGrandeTX commented Aug 17, 2018

@NeilMacMullen We're still root-causing this issue, but a work-around to the problem with set_time() is to re-introduce the "RTC" property to the "Device Has" list in targets.json for the MCU_NRF52832. We see that working properly with your test code. However, it's not clear what other ramifications that will have on your end app.

[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 17, 2018

Thanks @TacoGrandeTX I'll do some testing with this today and let you know of any relevant findings.
[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 17, 2018

On the basis of some light testing, this does look to have fixed the issues I was seeing and even works with the low-power workaround I describe in #7763. Thanks!
[Mirrored to Jira]

@TacoGrandeTX

This comment has been minimized.

Contributor

TacoGrandeTX commented Aug 17, 2018

@NeilMacMullen Thanks for the report - that's good to hear. Even without DEVICE_RTC we are still using RTC1 (running at 32kHz) for updating our internal ticker for time-keeping. Our internal rtc_lp_base is being set appropriately by set_time() but we might be mishandling the internal ticker. We will have more for you early next week.

[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 18, 2018

It seems I spoke too soon :( Although I haven't managed to find a simple repro case, the esp8266-driver is now failing consistently (after RTC added to targets.json). It looks like the Timer implementation is not returning sensible values for read_ms but I haven't yet been able to verify directly.
[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 20, 2018

Further to the above comment..... The difference in behaviour actually occurs when there are two threads in the system, a producer 'A' and a consumer 'B' .

A is running a tight loop using wait_ms(1) to interject some idle time between checks of a peripheral.
B is busy-waiting in a while(1) loop (checking a status flag set by B.

In this scenario, when the RTC feature is added, thread A stalls on the wait_ms(1) presumably because the scheduler is not pre-empting B at all. As evidence for this, as soon as B abandons its loop, the A loop resumes.

Adding a small wait inside the B loop allows the code to work correctly (as does reverting the RTC feature).

Clearly, a better design would be for the code in B to use waits but it's interesting the behaviour changes as RTC is added/removed.

FYI, the B code is the get/putc code in https://os.mbed.com/teams/ESP8266/code/esp8266-driver/file/6946b0b9e323/ESP8266/ATParser/ATParser.cpp/
[Mirrored to Jira]

@TacoGrandeTX

This comment has been minimized.

Contributor

TacoGrandeTX commented Aug 20, 2018

@NeilMacMullen I'm testing a fix that is looking good. The timer used to keep track of usec was not being reset on set_time():

void set_time(time_t t)
{
    _mutex->lock();
    if (_rtc_init != NULL) {
        _rtc_init();
    }
    if (_rtc_write != NULL) {
        _rtc_write(t);
        _rtc_lp_timer->reset();
    }
    _mutex->unlock();
}

Will update again ASAP (hopefully with a pull request).
[Mirrored to Jira]

@TacoGrandeTX

This comment has been minimized.

Contributor

TacoGrandeTX commented Aug 21, 2018

@NeilMacMullen - please see PR #7849
[Mirrored to Jira]

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 22, 2018

Thanks Ralph - does this mean the DEVICE_RTC change is now redundant?
[Mirrored to Jira]

@TacoGrandeTX

This comment has been minimized.

Contributor

TacoGrandeTX commented Aug 22, 2018

@NeilMacMullen - Yes it is redundant, sorry for not clarifying that. This fixes the original problem with no DEVICE_RTC. As you mentioned having other end-issues with DEVICE_RTC it would be good to test without it.
[Mirrored to Jira]

@ARMmbed ARMmbed deleted a comment from ciarmcom Oct 2, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 4, 2018

Internal Jira reference: https://jira.arm.com/browse/IOTDEV-1603

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2018

@NeilMacMullen - please see PR #7849

@NeilMacMullen Is this still valid issue and present on master?

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