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

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

Merged
merged 2 commits into from Apr 20, 2017

Conversation

Projects
None yet
10 participants
@nvlsianpu
Contributor

nvlsianpu commented Mar 24, 2017

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

@nvlsianpu nvlsianpu referenced this pull request Mar 24, 2017

Closed

nRF52 timer problem #3857

[NRF5]: If rtc overflow occurr while setting of timestam then the cco…
…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 nvlsianpu force-pushed the nvlsianpu:nrf5_rtc_ovf_bugfix branch to c6ef2f3 Mar 24, 2017

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Mar 24, 2017

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

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Mar 24, 2017

For curious images:
general bug behaviour:
Imgur
time window of the bug, one tick of RTC timer last ~30 us:
Imgur

@pan-

This comment has been minimized.

Member

pan- commented Mar 27, 2017

@nvlsianpu Could you also look at #4026 ?

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 29, 2017

/morph test-nightly

// 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)
{

This comment has been minimized.

@0xc0170

0xc0170 Mar 29, 2017

Member

nit: code style


if (cond) {

} else {

}

while (1) {

}

@0xc0170 0xc0170 requested a review from pan- Mar 29, 2017

@pan-

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

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

This comment has been minimized.

@anangl

anangl Mar 29, 2017

Contributor

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.

{
uint32_t prev_overflows;
while (1)

This comment has been minimized.

@anangl

anangl Mar 29, 2017

Contributor

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;
}

This comment has been minimized.

@nvlsianpu

nvlsianpu Mar 29, 2017

Contributor

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

This comment has been minimized.

mbed-bot commented Mar 29, 2017

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

This comment has been minimized.

Member

0xc0170 commented Mar 29, 2017

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

@nvlsianpu What do you think?

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

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Mar 30, 2017

0xc0170 Sure - good idea.

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c Outdated
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 comment has been minimized.

@anangl

anangl Apr 3, 2017

Contributor

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

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c Outdated
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 comment has been minimized.

@anangl

anangl Apr 3, 2017

Contributor

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).

targets/TARGET_NORDIC/TARGET_NRF5/us_ticker.c Outdated
#else
void COMMON_RTC_IRQ_HANDLER(void)
#endif
__STATIC_INLINE uint32_t rtc_ovf_evet_check(void)

This comment has been minimized.

@anangl

anangl Apr 3, 2017

Contributor

Did you mean rtc_ovf_event_check?

[NRF5] us_ticker:
- extarct for check rtc overflow
- make common_rtc_32bit_ticks_get safe against preemption.

@nvlsianpu nvlsianpu force-pushed the nvlsianpu:nrf5_rtc_ovf_bugfix branch to 4f99aa5 Apr 3, 2017

@anangl

anangl approved these changes Apr 4, 2017

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Apr 4, 2017

@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

This comment has been minimized.

Member

0xc0170 commented Apr 4, 2017

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

Restarted

cc @Sissors

@Sissors

This comment has been minimized.

Contributor

Sissors commented Apr 4, 2017

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

This comment has been minimized.

Contributor

nvlsianpu commented Apr 4, 2017

@Sissors

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

Yes I checked. It pass this as well.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 4, 2017

@pan- review please

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Apr 5, 2017

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

@pan-

pan- approved these changes Apr 5, 2017

@pan-

This comment has been minimized.

Member

pan- commented Apr 5, 2017

@0xc0170 LGTM

@geky

This comment has been minimized.

Member

geky commented Apr 5, 2017

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

This comment has been minimized.

mbed-bot commented Apr 6, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1842

All builds and test passed!

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

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment