Merge pull request #98 from MichaelBell/fix-flash-write

Keep display running while writing to flash

Author: Gadgetoid

drivers/st7701/st7701.cpp

@@ -98,11 +98,11 @@ namespace pimoroni {
 static ST7701* st7701_inst;
 
 // This ISR is triggered whenever the timing SM's FIFO is not full
-void __no_inline_not_in_flash_func(timing_isr)() {
+void __isr __no_inline_not_in_flash_func(timing_isr)() {
     st7701_inst->drive_timing();
 }
 
-void __no_inline_not_in_flash_func(ST7701::drive_timing)()
+void __not_in_flash_func(ST7701::drive_timing)()
 {
     while (!pio_sm_is_tx_fifo_full(st_pio, timing_sm)) {
         uint32_t instr;
@@ -153,17 +153,17 @@ void __no_inline_not_in_flash_func(ST7701::drive_timing)()
 }
 
 // This ISR is triggered at the end of each line transferred
-void __no_inline_not_in_flash_func(end_of_line_isr()) {
+void __isr __no_inline_not_in_flash_func(end_of_line_isr)() {
     st7701_inst->handle_end_of_line();
 }
 
-void __no_inline_not_in_flash_func(ST7701::handle_end_of_line())
+void __not_in_flash_func(ST7701::handle_end_of_line)()
 {
     if (st_pio->irq & 0x2) start_frame_xfer();
     else start_line_xfer();
 }
 
-void __no_inline_not_in_flash_func(ST7701::start_line_xfer())
+void __not_in_flash_func(ST7701::start_line_xfer)()
 {
     hw_clear_bits(&st_pio->irq, 0x1);
 
@@ -173,7 +173,7 @@ void __no_inline_not_in_flash_func(ST7701::start_line_xfer())
     else next_line_addr = &framebuffer[width * (display_row >> row_shift)];
 }
 
-void ST7701::start_frame_xfer()
+void __not_in_flash_func(ST7701::start_frame_xfer)()
 {
     hw_clear_bits(&st_pio->irq, 0x2);
 
@@ -385,13 +385,15 @@ void ST7701::start_frame_xfer()
       current = irq_get_exclusive_handler(pio_get_irq_num(st_pio, 1));
       if(current) irq_remove_handler(pio_get_irq_num(st_pio, 1), current);
       irq_set_exclusive_handler(pio_get_irq_num(st_pio, 1), timing_isr);
+      irq_set_priority(pio_get_irq_num(st_pio, 1), 0x40);
       irq_set_enabled(pio_get_irq_num(st_pio, 1), true);
 
       hw_set_bits(&st_pio->inte0, 0x300); // IRQ 0
       // Remove the MicroPython handler if it's set
       current = irq_get_exclusive_handler(pio_get_irq_num(st_pio, 0));
       if(current) irq_remove_handler(pio_get_irq_num(st_pio, 0), current);
       irq_set_exclusive_handler(pio_get_irq_num(st_pio, 0), end_of_line_isr);
+      irq_set_priority(pio_get_irq_num(st_pio, 0), 0x40);
       irq_set_enabled(pio_get_irq_num(st_pio, 0), true);
     }
 
@@ -524,7 +526,7 @@ void ST7701::start_frame_xfer()
       pio_sm_unclaim(st_pio, timing_sm);
     }
 
-    if(pio_sm_is_claimed(st_pio, palette_sm)) {
+    if(palette && pio_sm_is_claimed(st_pio, palette_sm)) {
       pio_sm_set_enabled(st_pio, palette_sm, false);
       pio_sm_clear_fifos(st_pio, palette_sm);
       pio_sm_unclaim(st_pio, palette_sm);

drivers/st7701/st7701.hpp

@@ -53,6 +53,7 @@ namespace pimoroni {
     // Parallel init
     ST7701(uint16_t width, uint16_t height, Rotation rotation, SPIPins control_pins, uint16_t* framebuffer, uint32_t* palette = nullptr,
       uint d0=1, uint hsync=19, uint vsync=20, uint lcd_de = 21, uint lcd_dot_clk = 22);
+    virtual ~ST7701() {}
 
     void init();
     void cleanup() override;

modules/c/presto/presto.cpp

@@ -9,6 +9,7 @@
 #include "hardware/structs/ioqspi.h"
 #include "hardware/structs/qmi.h"
 #include "hardware/structs/xip_ctrl.h"
+#include "hardware/irq.h"
 
 
 using namespace pimoroni;
@@ -57,7 +58,6 @@ typedef struct _Presto_obj_t {
     uint16_t width;
     uint16_t height;
     bool using_palette;
-    volatile bool exit_core1;
 
     // Automatic ambient backlight control
     volatile bool auto_ambient_leds;
@@ -76,6 +76,12 @@ typedef struct _ModPicoGraphics_obj_t {
 // Micropython object to prevent GC freeing it.
 static _Presto_obj_t *presto_obj;
 
+static volatile bool exit_core1;
+
+// ST7701 must be allocated into SRAM (not PSRAM), so reserve a buffer
+// for it here - Presto_make_new will placement new into this buffer.
+__attribute__((section(".uninitialized_data"))) static uint32_t st7701_buffer[sizeof(ST7701) / sizeof(uint32_t)];
+
 #define NUM_LEDS 7
 
 // These must be tweaked together
@@ -94,7 +100,7 @@ static void __no_inline_not_in_flash_func(update_backlight_leds)() {
         { 0, presto_obj->height - SAMPLE_RANGE }
     };
 
-    while (!presto_obj->exit_core1) {
+    while (!exit_core1) {
         if (presto_obj->auto_ambient_leds) {
             for (int i = 0; i < NUM_LEDS; ++i) {
                 uint32_t r = presto_obj->led_values[i].r;
@@ -132,7 +138,7 @@ static void __no_inline_not_in_flash_func(update_backlight_leds)() {
 
         presto_obj->presto->wait_for_vsync();
 
-        if (presto_obj->exit_core1) break;
+        if (exit_core1) break;
 
         // Note this section calls into code that executes from flash
         // It's important this is done during vsync to avoid artifacts,
@@ -161,24 +167,58 @@ static void __no_inline_not_in_flash_func(update_backlight_leds)() {
     multicore_fifo_push_blocking(1);
 }
 
+#define LOCKOUT_MAGIC_START 0x73a8831eu
+#define LOCKOUT_MAGIC_END (~LOCKOUT_MAGIC_START)
+
+// Alternative version of the lockout handler which does not disable interrupts
+// We know that all interrupts running on core1 will not access flash or PSRAM, so this is safe.
+static void __isr __not_in_flash_func(presto_core1_lockout_handler)(void) {
+    multicore_fifo_clear_irq();
+    while (multicore_fifo_rvalid()) {
+        if (sio_hw->fifo_rd == LOCKOUT_MAGIC_START) {
+            sio_hw->fifo_wr = LOCKOUT_MAGIC_START;
+            __sev();
+            while (multicore_fifo_pop_blocking_inline() != LOCKOUT_MAGIC_END) {
+                tight_loop_contents(); // not tight but endless potentially
+            }
+            sio_hw->fifo_wr = LOCKOUT_MAGIC_END;
+            __sev();
+        }
+    }
+}
+
+static bool ever_inited = false;
+
 void presto_core1_entry() {
     // The multicore lockout uses the FIFO, so we use just use sev and volatile flags to signal this core
     multicore_lockout_victim_init();
 
+    // Replace the lockout handler
+    irq_handler_t sdk_handler = irq_get_exclusive_handler(SIO_IRQ_FIFO);
+    irq_remove_handler(SIO_IRQ_FIFO, sdk_handler);
+    irq_set_exclusive_handler(SIO_IRQ_FIFO, presto_core1_lockout_handler);
+
+    ever_inited = true;
+
     presto_obj->presto->init();
 
     multicore_fifo_push_blocking(0); // Todo handle issues here?*/
 
     // Presto is now running the display using interrupts on this core.
     // We can also drive the backlight if requested.
-    while (!presto_obj->exit_core1) {
-        if (!presto_obj->exit_core1) {
+    while (!exit_core1) {
+        if (!exit_core1) {
             update_backlight_leds();
         }
     }
 
     presto_obj->presto->cleanup();
 
+    // Restore the original lockout handler and deinit.
+    irq_remove_handler(SIO_IRQ_FIFO, presto_core1_lockout_handler);
+    irq_set_exclusive_handler(SIO_IRQ_FIFO, sdk_handler);
+    multicore_lockout_victim_deinit();
+
     multicore_fifo_push_blocking(0);
 }
 
@@ -219,14 +259,14 @@ mp_obj_t Presto_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw,
     self->using_palette = args[ARG_palette].u_bool;
 
     presto_debug("m_new_class(ST7701...\n");
-    self->presto = m_new_class(ST7701, self->width, self->height, ROTATE_0,
+    self->presto = new (st7701_buffer) ST7701(self->width, self->height, ROTATE_0,
         SPIPins{spi1, LCD_CS, LCD_CLK, LCD_DAT, PIN_UNUSED, LCD_DC, BACKLIGHT},
         presto_buffer, self->using_palette ? presto_palette : nullptr,
         LCD_D0);
 
     presto_debug("launch core1\n");
     multicore_reset_core1();
-    self->exit_core1 = false;
+    exit_core1 = false;
     self->auto_ambient_leds = false;
 
     WS2812::RGB* buffer = m_new(WS2812::RGB, NUM_LEDS);
@@ -430,20 +470,20 @@ mp_obj_t Presto___del__(mp_obj_t self_in) {
     }
 
     presto_debug("stop core1\n");
-    presto_obj->exit_core1 = true;
+    exit_core1 = true;
     __sev();
 
-    int fifo_code;
+    uint32_t fifo_code;
     do {
-        fifo_code = multicore_fifo_pop_blocking();
+        while (!multicore_fifo_pop_timeout_us(1000, &fifo_code)) __sev();
         if (fifo_code == 1) {
             cleanup_leds();
         }
     } while (fifo_code != 0);
 
     presto_debug("core1 stopped\n");
 
-    m_del_class(ST7701, presto_obj->presto);
+    presto_obj->presto->~ST7701();
     presto_obj->presto = nullptr;
     presto_obj = nullptr;