Skip to content
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

[p2] fixes INTERRUPTS_01_isisr_willpreempt_servicedirqn test #2486

Merged
merged 4 commits into from
Jul 21, 2022
Merged

[p2] fixes INTERRUPTS_01_isisr_willpreempt_servicedirqn test #2486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d430a15
[p2] fixes INTERRUPTS_01_isisr_willpreempt_servicedirqn test
scott-brust Jul 20, 2022
028dcbc
[p2] Only restore handler if one was backed up.
scott-brust Jul 21, 2022
acfb1e1
[p2] simplify error condition handling
scott-brust Jul 21, 2022
8f75cdc
Use SCOPE_GUARD to ensure interrupts are re-enabled on all return con…
scott-brust Jul 21, 2022
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
6 changes: 0 additions & 6 deletions hal/inc/interrupts_hal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,6 @@ int hal_interrupt_set_direct_handler(IRQn_Type irqn, hal_interrupt_direct_handle

#ifdef USE_STDPERIPH_DRIVER

#if defined(STM32F10X_MD) || defined(STM32F10X_HD)
#include "stm32f10x.h"
#elif defined(STM32F2XX)
#include "stm32f2xx.h"
#endif // defined(STM32F10X_MD) || defined(STM32F10X_HD)

#ifdef nRF52840
#include <nrf52840.h>
#endif /* nRF52840 */
Expand Down
30 changes: 22 additions & 8 deletions hal/src/rtl872x/interrupts_hal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ extern "C" {
#include "pinmap_impl.h"
#include "gpio_hal.h"
#include "check.h"
#include "scope_guard.h"

#if HAL_PLATFORM_IO_EXTENSION && MODULE_FUNCTION != MOD_FUNC_BOOTLOADER
#if HAL_PLATFORM_MCP23S17
Expand All @@ -36,6 +37,7 @@ using namespace particle;
#endif

extern uintptr_t link_ram_interrupt_vectors_location[];
static uint32_t hal_interrupts_handler_backup[MAX_VECTOR_TABLE_NUM] = {};

namespace {

Expand Down Expand Up @@ -258,23 +260,35 @@ int hal_interrupt_set_direct_handler(IRQn_Type irqn, hal_interrupt_direct_handle
}

int32_t state = HAL_disable_irq();
volatile uint32_t* isrs = (volatile uint32_t*)&link_ram_interrupt_vectors_location;

SCOPE_GUARD ({
HAL_enable_irq(state);
});

if (handler == nullptr && (flags & HAL_INTERRUPT_DIRECT_FLAG_RESTORE)) {
// Restore
// HAL_Core_Restore_Interrupt(irqn);
// Restore old handler only if one was backed up
uint32_t old_handler = hal_interrupts_handler_backup[IRQN_TO_IDX(irqn)];
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit fragile in the fact that you can call this with handler=nullptr and HAL_INTERRUPT_DIRECT_FLAG_RESTORE when there is no backed-up handler and it will nuke the existing entry in VTOR. Consider a few approaches:

  1. Back up entire VTOR on first use if `!memcmp(hal_interrupts_handler_backup, 0, sizeof(hal_interrupts_handler_backup))
  2. Perhaps use some struct for backup instead and store the state along with backed-up ISR function

Copy link
Member

Choose a reason for hiding this comment

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

Or considering use std::pair(irqn, handler) along with vector().

Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid using vector here due to dynamic allocations. VTOR size is known and negligible, so we can keep the whole thing preallocated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there ever a valid use case to have a NULL interrupt handler? Can I just check if the backed up handler is not null, and only restore it if something has been saved?

if (old_handler) {
__NVIC_SetVector(irqn, (uint32_t)old_handler);
hal_interrupts_handler_backup[IRQN_TO_IDX(irqn)] = 0;
}
} else {
isrs[IRQN_TO_IDX(irqn)] = (uint32_t)handler;
// If there is currently a handler backup: Return error
CHECK_FALSE(hal_interrupts_handler_backup[IRQN_TO_IDX(irqn)], SYSTEM_ERROR_ALREADY_EXISTS);

// If there is a current handler, back it up
uint32_t current_handler = __NVIC_GetVector(irqn);
if (current_handler) {
hal_interrupts_handler_backup[IRQN_TO_IDX(irqn)] = current_handler;
}
}

if (flags & HAL_INTERRUPT_DIRECT_FLAG_DISABLE) {
// Disable
// sd_nvic_DisableIRQ(irqn);
__NVIC_DisableIRQ(irqn);
} else if (flags & HAL_INTERRUPT_DIRECT_FLAG_ENABLE) {
// sd_nvic_EnableIRQ(irqn);
__NVIC_SetVector(irqn, (uint32_t)handler);
}

HAL_enable_irq(state);

return 0;
}
7 changes: 7 additions & 0 deletions user/tests/wiring/no_fixture/interrupts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,11 @@ test(INTERRUPTS_01_isisr_willpreempt_servicedirqn)
assertFalse(hal_interrupt_will_preempt(SysTick_IRQn, SysTick_IRQn));
assertTrue(hal_interrupt_will_preempt(NonMaskableInt_IRQn, SysTick_IRQn));
assertFalse(hal_interrupt_will_preempt(SysTick_IRQn, NonMaskableInt_IRQn));

#if HAL_PLATFORM_RTL872X
// Using arbitrary interrupt
assertTrue(attachInterruptDirect(I2C0_IRQ, []() {return;}));
assertFalse(attachInterruptDirect(I2C0_IRQ, []() {return;}));
assertTrue(detachInterruptDirect(I2C0_IRQ));
#endif
}