[NRF5]: fix rtc overflow-while-set-timestamp issue #4016

nvlsianpu · 2017-03-24T11:32:03Z

Description

bug:
If rtc overflow occur while setting of timestamp then the compare event ocurr (erroneously) in 512s.
This cause #3857 issue.

changes:

move rtc ovf handler at the beginning of rtc handler for mitigate the case (mitigate issue for execution from rtc handler)
add repeating of operation of set a timestamp in case that rtc overflow occurred during the operation.

Status

READY

…mpara-event ocurre (erroneusly) in 512s. - move ovf handler at the begining of rtc handler for mitigate the case (mitigate issue for exexution from rtc handler) - add repeating of operation of set a timestamp in cas that rtc overflow occured during the operation.

nvlsianpu · 2017-03-24T14:04:47Z

@anangl I need your review. cc: @pan-

nvlsianpu · 2017-03-24T16:16:20Z

For curious images:
general bug behaviour:

time window of the bug, one tick of RTC timer last ~30 us:

pan- · 2017-03-27T10:48:06Z

@nvlsianpu Could you also look at #4026 ?

bridadan · 2017-03-29T01:33:31Z

/morph test-nightly

0xc0170 · 2017-03-29T10:14:55Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

+
+        // check in case of preemption by RTC OVF event (apply if call was from a low priority level)
+        if (prev_overflows != m_common_rtc_overflows)
+        {


nit: code style

if (cond) { } else { } while (1) { }

pan-

The changes indicated in #4026 (comment) have to be included in this PR.

anangl · 2017-03-29T09:43:05Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

+        // Don't disable this event. It shall occur periodically.
+
+        ++m_common_rtc_overflows;
+    }


I'd extract the above block to some static function, e.g. check_overflow, since the same code needs to be executed in two places.

anangl · 2017-03-29T10:02:00Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

+{
+    uint32_t prev_overflows;
+
+    while (1)


Such a loop should not be placed here, but in common_rtc_32bit_ticks_get(), since this problematic gluing of hardware ticks with software counted overflows can be called from other places as well; currently it is called from us_ticker_read() and lp_ticker_read().
I'd propose to put it like this (not tested):

uint32_t common_rtc_32bit_ticks_get(void) { uint32_t ticks; uint32_t prev_overflows; do { prev_overflows = m_common_rtc_overflows; ticks = nrf_rtc_counter_get(COMMON_RTC_INSTANCE); // The counter used for time measurements is less than 32 bit wide, // so its value is complemented with the number of registered overflows // of the counter. ticks += (m_common_rtc_overflows << RTC_COUNTER_BITS); int was_masked = __sd_nvic_irq_disable(); check_overflow(); if (!was_masked) { __sd_nvic_irq_enable(); } } while (m_common_rtc_overflows != prev_overflows); return ticks; }

Anyway this won't fix overflow issue for setting a timieout. Overflow might occurs during executing of internal_common_rtc_set_interrupt for instance. But preserve time readout is problem as well, see #4026

mbed-bot · 2017-03-29T14:45:05Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1776

All builds and test passed!

0xc0170 · 2017-03-29T15:11:46Z

The changes indicated in #4026 (comment) have to be included in this PR.

@nvlsianpu What do you think?

nvlsianpu · 2017-03-30T23:33:01Z

0xc0170 Sure - good idea.

anangl · 2017-04-03T12:27:25Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

-        break;
-    }
+        // check in case that OVF occurred during execution of a RTC handler (apply if call was from RTC handler)
+    } while (rtc_ovf_evet_check() != prev_overflows);


This loop is no longer needed, because now common_rtc_32bit_ticks_get() provides the protection.

anangl · 2017-04-03T12:32:47Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

    if (nrf_rtc_event_pending(COMMON_RTC_INSTANCE, NRF_RTC_EVENT_OVERFLOW)) {
        nrf_rtc_event_clear(COMMON_RTC_INSTANCE, NRF_RTC_EVENT_OVERFLOW);
        // Don't disable this event. It shall occur periodically.

        ++m_common_rtc_overflows;
    }

+    return m_common_rtc_overflows;


This is a bit ineffective. This variable is volatile, so it will be re-read every time, and the return value from the first call of this function is not used at all (see line 75). It would be better to not return anything here and read m_common_rtc_overflows when it is really needed (i.e. in line 182).

anangl · 2017-04-03T12:33:29Z

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c

-#else
-void COMMON_RTC_IRQ_HANDLER(void)
-#endif
+__STATIC_INLINE uint32_t rtc_ovf_evet_check(void)


Did you mean rtc_ovf_event_check?

- extarct for check rtc overflow - make common_rtc_32bit_ticks_get safe against preemption.

nvlsianpu · 2017-04-04T07:11:48Z

@pan- PR is updated to fix #4026. I verified that this PR resolves issues once more. @0xc0170 the Travis CI build has trouble with it-self.

0xc0170 · 2017-04-04T08:30:42Z

@0xc0170 the Travis CI build has trouble with it-self.

Restarted

cc @Sissors

Sissors · 2017-04-04T09:36:34Z

Since the structure of this file is quite different from the NRF51822 one it is hard to say for me if I think it is solved. So @nvlsianpu , did you check with my test program if no errors occur?

nvlsianpu · 2017-04-04T10:37:21Z

@Sissors

did you check with my test program if no errors occur?

Yes I checked. It pass this as well.

0xc0170 · 2017-04-04T11:50:45Z

@pan- review please

nvlsianpu · 2017-04-05T19:16:03Z

@0xc0170 what happened on continuous-integration/jenkins/pr-head?

pan- · 2017-04-05T22:14:54Z

@0xc0170 LGTM

geky · 2017-04-05T22:31:32Z

There is an issue with the continuous-integration/jenkins/pr-head that is being progressed here.

In the meantime we can run this guy. The morph test runs all of the unit tests in the code base, so if morph passes it's unlikely that continuous-integration/jenkins/pr-head will fail.
/morph test

mbed-bot · 2017-04-06T00:33:30Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1842

All builds and test passed!

Ports for Upcoming Targets Fixes and Changes 4008: [NUC472/M453] Support Bootloader and FlashIAP ARMmbed/mbed-os#4008 4102: Add SCL and SDA defs for I2C[0-2]; redefine I2C_[SCL,SDA] to I2C2 ARMmbed/mbed-os#4102 4118: STM32F4 Internal ADC channels rework ARMmbed/mbed-os#4118 4126: STM32F4 : remove SERIAL_TX and SERIAL_RX from available pins ARMmbed/mbed-os#4126 4148: Revert "STM32F4 Internal ADC channels rework" ARMmbed/mbed-os#4148 4152: STM32F4 Internal ADC channels rework ARMmbed/mbed-os#4152 4074: [Silicon Labs] Update pinout ARMmbed/mbed-os#4074 4133: U-BLOX_C030: Default XTAL is now 12MHz onboard. Option to use Debug 8MHz ARMmbed/mbed-os#4133 4142: EFM32: Fixed `pwmout_all_inactive` being inversed ARMmbed/mbed-os#4142 4016: [NRF5]: fix rtc overflow-while-set-timestamp issue ARMmbed/mbed-os#4016 4031: STM32 increase IAR heap size for big RAM targets ARMmbed/mbed-os#4031 4137: MCUXpresso: Update ARM linker files to eliminate reserving RAM for stack & heap ARMmbed/mbed-os#4137 4176: STM32L4 Internal ADC channels rework ARMmbed/mbed-os#4176 4154: STM32F7 Internal ADC channels rework ARMmbed/mbed-os#4154 4174: [NRF52840]: fix rtc overflow-while-set-timestamp issue ARMmbed/mbed-os#4174 4180: [UBLOX_C030] create target hierarchy for the specific versions of the C030 board ARMmbed/mbed-os#4180 4153: STM32F2: Internal ADC channels rework ARMmbed/mbed-os#4153

nvlsianpu mentioned this pull request Mar 24, 2017

nRF52 timer problem #3857

Closed

nvlsianpu force-pushed the nrf5_rtc_ovf_bugfix branch from 191d0a9 to c6ef2f3 Compare March 24, 2017 14:01

0xc0170 added the needs: review label Mar 27, 2017

pan- mentioned this pull request Mar 27, 2017

NRF51822 timer has a race condition #4026

Closed

0xc0170 reviewed Mar 29, 2017

View reviewed changes

0xc0170 requested a review from pan- March 29, 2017 10:15

pan- requested changes Mar 29, 2017

View reviewed changes

anangl suggested changes Mar 29, 2017

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Mar 29, 2017

anangl suggested changes Apr 3, 2017

View reviewed changes

[NRF5] us_ticker:

4f99aa5

- extarct for check rtc overflow - make common_rtc_32bit_ticks_get safe against preemption.

nvlsianpu force-pushed the nrf5_rtc_ovf_bugfix branch from 8de8a60 to 4f99aa5 Compare April 3, 2017 14:10

anangl approved these changes Apr 4, 2017

View reviewed changes

smarek mentioned this pull request Apr 4, 2017

Timer/Ticker/RTC/Thread issue on NRF51_DK and derived targets. Porting to custom board docs? #4108

Closed

pan- approved these changes Apr 5, 2017

View reviewed changes

nvlsianpu mentioned this pull request Apr 12, 2017

[NRF52840]: fix rtc overflow-while-set-timestamp issue #4174

Merged

sg- added ready for merge and removed needs: work labels Apr 12, 2017

adbridge added release-version: 5.4.4 and removed ready for merge labels Apr 20, 2017

adbridge merged commit 78c4850 into ARMmbed:master Apr 20, 2017

[NRF5]: fix rtc overflow-while-set-timestamp issue #4016

[NRF5]: fix rtc overflow-while-set-timestamp issue #4016

Uh oh!

Conversation

nvlsianpu commented Mar 24, 2017 • edited by pan- Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Uh oh!

nvlsianpu commented Mar 24, 2017

Uh oh!

nvlsianpu commented Mar 24, 2017

Uh oh!

pan- commented Mar 27, 2017

Uh oh!

bridadan commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- left a comment

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!

mbed-bot commented Mar 29, 2017

Output

Uh oh!

0xc0170 commented Mar 29, 2017

Uh oh!

nvlsianpu commented Mar 30, 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!

nvlsianpu commented Apr 4, 2017

Uh oh!

0xc0170 commented Apr 4, 2017

Uh oh!

Sissors commented Apr 4, 2017

Uh oh!

nvlsianpu commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 4, 2017

Uh oh!

nvlsianpu commented Apr 5, 2017

Uh oh!

pan- commented Apr 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

mbed-bot commented Apr 6, 2017

Output

Uh oh!

Uh oh!

nvlsianpu commented Mar 24, 2017 •

edited by pan-

Loading

nvlsianpu commented Apr 4, 2017 •

edited

Loading

geky commented Apr 5, 2017 •

edited

Loading