Fire interrupt function broken for nrf5x targets

[0xc0170]

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 - #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-

[screamerbg]

cc @ARMmbed/team-nordic

[jiandeng]

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?

[0xc0170]

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

[pan-]

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

[0xc0170]

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]

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.

[fkjagodzinski]

The proposed fix provided in d006eaa 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.

[pan-]

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

[0xc0170]

cc @ARMmbed/team-nordic Can you review these specific lines: d006eaa#diff-80a5f6c9883e4a449adb8fb70684d593R41

How should fire interrupt be implemented for nordic targets?

[nvlsianpu]

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

[bulislaw]

ping