Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gpio: clear irq status before calling user handler #4118

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

mammothbane
Copy link
Contributor

@mammothbane mammothbane commented Feb 18, 2025

This patch clears IRQ status on EXTI handlers before calling the user's handler.

Behavior without this patch was to clear IRQ afterwards (pseudocode):

if (irq_enabled) {
    call_user_handler();
    clear_irq_status();
}

This behavior meant that interrupts which fired during the user handler wouldn't be observed, since the IRQ status was unconditionally cleared afterwards — this throws away information. It risks the following:

IRQ -> USER CODE -> CLEAR IRQ
                ^
            INTERRUPT

The user code will never see or handle this interrupt condition. So if instead we clear the IRQ first, the handler will just be called again in this case.

Verification

Build a ring oscillator circuit: feed an output pin back into an input pin and invert the input level in an interrupt. Observe the feedback line on an oscilloscope. The circuit doesn't oscillate without this patch, but with it, it does.

This application can be used to verify: https://pub.npry.dev/irqfix (built .fap (API 80.1), gzipped so github will accept it). I symlinked into applications_user/ to build. It uses pins PA7 and PA6 as input and output, respectively.

The Toggle button can be used to "kick" the oscillator to start it if it doesn't start running automatically. This is what we see in the "no fix" image -- the toggle button inverts the level, the irq handler runs once, but then it stops, as the IRQ already fired within the handler and was then cleared.

Flipper in circuit
breadboard

No Fix
no_fix

With Fix
with_fix

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Previous behavior was the following for EXTI IRQ handlers:

    if (irq_enabled) {
        call_user_handler();
        clear_irq_status();
    }

This is a problem if the EXTI line changes during the IRQ: presently,
the IRQ status is effectively logically unobserved during the user
handler, as the status is unconditionally cleared afterwards.

You can see this by building a simple ring oscillator circuit: feed an
output pin back into an input pin and invert the input level in an
interrupt. The circuit doesn't oscillate without this patch, as the
IRQ triggers before the status is cleared. With this patch, the circuit
does oscillate.

This change inverts the order of the logic to clear the IRQ status
first.
@skotopes skotopes merged commit 290a6dc into flipperdevices:dev Feb 20, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants