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

Jump to bottom

Avoid freeing memory in detachInterrupt #2356

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Avoid freeing memory in detachInterrupt #2356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 65 additions & 17 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 @@ -81,6 +81,26 @@ class TestHandler
static int count;
};

class DetachingHandler
{
public:
DetachingHandler(pin_t p): pin(p) {}

void handler()
{
cnt++;
detachInterrupt(pin);
}

int count() {
return cnt;
}

private:
pin_t pin;
volatile int cnt = 0;
};

} // namespace

#if !HAL_PLATFORM_NRF52840 // TODO
Expand All @@ -95,7 +115,7 @@ test(INTERRUPTS_02_detached_handler_is_destroyed)
attachSystemInterrupt(SysInterrupt_SysTick, TestHandler()); // Override current handler
assertEqual(TestHandler::count, 1); // Previous handler has been destroyed
detachSystemInterrupt(SysInterrupt_SysTick);
assertEqual(TestHandler::count, 0);
assertEqual(TestHandler::count, 1); // Detaching does not destroy the handler since this would prevent use in an ISR
}

test(INTERRUPTS_03_isisr_willpreempt_servicedirqn)
Expand All @@ -118,6 +138,16 @@ test(INTERRUPTS_03_isisr_willpreempt_servicedirqn)
#endif // !HAL_PLATFORM_NRF52840

#if PLATFORM_ID == PLATFORM_PHOTON_PRODUCTION || PLATFORM_ID == PLATFORM_P1 || PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION
void trigger_falling_interrupt(pin_t pin) {
auto pinmap = HAL_Pin_Map();
static const uint16_t exti_line = pinmap[pin].gpio_pin;

pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
}


test(INTERRUPTS_04_attachInterruptDirect) {
const pin_t pin = D1;
const IRQn_Type irqn = EXTI9_5_IRQn;
Expand All @@ -132,9 +162,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
}, FALLING);

// Trigger
pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
trigger_falling_interrupt(pin);

// attachInterrupt handler should have been called
assertTrue(attachInterruptHandler == true);
Expand All @@ -148,9 +176,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
assertTrue(res);

// Trigger
pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
trigger_falling_interrupt(pin);

// Only a direct handler should have been called
assertTrue(attachInterruptDirectHandler == true);
Expand All @@ -163,9 +189,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
assertTrue(res);

// Trigger
pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
trigger_falling_interrupt(pin);

// attachInterrupt handler should have been called
assertTrue(attachInterruptHandler == true);
Expand All @@ -181,9 +205,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
assertTrue(res);

// Trigger
pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
trigger_falling_interrupt(pin);

// Only a direct handler should have been called
assertTrue(attachInterruptDirectHandler == true);
Expand All @@ -196,9 +218,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
assertTrue(res);

// Trigger
pinMode(pin, INPUT_PULLDOWN);
EXTI_GenerateSWInterrupt(exti_line);
pinMode(pin, INPUT_PULLUP);
trigger_falling_interrupt(pin);

// Not handler should have been called as IRQ should be disabled
assertTrue(attachInterruptHandler == false);
Expand All @@ -210,6 +230,34 @@ test(INTERRUPTS_04_attachInterruptDirect_1) {

detachInterrupt(pin);
}

test(INTERRUPTS_04_detachFromISR) {
// This test validates that calling detachInterrupt in an ISR is safe when attachInterrupt was called with
// an instance and member. This uses the same code path as attachInterrupt with a function pointer,
// validating that as well. It also validates that we can replace the handler with a new one.

const pin_t pin = D1;

DetachingHandler first_handler(pin);
DetachingHandler second_handler(pin);

pinMode(pin, INPUT_PULLUP);
attachInterrupt(pin, &DetachingHandler::handler, &first_handler, FALLING);

// Trigger
trigger_falling_interrupt(pin);

assertEqual(1, first_handler.count());
assertEqual(0, second_handler.count());

attachInterrupt(pin, &DetachingHandler::handler, &second_handler, FALLING);

// Trigger
trigger_falling_interrupt(pin);

assertEqual(1, first_handler.count());
assertEqual(1, second_handler.count());
}
#endif // PLATFORM_ID == PLATFORM_PHOTON_PRODUCTION || PLATFORM_ID == PLATFORM_P1 || PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION

#if PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION
Expand All @@ -220,4 +268,4 @@ test(INTERRUPTS_05_attachInterruptD7) {
assertFalse(res);
assertFalse(tem);
}
#endif
#endif
21 changes: 11 additions & 10 deletions wiring/src/spark_wiring_interrupts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@ static wiring_interrupt_handler_t* handlers[TOTAL_PINS];

wiring_interrupt_handler_t* allocate_handler(uint16_t pin, wiring_interrupt_handler_t& fn)
{
delete handlers[pin];
return handlers[pin] = new wiring_interrupt_handler_t(fn);
if (handlers[pin]) {
handlers[pin]->swap(fn);
} else {
handlers[pin] = new wiring_interrupt_handler_t(fn);
}
return handlers[pin];
}

void call_wiring_interrupt_handler(void* data)
Expand Down Expand Up @@ -105,10 +109,8 @@ bool detachInterrupt(uint16_t pin)
if (SYSTEM_ERROR_NONE != HAL_Interrupts_Detach(pin)) {
return false;
}
if (handlers[pin]) {
delete handlers[pin];
handlers[pin] = nullptr;
}
// NB: We do not `delete handlers[pin]` here since this would cause an error
// when detachInterrupt is called in an ISR.
return true;
}

Expand Down Expand Up @@ -159,10 +161,9 @@ bool attachSystemInterrupt(hal_irq_t irq, wiring_interrupt_handler_t handler)
*/
bool detachSystemInterrupt(hal_irq_t irq)
{
HAL_InterruptCallback prev = {};
const bool ok = HAL_Set_System_Interrupt_Handler(irq, NULL, &prev, NULL);
delete (wiring_interrupt_handler_t*)prev.data;
return ok;
// NB: We do not delete the previous handler here since this would cause an error
// when detachInterrupt is called in an ISR.
return HAL_Set_System_Interrupt_Handler(irq, NULL, NULL, NULL);
}

bool attachInterruptDirect(IRQn_Type irq, HAL_Direct_Interrupt_Handler handler, bool enable)
Expand Down