8000 rp2/rp2_flash.c: Disable IRQs before calling flash_erase/program. by iabdalkader · Pull Request #7683 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

rp2/rp2_flash.c: Disable IRQs before calling flash_erase/program. #7683

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

rp2/rp2_flash.c: Disable IRQs before calling flash_erase/program. #7683

wants to merge 1 commit into from

Conversation

iabdalkader
Copy link
Contributor
@iabdalkader iabdalkader commented Aug 19, 2021 •
edited
Loading

Flash erase/program functions disable the XIP bit. If any code runs from flash at the same time (ex an IRQ or code it calls) it will fail and cause a lockup.

Example using pio example + file write:

import time
from machine import Pin
import rp2


@rp2.asm_pio(set_init=rp2.PIO.OUT_LOW)
def blink_1hz():
    # fmt: off
    # Cycles: 1 + 1 + 6 + 32 * (30 + 1) = 1000
    irq(rel(0))
    set(pins, 1)
    set(x, 31)                  [5]
    label("delay_high")
    nop()                       [29]
    jmp(x_dec, "delay_high")

    # Cycles: 1 + 7 + 32 * (30 + 1) = 1000
    set(pins, 0)
    set(x, 31)                  [6]
    label("delay_low")
    nop()                       [29]
    jmp(x_dec, "delay_low")
    # fmt: on


# Create the StateMachine with the blink_1hz program, outputting on Pin(25).
sm = rp2.StateMachine(0, blink_1hz, freq=4000, set_base=Pin(25))

# Set the IRQ handler to print the millisecond timestamp.
sm.irq(lambda p: print("SM IRQ:", time.ticks_ms()))

# Start the StateMachine.
sm.active(1)

# buffer must be large enough to make sure it triggers erase/program each time.
f = open("test.txt", "w")
buf = b'a' * 16 * 1024  
for i in range(0, 10):
    f.write(buf)
f.close()

Output:

<irq>
True
SM IRQ: 5417
16384
16384

Followed by a lockup.

@dpgeorge
Copy link
Member

Thanks for this. The change will (of course) introduce latency to IRQs (and maybe even some will be missed) when there are writes to the filesystem. But I guess there's no way around this.

(On stm32 it's a bit different, the system will automatically stall any IRQs if flash is busy.)

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Aug 19, 2021 •
edited
Loading

The change will (of course) introduce latency to IRQs (and maybe even some will be missed) when there are writes to the filesystem.

Yes that's true, some DMA IRQs are missed in an audio module I'm working on which causes it to record an incomplete audio stream to file... This doesn't fix it but would it be slightly better if IRQs are restored briefly after flash_range_erase (and maybe even call event hook) and then disabled again before flash_range_program ? i.e:

diff --git a/ports/rp2/rp2_flash.c b/ports/rp2/rp2_flash.c
index b89cb6fd8..204e36dad 100644
--- a/ports/rp2/rp2_flash.c
+++ b/ports/rp2/rp2_flash.c
@@ -95,12 +95,17 @@ STATIC mp_obj_t rp2_flash_writeblocks(size_t n_args, const mp_obj_t *args) {
     mp_buffer_info_t bufinfo;
     mp_get_buffer_raise(args[2], &bufinfo, MP_BUFFER_READ);
     if (n_args == 3) {
+        uint32_t ints = save_and_disable_interrupts();
         flash_range_erase(self->flash_base + offset, bufinfo.len);
+        restore_interrupts(ints);
+        MICROPY_EVENT_POLL_HOOK
         // TODO check return value
     } else {
         offset += mp_obj_get_int(args[3]);
     }
+    uint32_t ints = save_and_disable_interrupts();
     flash_range_program(self->flash_base + offset, bufinfo.buf, bufinfo.len);
+    restore_interrupts(ints);
     // TODO check return value
     return mp_const_none;
 }

Edit, maybe a better way is to exclude specific critical IRQ handlers that run from RAM with __no_inline_not_in_flash_func and:

    uint32_t mask = *((io_rw_32 *) (PPB_BASE + M0PLUS_NVIC_ISER_OFFSET));
    mask &= ~(CRITICAL_IRQ_NUMBERS);
    irq_set_mask_enabled(mask, false);
    ....
    flash_range_erase(...);
    flash_range_program(...);
    ....
    irq_set_mask_enabled(mask, true);

@dpgeorge
Copy link
Member

his doesn't fix it but would it be slightly better if IRQs are restored briefly after flash_range_erase (and maybe even call event hook) and then disabled again before flash_range_program ?

Yes, would be good. A similar thing was done for the esp8266, see b6e5f82

@iabdalkader
Copy link
Contributor Author

Yes, would be good. A similar thing was done for the esp8266, see b6e5f82

Okay updated.

@iabdalkader
rp2/rp2_flash.c: Disable IRQs before calling flash_erase/program.
d27403c 
* Flash erase/program functions disable the XIP bit.
@iabdalkader
Copy link
Contributor Author

Used MICROPY_BEGIN/END_ATOMIC_SECTION instead.

This was referenced Aug 20, 2021
Closed
Closed
@dpgeorge
Copy link
Member

Merged in c82244a

@dpgeorge dpgeorge closed this Aug 29, 2021
@iabdalkader iabdalkader mentioned this pull request Feb 21, 2022
Open
@iabdalkader iabdalkader deleted the rp2_flash_fix branch February 21, 2022 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iabdalkader @dpgeorge
0