From 4f1f3e836393304434130d362771a39f6f8f859a Mon Sep 17 00:00:00 2001 From: Keith Packard Date: Sun, 24 Mar 2013 15:00:20 -0700 Subject: [PATCH] altos: Do not release interrupts from any pollchar function getchar relies on interrupts being blocked across the pollchar calls and into the sleep call or it may go to sleep with data pending. This prefixes all pollchar functions with _ to indicate that they are to be called with interrupts blocked and eliminates all interrupt manipulation calls from within the pollchar functions. Signed-off-by: Keith Packard --- src/avr/ao_serial_avr.c | 43 ++++++++++++++-------------- src/avr/ao_usb_avr.c | 42 +++++++++++---------------- src/cc1111/ao_serial.c | 8 +++--- src/cc1111/ao_usb.c | 14 +++++---- src/core/ao.h | 2 +- src/core/ao_packet.h | 2 +- src/core/ao_serial.h | 8 +++--- src/core/ao_stdio.c | 31 ++++++++++---------- src/drivers/ao_btm.c | 52 +++++++++++++++++++++------------- src/drivers/ao_packet.c | 6 ++-- src/drivers/ao_packet_master.c | 7 ++++- src/drivers/ao_packet_slave.c | 2 +- src/stm/ao_serial_stm.c | 45 ++++++++++++++--------------- src/stm/ao_usb_stm.c | 23 ++++++--------- 14 files changed, 144 insertions(+), 141 deletions(-) diff --git a/src/avr/ao_serial_avr.c b/src/avr/ao_serial_avr.c index dcee246c..e0f813d5 100644 --- a/src/avr/ao_serial_avr.c +++ b/src/avr/ao_serial_avr.c @@ -59,52 +59,51 @@ ISR(USART1_UDRE_vect) ao_wakeup(&ao_serial1_tx_fifo); } -char -ao_serial1_getchar(void) __critical -{ - char c; - cli(); - while (ao_fifo_empty(ao_serial1_rx_fifo)) - ao_sleep(&ao_serial1_rx_fifo); - ao_fifo_remove(ao_serial1_rx_fifo, c); - sei(); - return c; -} - #if USE_SERIAL_1_STDIN -char -ao_serial1_pollchar(void) __critical +int +_ao_serial1_pollchar(void) { char c; - cli(); if (ao_fifo_empty(ao_serial1_rx_fifo)) { sei(); return AO_READ_AGAIN; } ao_fifo_remove(ao_serial1_rx_fifo,c); - sei(); return c; } #endif +char +ao_serial1_getchar(void) __critical +{ + char c; + + ao_arch_block_interrupts(); + while (ao_fifo_empty(ao_serial1_rx_fifo)) + ao_sleep(&ao_serial1_rx_fifo); + ao_fifo_remove(ao_serial1_rx_fifo, c); + ao_arch_release_interrupts(); + return c; +} + void -ao_serial1_putchar(char c) __critical +ao_serial1_putchar(char c) { - cli(); + ao_arch_block_interrupts(); while (ao_fifo_full(ao_serial1_tx_fifo)) ao_sleep(&ao_serial1_tx_fifo); ao_fifo_insert(ao_serial1_tx_fifo, c); ao_serial_tx1_start(); - sei(); + ao_arch_release_interrupts(); } void ao_serial1_drain(void) __critical { - cli(); + ao_arch_block_interrupts(); while (!ao_fifo_empty(ao_serial1_tx_fifo)) ao_sleep(&ao_serial1_tx_fifo); - sei(); + ao_arch_release_interrupts(); } static const struct { @@ -155,7 +154,7 @@ ao_serial_init(void) (1 << RXCIE1) | /* Enable receive interrupts */ (1 << UDRIE1)); /* Enable transmit empty interrupts */ #if USE_SERIAL_1_STDIN - ao_add_stdio(ao_serial1_pollchar, + ao_add_stdio(_ao_serial1_pollchar, ao_serial1_putchar, NULL); #endif diff --git a/src/avr/ao_usb_avr.c b/src/avr/ao_usb_avr.c index 2ef546c9..bd75b17d 100644 --- a/src/avr/ao_usb_avr.c +++ b/src/avr/ao_usb_avr.c @@ -411,7 +411,7 @@ ao_usb_ep0(void) /* Wait for a free IN buffer */ static void -ao_usb_in_wait(void) +_ao_usb_in_wait(void) { for (;;) { /* Check if the current buffer is writable */ @@ -419,7 +419,6 @@ ao_usb_in_wait(void) if (UEINTX & (1 << RWAL)) break; - cli(); /* Wait for an IN buffer to be ready */ for (;;) { UENUM = AO_USB_IN_EP; @@ -430,24 +429,24 @@ ao_usb_in_wait(void) } /* Ack the interrupt */ UEINTX &= ~(1 << TXINI); - sei(); } } /* Queue the current IN buffer for transmission */ static void -ao_usb_in_send(void) +_ao_usb_in_send(void) { UENUM = AO_USB_IN_EP; UEINTX &= ~(1 << FIFOCON); } void -ao_usb_flush(void) __critical +ao_usb_flush(void) { if (!ao_usb_running) return; + ao_arch_block_interrupts(); /* Anytime we've sent a character since * the last time we flushed, we'll need * to send a packet -- the only other time @@ -457,18 +456,20 @@ ao_usb_flush(void) __critical */ if (!ao_usb_in_flushed) { ao_usb_in_flushed = 1; - ao_usb_in_wait(); - ao_usb_in_send(); + _ao_usb_in_wait(); + _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(); /* Queue a byte */ UENUM = AO_USB_IN_EP; @@ -476,11 +477,12 @@ ao_usb_putchar(char c) __critical __reentrant /* Send the packet when full */ if ((UEINTX & (1 << RWAL)) == 0) - ao_usb_in_send(); + _ao_usb_in_send(); ao_usb_in_flushed = 0; + ao_arch_release_interrupts(); } -static int +int _ao_usb_pollchar(void) { uint8_t c; @@ -517,25 +519,15 @@ _ao_usb_pollchar(void) return c; } -int -ao_usb_pollchar(void) -{ - int c; - cli(); - c = _ao_usb_pollchar(); - sei(); - return c; -} - char -ao_usb_getchar(void) __critical +ao_usb_getchar(void) { int 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; } @@ -668,5 +660,5 @@ ao_usb_init(void) #if USB_DEBUG ao_add_task(&ao_usb_echo_task, ao_usb_echo, "usb echo"); #endif - ao_add_stdio(ao_usb_pollchar, ao_usb_putchar, ao_usb_flush); + ao_add_stdio(_ao_usb_pollchar, ao_usb_putchar, ao_usb_flush); } diff --git a/src/cc1111/ao_serial.c b/src/cc1111/ao_serial.c index 8913a9b0..81727836 100644 --- a/src/cc1111/ao_serial.c +++ b/src/cc1111/ao_serial.c @@ -92,7 +92,7 @@ ao_serial0_getchar(void) __critical #if USE_SERIAL_0_STDIN int -ao_serial0_pollchar(void) __critical +_ao_serial0_pollchar(void) { uint8_t c; if (ao_fifo_empty(ao_serial0_rx_fifo)) @@ -180,7 +180,7 @@ ao_serial1_getchar(void) __critical #if USE_SERIAL_1_STDIN int -ao_serial1_pollchar(void) __critical +_ao_serial1_pollchar(void) { uint8_t c; if (ao_fifo_empty(ao_serial1_rx_fifo)) @@ -271,7 +271,7 @@ ao_serial_init(void) IEN0 |= IEN0_URX0IE; IEN2 |= IEN2_UTX0IE; #if USE_SERIAL_0_STDIN && !DELAY_SERIAL_0_STDIN - ao_add_stdio(ao_serial0_pollchar, + ao_add_stdio(_ao_serial0_pollchar, ao_serial0_putchar, NULL); #endif @@ -327,7 +327,7 @@ ao_serial_init(void) IEN2 |= IEN2_UTX1IE; #if USE_SERIAL_1_STDIN && !DELAY_SERIAL_1_STDIN - ao_add_stdio(ao_serial1_pollchar, + ao_add_stdio(_ao_serial1_pollchar, ao_serial1_putchar, NULL); #endif diff --git a/src/cc1111/ao_usb.c b/src/cc1111/ao_usb.c index f66e807c..8bd2efdf 100644 --- a/src/cc1111/ao_usb.c +++ b/src/cc1111/ao_usb.c @@ -383,18 +383,18 @@ ao_usb_putchar(char c) __critical __reentrant } int -ao_usb_pollchar(void) __critical +_ao_usb_pollchar(void) { uint8_t c; if (ao_usb_out_bytes == 0) { USBINDEX = AO_USB_OUT_EP; if ((USBCSOL & USBCSOL_OUTPKT_RDY) == 0) - return -1; + return AO_READ_AGAIN; ao_usb_out_bytes = (USBCNTH << 8) | USBCNTL; if (ao_usb_out_bytes == 0) { USBINDEX = AO_USB_OUT_EP; USBCSOL &= ~USBCSOL_OUTPKT_RDY; - return -1; + return AO_READ_AGAIN; } } --ao_usb_out_bytes; @@ -407,12 +407,14 @@ ao_usb_pollchar(void) __critical } char -ao_usb_getchar(void) __critical +ao_usb_getchar(void) { int c; - while ((c = ao_usb_pollchar()) == AO_READ_AGAIN) + ao_arch_block_interrupts(); + while ((c = _ao_usb_pollchar()) == AO_READ_AGAIN) ao_sleep(&ao_stdin_ready); + ao_arch_release_interrupts(); return c; } @@ -459,5 +461,5 @@ ao_usb_init(void) ao_usb_enable(); ao_add_task(&ao_usb_task, ao_usb_ep0, "usb"); - ao_add_stdio(ao_usb_pollchar, ao_usb_putchar, ao_usb_flush); + ao_add_stdio(_ao_usb_pollchar, ao_usb_putchar, ao_usb_flush); } diff --git a/src/core/ao.h b/src/core/ao.h index e3161b4c..6c790f69 100644 --- a/src/core/ao.h +++ b/src/core/ao.h @@ -638,7 +638,7 @@ ao_monitor_init(void) __reentrant; #define AO_READ_AGAIN (-1) struct ao_stdio { - int (*pollchar)(void); + int (*_pollchar)(void); /* Called with interrupts blocked */ void (*putchar)(char c) __reentrant; void (*flush)(void); uint8_t echo; diff --git a/src/core/ao_packet.h b/src/core/ao_packet.h index 08b184d6..6d121bb9 100644 --- a/src/core/ao_packet.h +++ b/src/core/ao_packet.h @@ -63,7 +63,7 @@ void ao_packet_putchar(char c) __reentrant; int -ao_packet_pollchar(void); +_ao_packet_pollchar(void); #if PACKET_HAS_MASTER /* ao_packet_master.c */ diff --git a/src/core/ao_serial.h b/src/core/ao_serial.h index a799bf2c..baf213c0 100644 --- a/src/core/ao_serial.h +++ b/src/core/ao_serial.h @@ -32,7 +32,7 @@ char ao_serial0_getchar(void); int -ao_serial0_pollchar(void); +_ao_serial0_pollchar(void); void ao_serial0_putchar(char c); @@ -52,7 +52,7 @@ char ao_serial1_getchar(void); int -ao_serial1_pollchar(void); +_ao_serial1_pollchar(void); void ao_serial1_putchar(char c); @@ -72,7 +72,7 @@ char ao_serial2_getchar(void); int -ao_serial2_pollchar(void); +_ao_serial2_pollchar(void); void ao_serial2_putchar(char c); @@ -92,7 +92,7 @@ char ao_serial3_getchar(void); int -ao_serial3_pollchar(void); +_ao_serial3_pollchar(void); void ao_serial3_putchar(char c); diff --git a/src/core/ao_stdio.c b/src/core/ao_stdio.c index 1748dfe8..977d74b1 100644 --- a/src/core/ao_stdio.c +++ b/src/core/ao_stdio.c @@ -99,20 +99,21 @@ char getchar(void) __reentrant { int c; - ao_arch_critical( - int8_t stdio = ao_cur_stdio; + int8_t 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; - ); + ao_arch_block_interrupts(); + 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; + ao_arch_release_interrupts(); return c; } @@ -123,13 +124,13 @@ ao_echo(void) } int8_t -ao_add_stdio(int (*pollchar)(void), +ao_add_stdio(int (*_pollchar)(void), void (*putchar)(char), void (*flush)(void)) __reentrant { if (ao_num_stdios == AO_NUM_STDIOS) ao_panic(AO_PANIC_STDIO); - ao_stdios[ao_num_stdios].pollchar = pollchar; + ao_stdios[ao_num_stdios]._pollchar = _pollchar; ao_stdios[ao_num_stdios].putchar = putchar; ao_stdios[ao_num_stdios].flush = flush; ao_stdios[ao_num_stdios].echo = 1; diff --git a/src/drivers/ao_btm.c b/src/drivers/ao_btm.c index c862200a..de1f31a3 100644 --- a/src/drivers/ao_btm.c +++ b/src/drivers/ao_btm.c @@ -19,9 +19,10 @@ #ifndef ao_serial_btm_getchar #define ao_serial_btm_putchar ao_serial1_putchar -#define ao_serial_btm_pollchar ao_serial1_pollchar +#define _ao_serial_btm_pollchar _ao_serial1_pollchar #define ao_serial_btm_set_speed ao_serial1_set_speed #define ao_serial_btm_drain ao_serial1_drain +#define ao_serial_btm_rx_fifo ao_serial1_rx_fifo #endif int8_t ao_btm_stdio; @@ -111,6 +112,30 @@ __code struct ao_cmds ao_btm_cmds[] = { #define AO_BTM_MAX_REPLY 16 __xdata char ao_btm_reply[AO_BTM_MAX_REPLY]; +/* + * Read one bluetooth character. + * Returns AO_READ_AGAIN if no character arrives within 10ms + */ + +static int +ao_btm_getchar(void) +{ + int c; + + ao_arch_block_interrupts(); + while ((c = _ao_serial_btm_pollchar()) == AO_READ_AGAIN) { + ao_alarm(AO_MS_TO_TICKS(10)); + c = ao_sleep(&ao_serial_btm_rx_fifo); + ao_clear_alarm(); + if (c) { + c = AO_READ_AGAIN; + break; + } + } + ao_arch_release_interrupts(); + return c; +} + /* * Read a line of data from the serial port, truncating * it after a few characters. @@ -122,24 +147,13 @@ ao_btm_get_line(void) uint8_t ao_btm_reply_len = 0; int c; - for (;;) { - - while ((c = ao_serial_btm_pollchar()) != AO_READ_AGAIN) { - ao_btm_log_in_char(c); - if (ao_btm_reply_len < sizeof (ao_btm_reply)) - ao_btm_reply[ao_btm_reply_len++] = c; - if (c == '\r' || c == '\n') - goto done; - } - for (c = 0; c < 10; c++) { - ao_delay(AO_MS_TO_TICKS(10)); - if (!ao_fifo_empty(ao_serial1_rx_fifo)) - break; - } - if (c == 10) - goto done; + while ((c = ao_btm_getchar()) != AO_READ_AGAIN) { + ao_btm_log_in_char(c); + if (ao_btm_reply_len < sizeof (ao_btm_reply)) + ao_btm_reply[ao_btm_reply_len++] = c; + if (c == '\r' || c == '\n') + break; } -done: for (c = ao_btm_reply_len; c < sizeof (ao_btm_reply);) ao_btm_reply[c++] = '\0'; return ao_btm_reply_len; @@ -279,7 +293,7 @@ ao_btm(void) /* Turn off status reporting */ ao_btm_cmd("ATQ1\r"); - ao_btm_stdio = ao_add_stdio(ao_serial_btm_pollchar, + ao_btm_stdio = ao_add_stdio(_ao_serial_btm_pollchar, ao_serial_btm_putchar, NULL); ao_btm_echo(0); diff --git a/src/drivers/ao_packet.c b/src/drivers/ao_packet.c index 91319923..5a507478 100644 --- a/src/drivers/ao_packet.c +++ b/src/drivers/ao_packet.c @@ -169,12 +169,10 @@ ao_packet_putchar(char c) __reentrant tx_data[ao_packet_tx_used++] = c; } +/* May be called with interrupts blocked */ int -ao_packet_pollchar(void) +_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 023c788b..4c0dc573 100644 --- a/src/drivers/ao_packet_master.c +++ b/src/drivers/ao_packet_master.c @@ -21,7 +21,12 @@ static char ao_packet_getchar(void) { int c; - while ((c = ao_packet_pollchar()) == AO_READ_AGAIN) { + + /* No need to block interrupts in this function as + * all packet variables are only modified from task + * context, not an interrupt handler + */ + while ((c = _ao_packet_pollchar()) == AO_READ_AGAIN) { if (!ao_packet_enable) break; if (ao_packet_master_sleeping) diff --git a/src/drivers/ao_packet_slave.c b/src/drivers/ao_packet_slave.c index e45775cb..e75df0d6 100644 --- a/src/drivers/ao_packet_slave.c +++ b/src/drivers/ao_packet_slave.c @@ -59,7 +59,7 @@ ao_packet_slave_stop(void) void ao_packet_slave_init(uint8_t enable) { - ao_add_stdio(ao_packet_pollchar, + ao_add_stdio(_ao_packet_pollchar, ao_packet_putchar, NULL); if (enable) diff --git a/src/stm/ao_serial_stm.c b/src/stm/ao_serial_stm.c index ce33f97e..2133c584 100644 --- a/src/stm/ao_serial_stm.c +++ b/src/stm/ao_serial_stm.c @@ -59,24 +59,11 @@ ao_usart_isr(struct ao_stm_usart *usart, int stdin) } } -char -ao_usart_getchar(struct ao_stm_usart *usart) -{ - char c; - ao_arch_block_interrupts(); - while (ao_fifo_empty(usart->rx_fifo)) - ao_sleep(&usart->rx_fifo); - ao_fifo_remove(usart->rx_fifo, c); - ao_arch_release_interrupts(); - return c; -} - int -ao_usart_pollchar(struct ao_stm_usart *usart) +_ao_usart_pollchar(struct ao_stm_usart *usart) { int c; - ao_arch_block_interrupts(); if (ao_fifo_empty(usart->rx_fifo)) c = AO_READ_AGAIN; else { @@ -84,10 +71,20 @@ ao_usart_pollchar(struct ao_stm_usart *usart) ao_fifo_remove(usart->rx_fifo,u); c = u; } - ao_arch_release_interrupts(); return c; } +char +ao_usart_getchar(struct ao_stm_usart *usart) +{ + int c; + ao_arch_block_interrupts(); + while ((c = _ao_usart_pollchar(usart)) == AO_READ_AGAIN) + ao_sleep(&usart->rx_fifo); + ao_arch_release_interrupts(); + return (char) c; +} + void ao_usart_putchar(struct ao_stm_usart *usart, char c) { @@ -201,9 +198,9 @@ ao_serial1_putchar(char c) } int -ao_serial1_pollchar(void) +_ao_serial1_pollchar(void) { - return ao_usart_pollchar(&ao_stm_usart1); + return _ao_usart_pollchar(&ao_stm_usart1); } void @@ -232,9 +229,9 @@ ao_serial2_putchar(char c) } int -ao_serial2_pollchar(void) +_ao_serial2_pollchar(void) { - return ao_usart_pollchar(&ao_stm_usart2); + return _ao_usart_pollchar(&ao_stm_usart2); } void @@ -263,9 +260,9 @@ ao_serial3_putchar(char c) } int -ao_serial3_pollchar(void) +_ao_serial3_pollchar(void) { - return ao_usart_pollchar(&ao_stm_usart3); + return _ao_usart_pollchar(&ao_stm_usart3); } void @@ -309,7 +306,7 @@ ao_serial_init(void) stm_nvic_set_enable(STM_ISR_USART1_POS); stm_nvic_set_priority(STM_ISR_USART1_POS, 4); #if USE_SERIAL_1_STDIN - ao_add_stdio(ao_serial1_pollchar, + ao_add_stdio(_ao_serial1_pollchar, ao_serial1_putchar, NULL); #endif @@ -346,7 +343,7 @@ ao_serial_init(void) stm_nvic_set_enable(STM_ISR_USART2_POS); stm_nvic_set_priority(STM_ISR_USART2_POS, 4); #if USE_SERIAL_2_STDIN - ao_add_stdio(ao_serial2_pollchar, + ao_add_stdio(_ao_serial2_pollchar, ao_serial2_putchar, NULL); #endif @@ -390,7 +387,7 @@ ao_serial_init(void) stm_nvic_set_enable(STM_ISR_USART3_POS); stm_nvic_set_priority(STM_ISR_USART3_POS, 4); #if USE_SERIAL_3_STDIN - ao_add_stdio(ao_serial3_pollchar, + ao_add_stdio(_ao_serial3_pollchar, ao_serial3_putchar, NULL); #endif diff --git a/src/stm/ao_usb_stm.c b/src/stm/ao_usb_stm.c index 9379e5cd..dfa58c42 100644 --- a/src/stm/ao_usb_stm.c +++ b/src/stm/ao_usb_stm.c @@ -229,10 +229,9 @@ ao_usb_set_stat_tx(int ep, uint32_t stat_tx) } static void -ao_usb_set_stat_rx(int ep, uint32_t stat_rx) { +_ao_usb_set_stat_rx(int ep, uint32_t stat_rx) { uint32_t epr_write, epr_old; - ao_arch_block_interrupts(); epr_write = epr_old = stm_usb.epr[ep]; epr_write &= STM_USB_EPR_PRESERVE_MASK; epr_write |= STM_USB_EPR_INVARIANT; @@ -240,6 +239,12 @@ ao_usb_set_stat_rx(int ep, uint32_t stat_rx) { STM_USB_EPR_STAT_RX_MASK << STM_USB_EPR_STAT_RX, stat_rx << STM_USB_EPR_STAT_RX); stm_usb.epr[ep] = epr_write; +} + +static void +ao_usb_set_stat_rx(int ep, uint32_t stat_rx) { + ao_arch_block_interrupts(); + _ao_usb_set_stat_rx(ep, stat_rx); ao_arch_release_interrupts(); } @@ -870,10 +875,10 @@ _ao_usb_out_recv(void) ao_usb_rx_pos = 0; /* ACK the packet */ - ao_usb_set_stat_rx(AO_USB_OUT_EPR, STM_USB_EPR_STAT_RX_VALID); + _ao_usb_set_stat_rx(AO_USB_OUT_EPR, STM_USB_EPR_STAT_RX_VALID); } -static int +int _ao_usb_pollchar(void) { uint8_t c; @@ -896,16 +901,6 @@ _ao_usb_pollchar(void) return c; } -int -ao_usb_pollchar(void) -{ - int c; - ao_arch_block_interrupts(); - c = _ao_usb_pollchar(); - ao_arch_release_interrupts(); - return c; -} - char ao_usb_getchar(void) { -- 2.30.2