|
| 1 | +# Bug Review: Issue #4336 — EventLoop receives callbacks one event late when setting FuriEventFlag from interrupts |
| 2 | + |
| 3 | +**Issue:** [flipperdevices/flipperzero-firmware#4336](https://github.com/flipperdevices/flipperzero-firmware/issues/4336) |
| 4 | +**Reporter:** @sorgloomer |
| 5 | +**Demo:** https://github.com/sorgloomer/flipper_eventflag_demo |
| 6 | +**Status:** Fixed in firmware. The sections below describe the bug, root cause (previous behavior), and the fix. |
| 7 | + |
| 8 | +--- |
| 9 | + |
| 10 | +## Summary |
| 11 | + |
| 12 | +When `furi_event_flag_set()` is called from interrupt context (e.g. GPIO interrupt) and the app uses `furi_event_loop_subscribe_event_flag()`, the event loop callback is woken **one event late**: the first interrupt does not trigger the callback, and each later callback sees the **previous** flag value. This matches a **level-triggered vs deferred-set** race caused by FreeRTOS’s design of `xEventGroupSetBitsFromISR()`. |
| 13 | + |
| 14 | +--- |
| 15 | + |
| 16 | +## Root cause |
| 17 | + |
| 18 | +### 1. FreeRTOS defers the actual bit set from ISR |
| 19 | + |
| 20 | +From `lib/FreeRTOS-Kernel/include/event_groups.h`: |
| 21 | + |
| 22 | +> Setting bits in an event group is not a deterministic operation … FreeRTOS does not allow nondeterministic operations to be performed in interrupts … **Therefore xEventGroupSetBitsFromISR() sends a message to the timer task** to have the set operation performed in the context of the timer task. |
| 23 | +
|
| 24 | +So when you call `xEventGroupSetBitsFromISR()` in an ISR: |
| 25 | + |
| 26 | +- A command is **queued** to the timer daemon. |
| 27 | +- The **bits are not set in the ISR**; they are set when the timer task later processes that command. |
| 28 | + |
| 29 | +### 2. Furi notifies the event loop immediately |
| 30 | + |
| 31 | +Previously in `furi/core/event_flag.c`, `furi_event_flag_set()` used the following ISR path (now fixed): |
| 32 | + |
| 33 | +```c |
| 34 | +uint32_t furi_event_flag_set(FuriEventFlag* instance, uint32_t flags) { |
| 35 | + // ... |
| 36 | + if(FURI_IS_IRQ_MODE()) { |
| 37 | + yield = pdFALSE; |
| 38 | + if(xEventGroupSetBitsFromISR(hEventGroup, (EventBits_t)flags, &yield) == pdFAIL) { |
| 39 | + // ... |
| 40 | + } else { |
| 41 | + rflags = flags; |
| 42 | + portYIELD_FROM_ISR(yield); |
| 43 | + } |
| 44 | + } else { |
| 45 | + rflags = xEventGroupSetBits(hEventGroup, (EventBits_t)flags); |
| 46 | + } |
| 47 | + |
| 48 | + if(rflags & flags) { |
| 49 | + furi_event_loop_link_notify(&instance->event_loop_link, FuriEventLoopEventIn); |
| 50 | + } |
| 51 | + // ... |
| 52 | +} |
| 53 | +``` |
| 54 | +
|
| 55 | +So in ISR we: |
| 56 | +
|
| 57 | +1. Call `xEventGroupSetBitsFromISR()` → only **schedules** the set in the timer task. |
| 58 | +2. Then call `furi_event_loop_link_notify()` → **wakes the event loop task right away**. |
| 59 | +
|
| 60 | +The event loop therefore wakes **before** the timer task has applied the new bits. |
| 61 | +
|
| 62 | +### 3. Event loop reads the flag level too early |
| 63 | +
|
| 64 | +When the loop runs, it processes the waiting item and (for level-triggered) calls `furi_event_flag_event_loop_get_level()` which uses `furi_event_flag_get()` → `xEventGroupGetBits()`. At that moment the bits from the **current** ISR have not been set yet, so: |
| 65 | +
|
| 66 | +- **First interrupt:** Loop wakes, sees no (or old) bits, does not call the callback (or sees previous value). The “current” set is still pending in the timer queue. |
| 67 | +- **Second interrupt:** Another set is queued, loop is woken again. When it runs, the **first** set may have already been applied by the timer task, so the callback sees the **previous** event’s bits. The second set may still be pending. |
| 68 | +
|
| 69 | +Result: callbacks are effectively **one event late** and the first interrupt can appear to be “lost.” |
| 70 | +
|
| 71 | +--- |
| 72 | +
|
| 73 | +## Why the first interrupt often doesn’t wake the loop (or shows no bits) |
| 74 | +
|
| 75 | +- Loop is idle, waiting in `xTaskNotifyWaitIndexed(..., ticks_to_sleep)`. |
| 76 | +- ISR runs: `xEventGroupSetBitsFromISR()` queues the set; `furi_event_loop_link_notify()` wakes the loop. |
| 77 | +- Loop runs, processes the event-flag item, calls `get_level()` → `furi_event_flag_get()`. The timer task may not have run yet, so bits are still 0 (or old). |
| 78 | +- For level-triggered, `furi_event_loop_process_level_event()` only invokes the callback when `get_level()` is true, so the callback may not run at all for that wakeup. |
| 79 | +- So the first interrupt can be “consumed” (loop woke, processed, found no bits) and the user sees no callback until the **next** interrupt, at which point they see the first interrupt’s bits. |
| 80 | +
|
| 81 | +--- |
| 82 | +
|
| 83 | +## Conclusion |
| 84 | +
|
| 85 | +The behavior is a **firmware-level design limitation**: the event loop is notified in the same critical section as the (deferred) `xEventGroupSetBitsFromISR()`, so the loop consistently sees the flag state **before** the current ISR’s set has been applied. This matches the “one event late” and “first interrupt missing” symptoms. |
| 86 | +
|
| 87 | +--- |
| 88 | +
|
| 89 | +## Workarounds (as in the issue thread) |
| 90 | +
|
| 91 | +1. **Use `FuriMessageQueue` for ISR → task** |
| 92 | + Post to a message queue from the ISR and subscribe with `furi_event_loop_subscribe_message_queue()`. Queues have ISR-safe enqueue and don’t rely on the timer daemon for the data to be visible. |
| 93 | +
|
| 94 | +2. **Use a counting semaphore** |
| 95 | + If you only need a wake-up count (no bit pattern), use a semaphore released from ISR and subscribe with `furi_event_loop_subscribe_semaphore()`. |
| 96 | +
|
| 97 | +3. **Avoid clearing flags in the callback in a way that assumes “current” set** |
| 98 | + Doesn’t fix the lag; at best it only avoids losing bits. The fundamental lag remains because the set is deferred. |
| 99 | +
|
| 100 | +--- |
| 101 | +
|
| 102 | +## Firmware fix (implemented) |
| 103 | +
|
| 104 | +**Approach:** Notify the event loop only **after** the bits are set. We use `xTimerPendFunctionCallFromISR()` with a custom callback that runs in the timer daemon: it calls `xEventGroupSetBits()` then `furi_event_loop_link_notify()`. So when the loop runs, the bits are already set. Same timer queue as before; failure when queue is full still returns `FuriFlagErrorResource`. See `furi/core/event_flag.c` (ISR path and `furi_event_flag_set_bits_and_notify_callback`). |
| 105 | +
|
| 106 | +**Other options (not chosen):** Documentation-only; or a separate message queue / buffer (would change API or add a parallel path). |
| 107 | +
|
| 108 | +**Regression test:** `test_furi_event_loop_event_flag_from_isr()` in `applications/debug/unit_tests/tests/furi/furi_event_loop_test.c` runs `furi_event_flag_set()` from a software-pending LPTIM2 ISR and asserts that the event loop callback receives the correct flag value for each of three triggers (bits 1, 2, 4). If the one-event-late bug returns, the second or third assertion would fail (callback would see the previous bit). Run with `unit_tests test_furi` on device. |
| 109 | +
|
| 110 | +--- |
| 111 | +
|
| 112 | +## Possible firmware fixes (for maintainers) — superseded by fix above |
| 113 | +
|
| 114 | +- **Document** that `furi_event_flag_set()` from ISR is deferred by FreeRTOS and can cause one-event lag when used with the event loop; recommend message queue or semaphore for ISR-driven events. |
| 115 | +- **Optionally:** Provide an ISR-safe path that doesn’t rely on the event group for **notification** (e.g. a separate “pending count” or queue of flag values) and have the event loop read from that instead of from the event group immediately after wake. That would require a different contract/link type for “event flag driven from ISR.” |
| 116 | +- **Alternative:** Use a software timer or a task that runs after the timer daemon to set the flag and then notify the event loop, so the set is always visible when the loop runs (adds latency and complexity). |
| 117 | +
|
| 118 | +--- |
| 119 | +
|
| 120 | +## Post-merge: notify the reporter |
| 121 | +
|
| 122 | +After the fix is merged, post a short comment on [the issue](https://github.com/flipperdevices/flipperzero-firmware/issues/4336) so the reporter (@sorgloomer) can retest. Suggested text: |
| 123 | +
|
| 124 | +> Fixed in [PR #xxxx](link-to-pr). The event loop now receives the correct flag value when `furi_event_flag_set()` is called from interrupt context (no one-event delay). Please retest with the latest firmware; the regression test `unit_tests test_furi` includes `test_furi_event_loop_event_flag_from_isr` which covers this path. |
| 125 | +
|
| 126 | +Replace `#xxxx` and `link-to-pr` with the actual PR number and URL once the fix is merged. |
| 127 | +
|
| 128 | +--- |
| 129 | +
|
| 130 | +## References |
| 131 | +
|
| 132 | +- Issue: https://github.com/flipperdevices/flipperzero-firmware/issues/4336 |
| 133 | +- FreeRTOS `event_groups.h` (doc for `xEventGroupSetBitsFromISR`) |
| 134 | +- `furi/core/event_flag.c` — `furi_event_flag_set()`, `furi_event_loop_link_notify()` |
| 135 | +- `furi/core/event_loop.c` — `furi_event_loop_process_waiting_list()`, `furi_event_loop_process_level_event()` |
| 136 | +- `furi/core/event_loop_i.h` — level vs edge handling |
0 commit comments