Mbed-os: Fire interrupt function broken for nrf5x targets

Created on 9 Oct 2017 · 18Comments · Source: ARMmbed/mbed-os

Description

Type: Bug
Priority: Minor

Bug

Target
Any nrf51 possibly

Toolchain:
Any

Expected behavior

Tickers (us or lp) should set pending bit and ticker irq handler should be instantly called.

Actual behavior

Test fails (no callback is called). If I replace fire_interrupt with invoking directly isr ticker handler, then it works.

+--------------+---------------+-------------------------------+-----------------------------------+--------+--------+--------+--------------------+
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test insert                       | 1      | 0      | OK     | 0.09               |
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test insert zero                  | 1      | 0      | OK     | 0.04               |
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test insert_absolute              | 1      | 0      | OK     | 0.1                |
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test insert_absolute zero         | 0      | 1      | FAIL   | 0.08               |
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test remove after insert          | 1      | 0      | OK     | 0.2                |
| NRF51_DK-ARM | NRF51_DK      | tests-mbed_drivers-timerevent | Test remove after insert_absolute | 1      | 0      | OK     | 0.21               |
+--------------+---------------+-------------------------------+-----------------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 1 FAIL / 5 OK

Steps to reproduce

Fetch this patch - https://github.com/ARMmbed/mbed-os/pull/5046, run tests and should see the failure.

This is nrf5 targets implementation for us ticker fire intterupt on master currently:

void us_ticker_fire_interrupt(void)
{
    uint32_t closest_safe_compare = common_rtc_32bit_ticks_get() + 2;
    nrf_rtc_cc_set(COMMON_RTC_INSTANCE, US_TICKER_CC_CHANNEL, RTC_WRAP(closest_safe_compare));
    nrf_rtc_event_enable(COMMON_RTC_INSTANCE, US_TICKER_INT_MASK);
}

This I believe should be for nrf51822 unified targets (on master it's not complete):

void us_ticker_fire_interrupt(void)
{
    uint32_t newCallbackTime = rtc1_getCounter() + FUZZY_RTC_TICKS;
    NRF_RTC1->CC[0]             = newCallbackTime & MAX_RTC_COUNTER_VAL;
    us_ticker_callbackTimestamp = newCallbackTime;
    us_ticker_callbackPending = true;
    rtc1_enableCompareInterrupt();
}

cc @nvlsianpu @anangl @pan-

nordic bug

Source

0xc0170

All 18 comments

cc @ARMmbed/team-nordic

screamerbg on 9 Oct 2017

The nrf5 targets implementation will trigger interrupt in around two ticks instead of triggering it instantly. Failing the insert_absolute zero case. If there is no way to actually trigger RTC.CC interrupt instantly, shall we consider trigger an SWI and call interrupt handler in the SWI?

jiandeng on 10 Oct 2017

The nrf5 targets implementation will trigger interrupt in around two ticks instead of triggering it instantly. Failing the insert_absolute zero case. If there is no way to actually trigger RTC.CC interrupt instantly, shall we consider trigger an SWI and call interrupt handler in the SWI?

That would be possibility, however this should be sufficient as it is (we got few targets that share similar implementation - scheduling the next interrupt as soon as possible).
As pointed out above, the implementation does not look correct for nrf5x targets, I got it fixed at least seems to me should be functional.

Please see the referenced PR above that is failing due to this bug. We will improve the test cases to handle targets that schedule ticker isr at the next possible tick (like nrf5x do, 2 ticks away).

0xc0170 on 10 Oct 2017

Immediate scheduling of the IRQ would be better if a solution is found. ~~Did you try to use nrf_rtc_event_enable ?~~

pan- on 10 Oct 2017

The code snippet is shared above, NRF51_DK tested with this for fire interrupt:

void us_ticker_fire_interrupt(void)
{
    core_util_critical_section_enter();
    uint32_t closest_safe_compare = common_rtc_32bit_ticks_get() + 2;

    nrf_rtc_cc_set(COMMON_RTC_INSTANCE, US_TICKER_CC_CHANNEL, RTC_WRAP(closest_safe_compare));
    nrf_rtc_event_enable(COMMON_RTC_INSTANCE, US_TICKER_INT_MASK);
    core_util_critical_section_exit();
}

0xc0170 on 10 Oct 2017

after discussion with @pan- , we are proposing a fix for this - using sw flag to indicate SW IRQ SET. Plus set pending IRQ. Thus in IRQ handler we check if SW IRQ was requested, and invoke ticker handler. The fix is now in the test PR referenced here.

Tested with nrf51_dk, test is now passing. It should be something simialar to what @jiandeng proposed above.

0xc0170 on 10 Oct 2017

The proposed fix provided in d006eaacbfafc29c6b14905ff8cc4afb266b5fe2 makes timerevent tests pass. Unfortunately a side effect is present -- a regression for GCC_ARM and IAR compilers on tests-mbed_hal-sleep_manager_racecondition and tests-mbed_drivers-ticker test cases. It's worth to point that everything is fine for ARM compiler though.

fkjagodzinski on 17 Nov 2017

@nvlsianpu Can you look at that ? It may be related to things seen in #5297 .

pan- on 17 Nov 2017

cc @ARMmbed/team-nordic Can you review these specific lines: https://github.com/ARMmbed/mbed-os/commit/d006eaacbfafc29c6b14905ff8cc4afb266b5fe2#diff-80a5f6c9883e4a449adb8fb70684d593R41

How should fire interrupt be implemented for nordic targets?

0xc0170 on 20 Nov 2017

@0xc0170 I will review and comment in a few days - Unfortunately I have other important task at the moment.

nvlsianpu on 20 Nov 2017

ping

bulislaw on 27 Nov 2017

@nvlsianpu Any proposals for this? I would like to understand better the failures if a next event is scheduled asap, fired instantly.

0xc0170 on 5 Dec 2017

@0xc0170 I will review and comment in a few days - Unfortunately I have other important task at the moment.

How can we help? We had some ideas above, but did not find a solution to this problem

0xc0170 on 15 Dec 2017

What is the status here. I run this test and It passed.

nvlsianpu on 19 Dec 2017

@nvlsianpu PR #5046 contains a proposed fix (commit d006eaacbfafc29c6b14905ff8cc4afb266b5fe2) which makes tests-mbed_drivers-timerevent pass but the fix has a side effect -- a regression for GCC_ARM and IAR compilers on tests-mbed_hal-sleep_manager_racecondition and tests-mbed_drivers-ticker test cases (everything is fine for ARM compiler though).

fkjagodzinski on 19 Dec 2017

👍1

@nvlsianpu PR #5046 contains a proposed fix (commit d006eaa) which makes tests-mbed_drivers-timerevent pass but the fix has a side effect -- a regression for GCC_ARM and IAR compilers on tests-mbed_hal-sleep_manager_racecondition and tests-mbed_drivers-ticker test cases (everything is fine for ARM compiler though).

Are you able to reproduce @nvlsianpu ?

0xc0170 on 8 Jan 2018

@fkjagodzinski We have a cyclic PR dependency issue. Is PR #5046 needed for this PR or is this PR needed for PR #5046?