From e80fa6de4ccc5c4851eab9fb941f9282d2e3eb16 Mon Sep 17 00:00:00 2001 From: Keith Packard Date: Wed, 24 Oct 2012 22:35:32 -0700 Subject: [PATCH] altos: Replace __critical usage with ao_arch_critical as needed sdcc offers __critical as a machine-independent way to block interrupts, but as gcc doesn't, we need to use a compiler-independent construct instead. ao_arch_critical has been around since the AVR port, but some old __critical usages remained. This fixes a bunch of random hangs when communicating with MM over USB or the radio as the various stdio loops were running without interrupts blocked between the test and the sleep. Signed-off-by: Keith Packard --- src/core/ao.h | 2 +- src/core/ao_ignite.c | 10 +++++---- src/core/ao_mutex.c | 8 +++---- src/core/ao_packet.h | 2 +- src/core/ao_panic.c | 3 ++- src/core/ao_stdio.c | 26 ++++++++++++----------- src/drivers/ao_btm.c | 24 +++++++++++---------- src/drivers/ao_packet.c | 8 ++++++- src/drivers/ao_packet_master.c | 2 +- src/stm/ao_arch.h | 26 ++++++++++++++++++++--- src/stm/ao_timer.c | 8 ++++--- src/stm/ao_usb_stm.c | 38 ++++++++++++++++++++-------------- 12 files changed, 99 insertions(+), 58 deletions(-) diff --git a/src/core/ao.h b/src/core/ao.h index e559e876..2b375cfd 100644 --- a/src/core/ao.h +++ b/src/core/ao.h @@ -101,7 +101,7 @@ ao_delay(uint16_t ticks); /* Set the ADC interval */ void -ao_timer_set_adc_interval(uint8_t interval) __critical; +ao_timer_set_adc_interval(uint8_t interval); /* Timer interrupt */ void diff --git a/src/core/ao_ignite.c b/src/core/ao_ignite.c index c7829fc3..693b7c7a 100644 --- a/src/core/ao_ignite.c +++ b/src/core/ao_ignite.c @@ -21,10 +21,12 @@ __xdata struct ao_ignition ao_ignition[2]; void -ao_ignite(enum ao_igniter igniter) __critical +ao_ignite(enum ao_igniter igniter) { + cli(); ao_ignition[igniter].request = 1; ao_wakeup(&ao_ignition); + sei(); } #ifndef AO_SENSE_DROGUE @@ -39,12 +41,12 @@ ao_igniter_status(enum ao_igniter igniter) __pdata int16_t value; __pdata uint8_t request, firing, fired; - __critical { + ao_arch_critical( ao_data_get(&packet); request = ao_ignition[igniter].request; fired = ao_ignition[igniter].fired; firing = ao_ignition[igniter].firing; - } + ); if (firing || (request && !fired)) return ao_igniter_active; @@ -79,7 +81,7 @@ ao_igniter_status(enum ao_igniter igniter) #endif void -ao_igniter_fire(enum ao_igniter igniter) __critical +ao_igniter_fire(enum ao_igniter igniter) { ao_ignition[igniter].firing = 1; switch(ao_config.ignite_mode) { diff --git a/src/core/ao_mutex.c b/src/core/ao_mutex.c index c82a7d57..952ff462 100644 --- a/src/core/ao_mutex.c +++ b/src/core/ao_mutex.c @@ -22,11 +22,11 @@ ao_mutex_get(__xdata uint8_t *mutex) __reentrant { if (*mutex == ao_cur_task->task_id) ao_panic(AO_PANIC_MUTEX); - __critical { + ao_arch_critical( while (*mutex) ao_sleep(mutex); *mutex = ao_cur_task->task_id; - } + ); } void @@ -34,8 +34,8 @@ ao_mutex_put(__xdata uint8_t *mutex) __reentrant { if (*mutex != ao_cur_task->task_id) ao_panic(AO_PANIC_MUTEX); - __critical { + ao_arch_critical( *mutex = 0; ao_wakeup(mutex); - } + ); } diff --git a/src/core/ao_packet.h b/src/core/ao_packet.h index 9058c347..0eafd3b2 100644 --- a/src/core/ao_packet.h +++ b/src/core/ao_packet.h @@ -63,7 +63,7 @@ void ao_packet_putchar(char c) __reentrant; char -ao_packet_pollchar(void) __critical; +ao_packet_pollchar(void); #if PACKET_HAS_MASTER /* ao_packet_master.c */ diff --git a/src/core/ao_panic.c b/src/core/ao_panic.c index 3c0b471e..c29cd8fe 100644 --- a/src/core/ao_panic.c +++ b/src/core/ao_panic.c @@ -53,7 +53,8 @@ ao_panic(uint8_t reason) ao_cur_task = NULL; printf ("panic %d\n", reason); #endif - __critical for (;;) { + ao_arch_block_interrupts(); + for (;;) { ao_panic_delay(20); for (n = 0; n < 5; n++) { ao_led_on(AO_LED_PANIC); diff --git a/src/core/ao_stdio.c b/src/core/ao_stdio.c index 656b23c9..8cf66a23 100644 --- a/src/core/ao_stdio.c +++ b/src/core/ao_stdio.c @@ -96,21 +96,23 @@ flush(void) __xdata uint8_t ao_stdin_ready; char -getchar(void) __reentrant __critical +getchar(void) __reentrant { char c; - int8_t stdio = ao_cur_stdio; + ao_arch_critical( + int8_t stdio = ao_cur_stdio; - for (;;) { - c = ao_stdios[stdio].pollchar(); - if (c != AO_READ_AGAIN) - break; - if (++stdio == ao_num_stdios) - stdio = 0; - if (stdio == ao_cur_stdio) - ao_sleep(&ao_stdin_ready); - } - ao_cur_stdio = stdio; + for (;;) { + c = ao_stdios[stdio].pollchar(); + if (c != AO_READ_AGAIN) + break; + if (++stdio == ao_num_stdios) + stdio = 0; + if (stdio == ao_cur_stdio) + ao_sleep(&ao_stdin_ready); + } + ao_cur_stdio = stdio; + ); return c; } diff --git a/src/drivers/ao_btm.c b/src/drivers/ao_btm.c index f193ac8e..f3816047 100644 --- a/src/drivers/ao_btm.c +++ b/src/drivers/ao_btm.c @@ -312,18 +312,20 @@ __xdata struct ao_task ao_btm_task; #endif void -ao_btm_check_link() __critical +ao_btm_check_link() { - /* Check the pin and configure the interrupt detector to wait for the - * pin to flip the other way - */ - if (BT_LINK_PIN) { - ao_btm_connected = 0; - PICTL |= BT_PICTL_ICON; - } else { - ao_btm_connected = 1; - PICTL &= ~BT_PICTL_ICON; - } + ao_arch_critical( + /* Check the pin and configure the interrupt detector to wait for the + * pin to flip the other way + */ + if (BT_LINK_PIN) { + ao_btm_connected = 0; + PICTL |= BT_PICTL_ICON; + } else { + ao_btm_connected = 1; + PICTL &= ~BT_PICTL_ICON; + } + ); } void diff --git a/src/drivers/ao_packet.c b/src/drivers/ao_packet.c index 2bada949..3c1e7a18 100644 --- a/src/drivers/ao_packet.c +++ b/src/drivers/ao_packet.c @@ -155,6 +155,9 @@ ao_packet_flush(void) void ao_packet_putchar(char c) __reentrant { + /* No need to block interrupts, all variables here + * are only manipulated in task context + */ while (ao_packet_tx_used == AO_PACKET_MAX && ao_packet_enable) { #if PACKET_HAS_MASTER ao_packet_flush(); @@ -167,8 +170,11 @@ ao_packet_putchar(char c) __reentrant } char -ao_packet_pollchar(void) __critical +ao_packet_pollchar(void) { + /* No need to block interrupts, all variables here + * are only manipulated in task context + */ if (!ao_packet_enable) return AO_READ_AGAIN; diff --git a/src/drivers/ao_packet_master.c b/src/drivers/ao_packet_master.c index e97a6648..481232df 100644 --- a/src/drivers/ao_packet_master.c +++ b/src/drivers/ao_packet_master.c @@ -18,7 +18,7 @@ #include "ao.h" static char -ao_packet_getchar(void) __critical +ao_packet_getchar(void) { char c; while ((c = ao_packet_pollchar()) == AO_READ_AGAIN) { diff --git a/src/stm/ao_arch.h b/src/stm/ao_arch.h index f2de719c..0c3cfc91 100644 --- a/src/stm/ao_arch.h +++ b/src/stm/ao_arch.h @@ -43,7 +43,6 @@ #define __xdata #define __code const #define __reentrant -#define __critical #define __interrupt(n) #define __at(n) @@ -83,8 +82,29 @@ extern const uint32_t ao_radio_cal; #define ao_arch_task_members\ uint32_t *sp; /* saved stack pointer */ -#define cli() asm("cpsid i") -#define sei() asm("cpsie i") +#define ao_arch_block_interrupts() asm("cpsid i") +#define ao_arch_release_interrupts() asm("cpsie i") + +#define cli() ao_arch_block_interrupts() +#define sei() ao_arch_release_interrupts() + +static uint32_t +ao_arch_irqsave(void) { + uint32_t primask; + asm("mrs %0,primask" : "=&r" (primask)); + ao_arch_block_interrupts(); + return primask; +} + +static void +ao_arch_irqrestore(uint32_t primask) { + asm("msr primask,%0" : : "r" (primask)); +} + +static void +ao_arch_memory_barrier() { + asm volatile("" ::: "memory"); +} #define ao_arch_init_stack(task, start) do { \ uint32_t *sp = (uint32_t *) (task->stack + AO_STACK_SIZE); \ diff --git a/src/stm/ao_timer.c b/src/stm/ao_timer.c index f3011d3f..d82a803e 100644 --- a/src/stm/ao_timer.c +++ b/src/stm/ao_timer.c @@ -56,10 +56,12 @@ void stm_tim6_isr(void) #if HAS_ADC void -ao_timer_set_adc_interval(uint8_t interval) __critical +ao_timer_set_adc_interval(uint8_t interval) { - ao_data_interval = interval; - ao_data_count = 0; + ao_arch_critical( + ao_data_interval = interval; + ao_data_count = 0; + ); } #endif diff --git a/src/stm/ao_usb_stm.c b/src/stm/ao_usb_stm.c index 4f37a7d9..8e7dacc5 100644 --- a/src/stm/ao_usb_stm.c +++ b/src/stm/ao_usb_stm.c @@ -799,25 +799,23 @@ ao_usb_in_send(void) ao_usb_tx_count = 0; } -/* Wait for a free IN buffer */ +/* Wait for a free IN buffer. Interrupts are blocked */ static void -ao_usb_in_wait(void) +_ao_usb_in_wait(void) { for (;;) { /* Check if the current buffer is writable */ if (ao_usb_tx_count < AO_USB_IN_SIZE) break; - cli(); /* Wait for an IN buffer to be ready */ while (ao_usb_in_pending) ao_sleep(&ao_usb_in_pending); - sei(); } } void -ao_usb_flush(void) __critical +ao_usb_flush(void) { if (!ao_usb_running) return; @@ -829,24 +827,25 @@ ao_usb_flush(void) __critical * packet was full, in which case we now * want to send an empty packet */ + ao_arch_block_interrupts(); if (!ao_usb_in_flushed) { ao_usb_in_flushed = 1; - cli(); /* Wait for an IN buffer to be ready */ while (ao_usb_in_pending) ao_sleep(&ao_usb_in_pending); - sei(); ao_usb_in_send(); } + ao_arch_release_interrupts(); } void -ao_usb_putchar(char c) __critical __reentrant +ao_usb_putchar(char c) { if (!ao_usb_running) return; - ao_usb_in_wait(); + ao_arch_block_interrupts(); + _ao_usb_in_wait(); ao_usb_in_flushed = 0; ao_usb_tx_buffer[ao_usb_tx_count++] = (uint8_t) c; @@ -854,10 +853,11 @@ ao_usb_putchar(char c) __critical __reentrant /* Send the packet when full */ if (ao_usb_tx_count == AO_USB_IN_SIZE) ao_usb_in_send(); + ao_arch_release_interrupts(); } static void -ao_usb_out_recv(void) +_ao_usb_out_recv(void) { ao_usb_out_avail = 0; @@ -888,7 +888,7 @@ _ao_usb_pollchar(void) /* Check to see if a packet has arrived */ if (!ao_usb_out_avail) return AO_READ_AGAIN; - ao_usb_out_recv(); + _ao_usb_out_recv(); } /* Pull a character out of the fifo */ @@ -900,27 +900,28 @@ char ao_usb_pollchar(void) { char c; - cli(); + ao_arch_block_interrupts(); c = _ao_usb_pollchar(); - sei(); + ao_arch_release_interrupts(); return c; } char -ao_usb_getchar(void) __critical +ao_usb_getchar(void) { char c; - cli(); + ao_arch_block_interrupts(); while ((c = _ao_usb_pollchar()) == AO_READ_AGAIN) ao_sleep(&ao_stdin_ready); - sei(); + ao_arch_release_interrupts(); return c; } void ao_usb_disable(void) { + ao_arch_block_interrupts(); stm_usb.cntr = (1 << STM_USB_CNTR_FRES); stm_usb.istr = 0; @@ -932,6 +933,7 @@ ao_usb_disable(void) /* Disable the interface */ stm_rcc.apb1enr &+ ~(1 << STM_RCC_APB1ENR_USBEN); + ao_arch_release_interrupts(); } void @@ -954,6 +956,8 @@ ao_usb_enable(void) * pulled low and doesn't work at all */ + ao_arch_block_interrupts(); + /* Route interrupts */ stm_nvic_set_priority(STM_ISR_USB_LP_POS, 3); stm_nvic_set_enable(STM_ISR_USB_LP_POS); @@ -985,6 +989,8 @@ ao_usb_enable(void) (0 << STM_USB_CNTR_PDWN) | (0 << STM_USB_CNTR_FRES)); + ao_arch_release_interrupts(); + for (t = 0; t < 1000; t++) ao_arch_nop(); /* Enable USB pull-up */ -- 2.30.2