Mbed-os: missed out on "fire_interrupt"

[maciejbocianski]

Bug catched while #6515 testing on NUCLEO_F746ZG and ARM compiler on CI

The problem arises from different time base used in our hal and target implementation.
HAL ticker hal/mbed_ticker_api.c use [us] while target implementation targets/TARGET_STM/lp_ticker.c use [ticks] (on this target 1[tick] == 61[us])

Fails catched in #6515 is the case when we try to set interrupt with time now + 3[tick].
Due to interrupt setting last 3[tick] it's not fired. But hopefully _ticker_match_interval_passed
should catch this but it doesn't

While comparing cur_tick >= match_tick in _ticker_match_interval_passed in us it fails.
Time read from ticker->interface->read() is count in 61us intervals thus the problem.

So when ticker frequency is much less then 1MHz the then cur_tick != match_tick will be almost always true and _ticker_match_interval_passed won't trigger off

prev_tick ==10306438[us]
cur_tick==10306621[us]
match_tick==10306642[us]

10306621[us] == 168961[tick]
10306642[us] == 168961[tick]

This problem is not present in new HAL API since it use ticks instead us.

https://github.com/ARMmbed/mbed-os/blob/c8d72c524dd7ac1b537bd5fe230572cb703f0532/hal/mbed_ticker_api.c#L246-L251

Potential fix, add additional check ((match_tick - cur_tick) <= lp_TickPeriod_us):

uint32_t lp_TickPeriod_us;
int _ticker_match_interval_passed(timestamp_t prev_tick, timestamp_t cur_tick, timestamp_t match_tick)
{
    if (match_tick > prev_tick) {
        return (cur_tick >= match_tick) || ((match_tick - cur_tick) <= lp_TickPeriod_us) || (cur_tick < prev_tick);
    } else {
        return (cur_tick < prev_tick) && (cur_tick >= match_tick);
    }
}

Description

• Type: Bug
• Related issue: #6515
• Priority: Major

---

Bug

Target
NUCLEO_F746ZG|likely all

Toolchain:
ARM

Toolchain version:

mbed-cli version:
1.2.0

mbed-os sha:
8b660eb3e Ticker test: Add ticker_irq_handler call because the time reaching the previous interrupt_timestamp would have triggered one anyway
bfb28a7a8 Only schedule mbed_ticker interrupt if queue->head is changed
6634e4606 (origin/master, origin/HEAD, master) Merge pull request #6547 from marcuschangarm/feature-nrf52
b964420bb Reorganize targets.json for NRF52 based targets

DAPLink version:

Expected behavior
test_multi_ticker should pass

Actual behavior
test_multi_ticker test is failing

Steps to reproduce

Get mbed master and Apply #6515
then run:
mbed test -t ARM -m NUCLEO_F746ZG -n tests-mbed_drivers-lp_ticker -vv --profile develop_ci
where develop_ci is develop profile plus flags used on CI: "-DMBED_TRAP_ERRORS_ENABLED=1", "-DMBED_HEAP_STATS_ENABLED=1", "-DMBED_STACK_STATS_ENABLED=1"

[0xc0170]

@ARMmbed/team-st-mcd @c1728p9

Potential fix, add additional check ((match_tick - cur_tick) <= lp_TickPeriod_us):

I dont see in hte code snippet what was added (only that condition) ?

[maciejbocianski]

@0xc0170
Yes only the condition.

Here is branch used for bug reproduction
https://github.com/maciejbocianski/mbed-os/tree/debugging_only_schedule_head_multiticker
Last commit contains fix specific for NUCLEO_F746ZG target

It will be hard to have generic fix for all targets (on master) since lp_ticker_get_info function is stubbed on master and return 1MHz frequency for most targets. It's crucial to compute tick period.
Could implement it for NUCLEO_F746ZG.

[maciejbocianski]

@ARMmbed/team-st-mcd
this loop takes 3 tick (183us) for NUCLEO_F746ZG
it's OK ?
https://github.com/ARMmbed/mbed-os/blob/c8d72c524dd7ac1b537bd5fe230572cb703f0532/targets/TARGET_STM/lp_ticker.c#L252-L253

[jeromecoutant]

Hi

This problem is not present in new HAL API since it use ticks instead us

Did you check the issue in the ticker branch ?
So what do you expect from us ...?

[c1728p9]

@maciejbocianski there is a mechanism already in place to handle low power tickers which take multiple clock cycles to synchronize - LOWPOWERTIMER_DELAY_TICKS - https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_lp_ticker_wrapper.cpp#L18

Does this solve the problem you are seeing?

[maciejbocianski]

@c1728p9 yes it solves the problem
setting 3 tick delay for this target (1tick == 61us for NUCLEO_F746ZG)

diff --git a/targets/targets.json b/targets/targets.json
index b283c4f5f..0399de8d0 100755
--- a/targets/targets.json
+++ b/targets/targets.json
@@ -1345,7 +1345,7 @@
                 "value": 1
             }
         },
-        "macros_add": ["USBHOST_OTHER"],
+        "macros_add": ["USBHOST_OTHER", "LOWPOWERTIMER_DELAY_TICKS=183"],
         "supported_form_factors": ["ARDUINO"],
         "detect_code": ["0816"],
         "device_has_add": ["ANALOGOUT", "CAN", "LOWPOWERTIMER", "SERIAL_ASYNCH", "TRNG", "FLASH"],

[0xc0170]

Shall this be closed with reference to the ticker branch that will fix this ?

[maciejbocianski]

@0xc0170 I think so, and #6515 could be postponed till ticker feature branch merging

[ciarmcom]

ARM Internal Ref: MBOTRIAGE-59

[maciejbocianski]

ticker feature branch was merged

6515 also was merged

Issue solved