Skip to content

stm32: Remove preemptive keyboard interrupt via PendSV #15988

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

Merged
merged 2 commits into from
Oct 14, 2024
Merged

stm32: Remove preemptive keyboard interrupt via PendSV #15988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ece950d
stm32/pendsv: Remove preemptive keyboard interrupt via PendSV.
dpgeorge Oct 10, 2024
1223fa9
renesas-ra/pendsv: Remove preemptive keyboard interrupt via PendSV.
dpgeorge Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 1 addition & 52 deletions ports/renesas-ra/pendsv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@
#include "pendsv.h"
#include "irq.h"

// This variable is used to save the exception object between a ctrl-C and the
// PENDSV call that actually raises the exception. It must be non-static
// otherwise gcc-5 optimises it away. It can point to the heap but is not
// traced by GC. This is okay because we only ever set it to
// mp_kbd_exception which is in the root-pointer set.
void *pendsv_object;

#if defined(PENDSV_DISPATCH_NUM_SLOTS)
uint32_t pendsv_dispatch_active;
pendsv_dispatch_t pendsv_dispatch_table[PENDSV_DISPATCH_NUM_SLOTS];
Expand All @@ -51,24 +44,6 @@ void pendsv_init(void) {
NVIC_SetPriority(PendSV_IRQn, IRQ_PRI_PENDSV);
}

// Call this function to raise a pending exception during an interrupt.
// It will first try to raise the exception "softly" by setting the
// mp_pending_exception variable and hoping that the VM will notice it.
// If this function is called a second time (ie with the mp_pending_exception
// variable already set) then it will force the exception by using the hardware
// PENDSV feature. This will wait until all interrupts are finished then raise
// the given exception object using nlr_jump in the context of the top-level
// thread.
void pendsv_kbd_intr(void) {
if (MP_STATE_MAIN_THREAD(mp_pending_exception) == MP_OBJ_NULL) {
mp_sched_keyboard_interrupt();
} else {
MP_STATE_MAIN_THREAD(mp_pending_exception) = MP_OBJ_NULL;
pendsv_object = &MP_STATE_VM(mp_kbd_exception);
SCB->ICSR = SCB_ICSR_PENDSVSET_Msk;
}
}

#if defined(PENDSV_DISPATCH_NUM_SLOTS)
void pendsv_schedule_dispatch(size_t slot, pendsv_dispatch_t f) {
pendsv_dispatch_table[slot] = f;
Expand All @@ -90,10 +65,7 @@ void pendsv_dispatch_handler(void) {
__attribute__((naked)) void PendSV_Handler(void) {
// Handle a PendSV interrupt
//
// For the case of an asynchronous exception, re-jig the
// stack so that when we return from this interrupt handler
// it returns instead to nlr_jump with argument pendsv_object
// note that stack has a different layout if DEBUG is enabled
// Calls any pending functions in pendsv_dispatch_table.
//
// For the case of a thread switch, swap stacks.
//
Expand Down Expand Up @@ -132,27 +104,6 @@ __attribute__((naked)) void PendSV_Handler(void) {
".no_dispatch:\n"
#endif

// Check if there is an active object to throw via nlr_jump
"ldr r1, pendsv_object_ptr\n"
"ldr r0, [r1]\n"
"cmp r0, #0\n"
"beq .no_obj\n"
#if defined(PENDSV_DEBUG)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the comment just above this, about the debug layout has been invalid and now outdated. Glad to see the PENDSV_DEBUG removed, it should be removed from all ports.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now removed from all ports (although I see there's still a few stray -DPENDSV_DEBUG remaining in Makefile's that should be removed).

As for the comments for the stack layout: feel free to open a PR to fix those.

Copy link
Contributor

@iabdalkader iabdalkader Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the comments for the stack layout: feel free to open a PR to fix those.

I think we should remove, it was never correct; the stack layout doesn't change with different builds, the registers are stacked by the hardware so the layout is always the same. I did some extensive testing here:

#315 (comment)

The hardfault reported in #315 was due to this very same issue, but back then (10 years ago) we assumed, incorrectly, that the stack layout changed for debug builds. This was the reason PENDSV_DEBUG was introduced, and it somehow made debugging made work again, for a while at least.

"str r0, [sp, #8]\n" // store to r0 on stack
#else
"str r0, [sp, #0]\n" // store to r0 on stack
#endif
"mov r0, #0\n"
"str r0, [r1]\n" // clear pendsv_object
"ldr r0, nlr_jump_ptr\n"
#if defined(PENDSV_DEBUG)
"str r0, [sp, #32]\n" // store to pc on stack
#else
"str r0, [sp, #24]\n" // store to pc on stack
#endif
"bx lr\n" // return from interrupt; will return to nlr_jump
".no_obj:\n" // pendsv_object==NULL

#if MICROPY_PY_THREAD
// Do a thread context switch
"push {r4-r11, lr}\n"
Expand All @@ -178,7 +129,5 @@ __attribute__((naked)) void PendSV_Handler(void) {
#if defined(PENDSV_DISPATCH_NUM_SLOTS)
"pendsv_dispatch_active_ptr: .word pendsv_dispatch_active\n"
#endif
"pendsv_object_ptr: .word pendsv_object\n"
"nlr_jump_ptr: .word nlr_jump\n"
);
}
1 change: 0 additions & 1 deletion ports/renesas-ra/pendsv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ enum {
typedef void (*pendsv_dispatch_t)(void);

void pendsv_init(void);
void pendsv_kbd_intr(void);
void pendsv_schedule_dispatch(size_t slot, pendsv_dispatch_t f);

#endif // MICROPY_INCLUDED_RENESAS_RA_PENDSV_H
2 changes: 1 addition & 1 deletion ports/renesas-ra/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static KEYEX_CB keyex_cb[MICROPY_HW_MAX_UART] = {(KEYEX_CB)NULL};

static int chk_kbd_interrupt(int d) {
if (d == mp_interrupt_char) {
pendsv_kbd_intr();
mp_sched_keyboard_interrupt();
return 1;
} else {
return 0;
Expand Down
2 changes: 1 addition & 1 deletion ports/stm32/boards/common_isr_ram/common_isr.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
flash (in main.c) along with the isr_vector above.
*/
. = ALIGN(4);
*(.text.pendsv_kbd_intr)
*(.text.mp_sched_keyboard_interrupt)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewleech it's curious that you only needed to put pendsv_kbd_intr in RAM. That function calls mp_sched_keyboard_interrupt so I wonder why you didn't need to put the latter in RAM as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that is interesting... maybe neither of them actually need to be in ram... or more likely my testing didn't catch the scenario where ctrl-c was being handled by the UART isr at the exact moment flash was locked. Just before or just after it wouldn't matter.
I think I added that function to the ram list based just on review of things called from the isr function but forgot to follow it down the call stack to subsequent functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for confirming. I'll leave this as it is in this PR because it makes sense to have this function in RAM.

*(.text.pendsv_schedule_dispatch)
*(.text.storage_systick_callback)
*(.text.SysTick_Handler)
Expand Down
53 changes: 1 addition & 52 deletions ports/stm32/pendsv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@
#include "pendsv.h"
#include "irq.h"

// This variable is used to save the exception object between a ctrl-C and the
// PENDSV call that actually raises the exception. It must be non-static
// otherwise gcc-5 optimises it away. It can point to the heap but is not
// traced by GC. This is okay because we only ever set it to
// mp_kbd_exception which is in the root-pointer set.
void *pendsv_object;

#if defined(PENDSV_DISPATCH_NUM_SLOTS)
uint32_t pendsv_dispatch_active;
pendsv_dispatch_t pendsv_dispatch_table[PENDSV_DISPATCH_NUM_SLOTS];
Expand All @@ -51,24 +44,6 @@ void pendsv_init(void) {
NVIC_SetPriority(PendSV_IRQn, IRQ_PRI_PENDSV);
}

// Call this function to raise a pending exception during an interrupt.
// It will first try to raise the exception "softly" by setting the
// mp_pending_exception variable and hoping that the VM will notice it.
// If this function is called a second time (ie with the mp_pending_exception
// variable already set) then it will force the exception by using the hardware
// PENDSV feature. This will wait until all interrupts are finished then raise
// the given exception object using nlr_jump in the context of the top-level
// thread.
void pendsv_kbd_intr(void) {
if (MP_STATE_MAIN_THREAD(mp_pending_exception) == MP_OBJ_NULL) {
mp_sched_keyboard_interrupt();
} else {
MP_STATE_MAIN_THREAD(mp_pending_exception) = MP_OBJ_NULL;
pendsv_object = &MP_STATE_VM(mp_kbd_exception);
SCB->ICSR = SCB_ICSR_PENDSVSET_Msk;
}
}

#if defined(PENDSV_DISPATCH_NUM_SLOTS)
void pendsv_schedule_dispatch(size_t slot, pendsv_dispatch_t f) {
pendsv_dispatch_table[slot] = f;
Expand All @@ -90,10 +65,7 @@ void pendsv_dispatch_handler(void) {
__attribute__((naked)) void PendSV_Handler(void) {
// Handle a PendSV interrupt
//
// For the case of an asynchronous exception, re-jig the
// stack so that when we return from this interrupt handler
// it returns instead to nlr_jump with argument pendsv_object
// note that stack has a different layout if DEBUG is enabled
// Calls any pending functions in pendsv_dispatch_table.
//
// For the case of a thread switch, swap stacks.
//
Expand Down Expand Up @@ -132,27 +104,6 @@ __attribute__((naked)) void PendSV_Handler(void) {
".no_dispatch:\n"
#endif

// Check if there is an active object to throw via nlr_jump
"ldr r1, pendsv_object_ptr\n"
"ldr r0, [r1]\n"
"cmp r0, #0\n"
"beq .no_obj\n"
#if defined(PENDSV_DEBUG)
"str r0, [sp, #8]\n" // store to r0 on stack
#else
"str r0, [sp, #0]\n" // store to r0 on stack
#endif
"mov r0, #0\n"
"str r0, [r1]\n" // clear pendsv_object
"ldr r0, nlr_jump_ptr\n"
#if defined(PENDSV_DEBUG)
"str r0, [sp, #32]\n" // store to pc on stack
#else
"str r0, [sp, #24]\n" // store to pc on stack
#endif
"bx lr\n" // return from interrupt; will return to nlr_jump
".no_obj:\n" // pendsv_object==NULL

#if MICROPY_PY_THREAD
// Do a thread context switch
"push {r4-r11, lr}\n"
Expand All @@ -178,7 +129,5 @@ __attribute__((naked)) void PendSV_Handler(void) {
#if defined(PENDSV_DISPATCH_NUM_SLOTS)
"pendsv_dispatch_active_ptr: .word pendsv_dispatch_active\n"
#endif
"pendsv_object_ptr: .word pendsv_object\n"
"nlr_jump_ptr: .word nlr_jump\n"
);
}
1 change: 0 additions & 1 deletion ports/stm32/pendsv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ enum {
typedef void (*pendsv_dispatch_t)(void);

void pendsv_init(void);
void pendsv_kbd_intr(void);
void pendsv_schedule_dispatch(size_t slot, pendsv_dispatch_t f);

#endif // MICROPY_INCLUDED_STM32_PENDSV_H
2 changes: 1 addition & 1 deletion ports/stm32/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,7 @@ void uart_irq_handler(mp_uint_t uart_id) {
data &= self->char_mask;
if (self->attached_to_repl && data == mp_interrupt_char) {
// Handle interrupt coming in on a UART REPL
pendsv_kbd_intr();
mp_sched_keyboard_interrupt();
} else {
if (self->char_width == CHAR_WIDTH_9BIT) {
((uint16_t *)self->read_buf)[self->read_buf_head] = data;
Expand Down
2 changes: 1 addition & 1 deletion ports/stm32/usbd_cdc_interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ int8_t usbd_cdc_receive(usbd_cdc_state_t *cdc_in, size_t len) {
// copy the incoming data into the circular buffer
for (const uint8_t *src = cdc->rx_packet_buf, *top = cdc->rx_packet_buf + len; src < top; ++src) {
if (cdc->attached_to_repl && *src == mp_interrupt_char) {
pendsv_kbd_intr();
mp_sched_keyboard_interrupt();
} else {
uint16_t next_put = (cdc->rx_buf_put + 1) & (MICROPY_HW_USB_CDC_RX_DATA_SIZE - 1);
if (next_put == cdc->rx_buf_get) {
Expand Down
Loading