flipperdevices / flipperzero-firmware

Flipper Zero firmware source code
https://flipperzero.one
GNU General Public License v3.0
12.15k stars 2.65k forks source link

PB2 claimed by nfc_isr callback at FAP start #2690

Closed kbembedded closed 1 year ago

kbembedded commented 1 year ago

Describe the bug.

Some changes in 2a26680acbfc2c526b48bac57b65132a0a1a94bc exposed a situation where some FAPs fail if they attempt use PB2 as an interrupt.

Specifically:

firmware/targets/f7/furi_hal/furi_hal_gpio.c

@@ -178,7 +178,7 @@ void furi_hal_gpio_add_int_callback(const GpioPin* gpio, GpioExtiCallback cb, vo

     FURI_CRITICAL_ENTER();
     uint8_t pin_num = furi_hal_gpio_get_pin_num(gpio);
-    furi_assert(gpio_interrupt[pin_num].callback == NULL);
+    furi_check(gpio_interrupt[pin_num].callback == NULL);

Went from an assert to a runtime check, causing FAPs that call furi_hal_gpio_add_int_callback() against PB2 to fail the furi_check now.

The code that adds this callback was modified in the aforementioned commit, but not significantly changed as far as I can tell:

lib/ST25RFAL002/platform.c

+        furi_hal_gpio_add_int_callback(&gpio_nfc_irq_rfid_pull, nfc_isr, NULL);
+        // Disable interrupt callback as the pin is shared between 2 apps
+        // It is enabled in rfalLowPowerModeStop()
+        furi_hal_gpio_disable_int_callback(&gpio_nfc_irq_rfid_pull);

So currently, at least as of Release 0.83.1, any FAPs that are started already have PB2 claimed as an int callback to nfc_isr() but disabled. If any FAP wants to use this it first needs to call fure_hal_gpio_remove_int_callback.

Reproduction

Easiest way I know is to grab the latest pokemon binary from flipC.org and run it: https://flipc.org/EstebanFuentealba/Flipper-Zero-Game-Boy-Pokemon-Trading?branch=main

At time of writing, I have a draft PR in that repo that works around the issue, but I do not think this is a proper fix: https://github.com/EstebanFuentealba/Flipper-Zero-Game-Boy-Pokemon-Trading/pull/11 https://github.com/EstebanFuentealba/Flipper-Zero-Game-Boy-Pokemon-Trading/pull/11/commits/b4cee73a575b40f03e0f53a1fdc4462c9488f274

Target

No response

Logs

No response

Anything else?

Is this expected behavior? Because it is quite surprising to have an application need to first clear a callback before it can set one up. It should certainly need to clean up after itself when done.

I'm unsure if that int callback should be set up. If the furi_check should instead check if the .ready member is valid (set to false by furi_hal_gpio_disable_int_callback() etc.

If this is not considered an issue, then are FAPs responsible for saving any existing int callbacks and restoring them upon exit?

skotopes commented 1 year ago

https://miro.com/app/board/uXjVO_LaYYI=/?moveToWidget=3458764544420592132&cot=14

This EXTI line is explicitly marked as shared with NFC and unusable for other needs. Will be changed after NFC refactoring.