FuriEventFlag: furi_event_flag_clear() notifies the event loop before the ISR-deferred clear is applied

[herbenderbler]

Summary

furi_event_flag_clear() has the same deferred-set/early-notify defect on its ISR path that #4336 reported for furi_event_flag_set(), and that PR #4358 fixes for the set side. The clear side is untouched and still affected.

This is latent: no in-tree caller reaches it today (see Reachability). Filing it as a follow-up rather than folding it into #4358, per discussion there.

The defect

furi/core/event_flag.c, furi_event_flag_clear():

if(FURI_IS_IRQ_MODE()) {
    rflags = xEventGroupGetBitsFromISR(hEventGroup);

    if(xEventGroupClearBitsFromISR(hEventGroup, (EventBits_t)flags) == pdFAIL) {
        rflags = (uint32_t)FuriStatusErrorResource;
    } else {
        /* xEventGroupClearBitsFromISR only registers clear operation in the timer command queue. */
        portYIELD_FROM_ISR(pdTRUE);
    }
} else {
    rflags = xEventGroupClearBits(hEventGroup, (EventBits_t)flags);
}

if(rflags & flags) {
    furi_event_loop_link_notify(&instance->event_loop_link, FuriEventLoopEventOut);
}

xEventGroupClearBitsFromISR() does not clear the bits; it queues the clear for the timer daemon (the comment in the code already says so). But the furi_event_loop_link_notify(..., FuriEventLoopEventOut) sits outside the if/else and fires synchronously from the ISR, before the daemon has applied the clear.

The event loop thread runs at FuriThreadPriorityNormal (16) and the timer daemon at configTIMER_TASK_PRIORITY (2), so the loop reliably wins the race and evaluates the level against stale bits.

For FuriEventLoopEventOut, furi_event_flag_event_loop_get_level() returns:

(furi_event_flag_get(instance) & FURI_EVENT_FLAG_VALID_BITS) != FURI_EVENT_FLAG_VALID_BITS

So with all 24 valid bits set, an ISR-context furi_event_flag_clear(flag, 0x1) gives:

1. ISR notifies EventOut immediately; item goes on the waiting list.
2. Loop wakes, get_level() reads the unmodified bits, 0xFFFFFF != 0xFFFFFF is false, so no callback runs and furi_event_loop_process_level_event() returns Complete. The item leaves the waiting list.
3. vEventGroupClearBitsCallback runs later in the daemon and clears the bits, but nothing re-notifies the loop.

The event is lost. This mirrors the set path in #4336: not a narrow race, but the deterministic outcome given the priority gap.

Reachability

Latent today. Every in-tree caller of furi_event_flag_clear() is task context, so the FURI_IS_IRQ_MODE() branch is unreachable:

• applications/services/bt/bt_service/bt.c (RPC/GAP threads)
• api_lock_relock() in lib/toolbox/api_lock.h, used from applications/debug/unit_tests/tests/rpc/rpc_test.c

Note that EventOut subscriptions on event flags do exist in tree (applications/debug/unit_tests/tests/furi/furi_event_loop_test.c), so only the ISR-context-clear half of the precondition is currently missing.

It's worth fixing because furi_event_flag_clear() is part of the public FAP API (api_symbols.csv), so an app or a future in-firmware ISR can hit it, and #4358 now documents furi_event_flag_set() as ISR-safe, which invites the symmetric expectation of clear.

Suggested fix

Mirror #4358: move the EventOut notify into a pended callback so it only runs after the clear is applied, e.g. xTimerPendFunctionCallFromISR() running xEventGroupClearBits() followed by furi_event_loop_link_notify(..., FuriEventLoopEventOut). As in #4358, the callback should keep the instance alive across the two calls (xEventGroupClearBits() can yield internally via xTaskResumeAll()).

The existing portYIELD_FROM_ISR(pdTRUE) and its ordering rationale (FreeRTOS-Kernel#93) would need re-checking against any such change.

Reproducing

No in-tree reproducer exists, since nothing calls clear() from an ISR. test_furi_event_loop_event_flag_from_isr() (added in #4358) is a close template: it drives the ISR path via a software-pending LPTIM2 interrupt and could be adapted to subscribe FuriEventLoopEventOut and call furi_event_flag_clear() from the ISR.

[munzzyy]

Confirmed against dev. xEventGroupClearBitsFromISR() is just xTimerPendFunctionCallFromISR(vEventGroupClearBitsCallback, ...) (lib/FreeRTOS-Kernel/event_groups.c:509), and configTIMER_TASK_PRIORITY is 2 against FuriThreadPriorityNormal 16, so the loop does win the race.

One thing on the same lines. The pdFAIL branch sets rflags to (uint32_t)FuriStatusErrorResource, which is 0xFFFFFFFD, and then falls into the shared if(rflags & flags) at event_flag.c:98. So a failed clear still notifies EventOut for any mask except 0x2. Moving the notify into the task branch the way you did for set in #4358 would cover that too. Worth folding in?