gblink: fix MALVEKE pinout breaking OK button

Setting an interrupt on PB3 would clobber the interrupt for the OK button (due to them sharing an interrupt) with no way to restore those setting later. Signed-off-by: Kris Bahnsen <[email protected]>
RogueMaster · May 2, 2024 · 421f945 · 421f945
1 parent 01195d6
commit 421f945
Show file tree

Hide file tree

Showing 2 changed files with 235 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -1,7 +1,7 @@
 # Flipper Game Boy Game Link Cable API
 Simple API that can be included in projects to provide a flexible and easy way to handle data exchange over a Game Link Cable.
 
-Current Version: 0.5
+Current Version: 0.6
 
 Available from: https://github.com/kbembedded/flipper-gblink
 
@@ -11,15 +11,16 @@ Available from: https://github.com/kbembedded/flipper-gblink
 - [x] Callback on byte transfer completion  
 - [x] Flexibility in IO pin selection at alloc time  
 - [x] Ability to enable and disable interrupt on input clock  
-- [x] Ability to set timeout in microseconds between clock edges. If exceeded, it is assumed the next clock is the first bit of a byte
+- [x] Ability to set timeout in microseconds between clock edges. If exceeded, it is assumed the next clock is the first bit of a byte  
 - [x] Set a NO\_DATA\_BYTE pattern. i.e. after a byte transfer is complete, a default byte is prepared to be sent out if no new data is provided before the transfer starts  
-- [x] Supports communication to GBC
-- [x] Supports communication to GBA using GBC games
-- [ ] Supports communication to GB (untested, but should work)
-- [ ] Supports communication to GBA using GBA games
+- [x] Supports communication to GBC  
+- [x] Supports communication to GBA using GBC games  
+- [x] Supports older MALVEKE pinouts that would previously cause the Okay button to stop functioning after a trade  
+- [ ] Supports communication to GB (untested, but should work)  
+- [ ] Supports communication to GBA using GBA games  
 - [ ] Function as INT clock source. i.e. Flipper Zero drives the clock line  
-- [ ] Drive clock at varying speeds as GBC supports
-- [ ] Proper documentation
+- [ ] Drive clock at varying speeds as GBC supports  
+- [ ] Proper documentation  
 
 ## Use example
 See https://github.com/EstebanFuentealba/Flipper-Zero-Game-Boy-Pokemon-Trading
@@ -31,7 +32,6 @@ App(
     fap_private_libs=[
         Lib(
             name="flipper-gblink",
-            sources=["gblink.c"],
         ),
     ],
 ...

diff --git a/gblink.c b/gblink.c
@@ -3,6 +3,8 @@
 
 #include <furi.h>
 #include <furi_hal.h>
+#include <stm32wbxx_ll_exti.h>
+#include <stm32wbxx_ll_system.h>
 
 #include <stdint.h>
 
@@ -55,6 +57,12 @@ struct gblink {
 
 	void (*callback)(void* cb_context, uint8_t in);
 	void *cb_context;
+
+	uint32_t* ivt_mirror;
+	uint32_t ivt_mirror_offs;
+	bool exti3_rise_enable;
+	bool exti3_fall_enable;
+	bool exti3_event_enable;
 };
 
 static void gblink_shift_in(struct gblink *gblink)
@@ -100,7 +108,7 @@ static void gblink_shift_out(struct gblink *gblink)
 	gblink->out <<= 1;
 }
 
-static void gblink_clk_callback(void *context)
+static void gblink_clk_isr(void *context)
 {
 	furi_assert(context);
 	struct gblink *gblink = context;
@@ -114,6 +122,189 @@ static void gblink_clk_callback(void *context)
 	}
 }
 
+/* NOTE WELL! This function is absurdly hacky and a stupid workaround to a
+ * stupid issue that doesn't really have any other solution in the current
+ * Flipper/FURI API. I'm over-commenting this so we know exactly what is going
+ * on if we ever have to re-visit this mess.
+ *
+ * This block of text below describes the overall idea, more specific comments
+ * in the function body.
+ *
+ * TODO: make this more generic for any other GPIOs that might conflict with
+ * exti interrupts. PA6, PB3, PC3, PB2? (NFC), PA13, PB6
+ * NOTE: This is only set up at the moment for PB3, hardcoded
+ *
+ * There are multiple problems that this workaround is handling. EXTI interrupts
+ * are shared among multiple pins. The FURI core maintains per-pin ISRs in a
+ * private struct that has no way to read, save, or otherwise be able to put
+ * back the ISR that would service a conflicting EXTI. e.g. PB3 and PH3
+ * (the OK button) both share EXTI3. Setting an interrupt on PB3 will clobber
+ * the FURI ISR callback/context pair as well as change EXTI3 to use PB3 as
+ * the interrupt source.
+ *
+ * To make an interrupt work correctly on PB3 and not break the OK button
+ * we need a way to set an interrupt for PB3 in a way that doesn't clobber the
+ * private FURI GPIO ISR handles and can let the interrupt for the OK button
+ * work again when we're done.
+ *
+ * The general concept of this workaround is to modify the IVT to create our
+ * own handler for EXTI3 interrupts. Doing this leaves the aforementioned private
+ * GPIO struct unmodified and disables the OK button from triggering an interrupt.
+ * The IVT is normally located at the lowest addresses of flash (which is located
+ * at 0x08000000 and mapped at runtime to 0x00000000); this means the IVT cannot
+ * be changed at runtime.
+ *
+ * To make this work, we use the Vector Table Offset Register (VTOR) in the
+ * System Control Block (SCB). The VTOR allows for changing the location of the
+ * IVT. We copy the IVT to a location in memory, and then do a dance to safely
+ * set up the GPIO interrupt to PB3, and swap in our IVT with the modified EXTI3
+ * handler.
+ *
+ * When undoing this, the process is not quite in reverse as we have to put back
+ * specific interrupt settings that we very likely would have clobbered but have
+ * the ability to save beforehand.
+ *
+ * Wrapping the steps in disabling the EXTI3 interrupt is probably not needed,
+ * but is a precaution since we are changing the interrupt sources in weird ways.
+ */
+/* Used to map our callback context in a way the handler can access */
+static void *exti3_cb_context;
+static void gblink_exti3_IRQHandler(void) {
+	if(LL_EXTI_IsActiveFlag_0_31(LL_EXTI_LINE_3)) {
+		gblink_clk_isr(exti3_cb_context);
+		LL_EXTI_ClearFlag_0_31(LL_EXTI_LINE_3);
+	}
+}
+
+static void gblink_gross_exti_workaround(struct gblink *gblink)
+{
+	/* This process makes a number of assumptions, including that the IVT
+	 * is located at 0x00000000, that the lowest flash page is mapped to
+	 * that base address, and that the VTOR points to 0x00000000.
+	 * There are runtime protections in place to prevent reading from the
+	 * first 1 MB of addresses. So we have to always assume that the lowest
+	 * page of flash is mapped to 0x00000000 and read the IVT from the that
+	 * page in flash directly.
+	 * The only check we can really do here is ensuring VTOR is 0 and that
+	 * Main memory is mapped to 0x00000000. If either of those are not true,
+	 * then we can't continue.
+	 */
+	furi_check(SCB->VTOR == 0x0);
+	furi_check(LL_SYSCFG_GetRemapMemory() == LL_SYSCFG_REMAP_FLASH);
+
+	/* Create a mirror of the existing IVT from CPU 1
+	 * The IVT on this platform has 79 entries; 63 maskable, 10 non-maskable,
+	 * 6 reserved. The maskable interrupts start at offset 16.
+	 * CMSIS documentation says that the boundary for IVT must be aligned to
+	 * the number of interrupts, rounded up to the nearest power of two, and
+	 * then multiplied by the word width of the CPU. 79 rounds up to 128
+	 * with a word width of 4, this is 512/0x200 bytes.
+	 * As there is no good way with FreeRTOS to request an alloc at an
+	 * aligned boundary, allocate the amount of data we need, plus 0x200
+	 * bytes, to guarantee that we can put the table in a location that is
+	 * properly aligned. Once we find a suitable base address, this offset
+	 * is saved for later.
+	 */
+	gblink->ivt_mirror = malloc((79 * sizeof(uint32_t)) + 0x200);
+	gblink->ivt_mirror_offs = (uint32_t)gblink->ivt_mirror;
+	while (gblink->ivt_mirror_offs & 0x1FF)
+		gblink->ivt_mirror_offs++;
+	/* 0x08000000 is used instead of 0x00000000 because everything complains
+	 * using a NULL pointer.
+	 */
+	memcpy((uint32_t *)gblink->ivt_mirror_offs, ((uint32_t *)0x08000000), 79 * sizeof(uint32_t));
+
+	/* Point our IVT's EXTI3 interrupt to our desired interrupt handler.
+	 * Also copy the gblink struct to the global var that the interrupt
+	 * handler will use to make further calls.
+	 */
+	((uint32_t *)gblink->ivt_mirror_offs)[25] = (uint32_t)gblink_exti3_IRQHandler; // 16 NMI + offset of 9 for EXTI3
+	exti3_cb_context = gblink;
+
+	/* Disable the EXTI3 interrupt. This lets us do bad things without
+	 * fear of an IRQ hitting in the middle.
+	 */
+	LL_EXTI_DisableIT_0_31(LL_EXTI_LINE_3);
+
+	/* Save the existing rise/fall trigger settings. In theory, these should
+	 * really never change through the life of the flipper OS. But for safety
+	 * we always save them rather than just blindly restoring the same settings
+	 * back when we undo this later.
+	 */
+	gblink->exti3_rise_enable = LL_EXTI_IsEnabledRisingTrig_0_31(LL_EXTI_LINE_3);
+	gblink->exti3_fall_enable = LL_EXTI_IsEnabledFallingTrig_0_31(LL_EXTI_LINE_3);
+	gblink->exti3_event_enable = LL_EXTI_IsEnabledEvent_0_31(LL_EXTI_LINE_3);
+
+	/* Now, set up our desired pin settings. This will only clobber exti3
+	 * settings and will not affect the actual interrupt vector address.
+	 * Settings include the rising/falling/event triggers which we just
+	 * saved.
+	 */
+	furi_hal_gpio_init(gblink->clk, GpioModeInterruptRiseFall, GpioPullUp, GpioSpeedVeryHigh);
+
+	/* Update the NVIC table to point at our desired table.
+	 * Out of safety, stop the world around changing the VTOR reg.
+	 */
+	FURI_CRITICAL_ENTER();
+	SCB->VTOR = gblink->ivt_mirror_offs;
+	FURI_CRITICAL_EXIT();
+
+	/* Last, enable the interrupts and hope everything works. */
+	LL_EXTI_EnableIT_0_31(LL_EXTI_LINE_3);
+}
+
+static void gblink_gross_exti_workaround_undo(struct gblink *gblink)
+{
+	/* First, disable the EXTI3 interrupt. This lets us do bad things without
+	 * fear of an IRQ hitting in the middle.
+	 */
+	LL_EXTI_DisableIT_0_31(LL_EXTI_LINE_3);
+
+	/* Set the correct input source, PH3/OK button, to EXTI3. It is important
+	 * to do this before calling furi_hal_gpio_init() on PB3. When that func
+	 * is called with no interrupt settings enabled, if the EXTI source
+	 * matches the pin, and the interrupt is enabled, interrupts will be
+	 * disabled. By manually setting the EXTI3 source here, it no longer
+	 * matches the PB3 pin, and our changing of IO settings on our GPIO pin
+	 * to no longer have interrupts will not affect the shared IRQ.
+	 */
+	LL_SYSCFG_SetEXTISource(LL_SYSCFG_EXTI_PORTH, LL_SYSCFG_EXTI_LINE3);
+
+	/* Set the correct rise/fall/event settings back */
+	if (gblink->exti3_rise_enable)
+		LL_EXTI_EnableRisingTrig_0_31(LL_EXTI_LINE_3);
+	else
+		LL_EXTI_DisableRisingTrig_0_31(LL_EXTI_LINE_3);
+
+	if (gblink->exti3_fall_enable)
+		LL_EXTI_EnableFallingTrig_0_31(LL_EXTI_LINE_3);
+	else
+		LL_EXTI_DisableFallingTrig_0_31(LL_EXTI_LINE_3);
+
+	if (gblink->exti3_event_enable)
+		LL_EXTI_EnableEvent_0_31(LL_EXTI_LINE_3);
+	else
+		LL_EXTI_DisableEvent_0_31(LL_EXTI_LINE_3);
+
+	/* "Release" the GPIO by putting it back in a known idle state. */
+	furi_hal_gpio_init_simple(gblink->clk, GpioModeAnalog);
+
+	/* Set the IVT back to the normal, in-flash table. Stopping the world
+	 * while we do so.
+	 * NOTE: This just assumes the VTOR is always at 0x0 by default, if this
+	 * ever changes in the Flipper OS, then that will be a problem.
+	 */
+	FURI_CRITICAL_ENTER();
+	SCB->VTOR = 0x0;
+	FURI_CRITICAL_EXIT();
+
+	/* Re-enable the interrupt, OK button should work again. */
+	LL_EXTI_EnableIT_0_31(LL_EXTI_LINE_3);
+
+	/* Free the alloc()ed mirror space */
+	free(gblink->ivt_mirror);
+}
+
 void gblink_clk_source_set(void *handle, int source)
 {
 	furi_assert(handle);
@@ -153,6 +344,9 @@ void gblink_transfer(void *handle, uint8_t val)
 	 * out_buf and set out_buf_valid. When the ISR is finished writing the
 	 * next byte it will check out_buf_valid and copy in out_buf.
 	 *
+	 * The correct/smart way of doing this would be a mutex rather than
+	 * stopping the world.
+	 *
 	 * Realistically, this should only ever be called from the transfer
 	 * complete callback. There are few situations outside of that which
 	 * would make sense.
@@ -218,18 +412,35 @@ void *gblink_alloc(struct gblink_def *gblink_def)
 	gblink->cb_context = gblink_def->cb_context;
 
 	/* Set up pins */
-	/* Currently assumes external clock source only */
+	/* TODO: Set up a list of pins that are not safe to use with interrupts.
+	 * I do believe the main FURI GPIO struct has this data baked in so that
+	 * could be used. For now though, we're only checking for the MALVEKE
+	 * pinout which uses a clk pin that has its IRQ shared with the Okay
+	 * button.
+	 * See the work done in pokemon trade tool custom pinout selection for
+	 * an idea of how to check all that.
+	 */
+	/* TODO: Currently assumes external clock source only */
 	/* XXX: This might actually be open-drain on real GB hardware */
 	furi_hal_gpio_write(gblink->serout, false);
 	furi_hal_gpio_init(gblink->serout, GpioModeOutputPushPull, GpioPullNo, GpioSpeedVeryHigh);
 	furi_hal_gpio_write(gblink->serin, false);
 	furi_hal_gpio_init(gblink->serin, GpioModeInput, GpioPullUp, GpioSpeedVeryHigh);
-	furi_hal_gpio_init(gblink->clk, GpioModeInterruptRiseFall, GpioPullUp, GpioSpeedVeryHigh);
 
 	/* Set up interrupt on clock */
-	/* This may not be needed after NFC refactor */
-	furi_hal_gpio_remove_int_callback(gblink->clk);
-	furi_hal_gpio_add_int_callback(gblink->clk, gblink_clk_callback, gblink);
+	if (gblink->clk == &gpio_ext_pb3) {
+		/* The clock pin is on a pin that is not safe to set an interrupt
+		 * on, so we do a gross workaround to get an interrupt enabled
+		 * on that pin in a way that can be undone safely later with
+		 * no impact to the shared IRQ.
+		 */
+		gblink_gross_exti_workaround(gblink);
+	} else {
+		furi_hal_gpio_init(gblink->clk, GpioModeInterruptRiseFall, GpioPullUp, GpioSpeedVeryHigh);
+		/* This may not be needed after NFC refactor */
+		furi_hal_gpio_remove_int_callback(gblink->clk);
+		furi_hal_gpio_add_int_callback(gblink->clk, gblink_clk_isr, gblink);
+	}
 
 	return gblink;
 }
@@ -239,8 +450,15 @@ void gblink_free(void *handle)
 	furi_assert(handle);
 	struct gblink *gblink = handle;
 
-	/* Remove interrupt, set IO to sane state */
-	furi_hal_gpio_remove_int_callback(gblink->clk);
+	if (gblink->clk == &gpio_ext_pb3) {
+		/* This handles switching the IVT back and putting the EXTI
+		 * regs and pin regs in a valid state for normal use.
+		 */
+		gblink_gross_exti_workaround_undo(gblink);
+	} else {
+		/* Remove interrupt, set IO to sane state */
+		furi_hal_gpio_remove_int_callback(gblink->clk);
+	}
 	furi_hal_gpio_init_simple(gblink->serin, GpioModeAnalog);
 	furi_hal_gpio_init_simple(gblink->serout, GpioModeAnalog);
 	furi_hal_gpio_init_simple(gblink->clk, GpioModeAnalog);