Add architecture comments, cleanup uart.c code

Comments added to UART.c trying to explain (as best as I understand it)
the changes done to support GDB and how they interact with standard
operation.

Fix the uart_uninit to stop the ISR and then free appropriately.

Fix uart_isr_handle_data (GDB's shim for sending chars to the 8266 app)
to do the exact same thing as the standard UART handler including set
the overflow properly and either discard or overwrite in that case.

Fix serial reception when GDB enabled by enabling the user recv ISR.

Remove commented attributes from gdbstub, leftover from the move to
flash.

General logic cleanup per comments in the PR.

Author: earlephilhower

cores/esp8266/uart.c

@@ -47,7 +47,29 @@
 #include "user_interface.h"
 #include "uart_register.h"
 
-//const char overrun_str [] PROGMEM STORE_ATTR = "uart input full!\r\n";
+/*
+  Some general architecture for GDB integration with the UART to enable
+  serial debugging.
+
+  UART1 is transmit only and can never be used by GDB.
+
+  When gdbstub_has_uart_isr_control() (only true in the case GDB is enabled),
+  UART0 needs to be under the control of the GDB stub for enable/disable/irq
+  (but speed, parity, etc. still alllowable).  Basically, GDB needs to make
+  sure that UART0 is never really disabled.
+
+  GDB sets up UART0 with a fifo and a 2-character timeout during init.  This
+  is required to ensure that GDBStub can check every character coming in, even
+  if it is not read by the user app or if the commands don't hit the FIFO
+  interrupt level.  It checks every character that comes in, and if GDB isn't
+  active just passes them to the user RAM FIFO unless it's a Ctrl-C (0x03).
+
+  GDBStub doesn't care about the struct uart_*, and allocating it or freeing
+  it has no effect (so you can do Serial.end() and free the uart...but as
+  mentioned above even if you Serial.end, the actual UART0 HW will still be
+  kept running to enable GDB to do commands.
+*/
+
 static int s_uart_debug_nr = UART0;
 
 
@@ -250,17 +272,41 @@ uart_read(uart_t* uart, char* userbuffer, size_t usersize)
     return ret;
 }
 
+// When GDB is running, this is called one byte at a time to stuff the user FIFO
+// instead of the uart_isr...uart_rx_copy_fifo_to_buffer_unsafe()
+// Since we've already read the bytes from the FIFO, can't use that
+// function directly and need to implement it bytewise here
 static void ICACHE_RAM_ATTR uart_isr_handle_data(void* arg, uint8_t data)
 {
     uart_t* uart = (uart_t*)arg;
     if(uart == NULL || !uart->rx_enabled) {
         return;
     }
-    size_t nextPos = (uart->rx_buffer->wpos + 1) % uart->rx_buffer->size;
-    if(nextPos != uart->rx_buffer->rpos) {
-        uart->rx_buffer->buffer[uart->rx_buffer->wpos] = data;
-        uart->rx_buffer->wpos = nextPos;
+
+// Copy all the rx fifo bytes that fit into the rx buffer
+// called by ISR
+    struct uart_rx_buffer_ *rx_buffer = uart->rx_buffer;
+
+    size_t nextPos = (rx_buffer->wpos + 1) % rx_buffer->size;
+    if(nextPos == rx_buffer->rpos)
+    {
+        uart->rx_overrun = true;
+        //os_printf_plus(overrun_str);
+
+        // a choice has to be made here,
+        // do we discard newest or oldest data?
+#ifdef UART_DISCARD_NEWEST
+        // discard newest data
+        // Stop copying if rx buffer is full
+        return;
+#else
+        // discard oldest data
+        if (++rx_buffer->rpos == rx_buffer->size)
+            rx_buffer->rpos = 0;
+#endif
     }
+    rx_buffer->buffer[rx_buffer->wpos] = data;
+    rx_buffer->wpos = nextPos;
 }
 
 size_t 
@@ -299,7 +345,7 @@ uart_get_rx_buffer_size(uart_t* uart)
     return uart && uart->rx_enabled? uart->rx_buffer->size: 0;
 }
 
-
+// The default ISR handler called when GDB is not enabled
 void ICACHE_RAM_ATTR 
 uart_isr(void * arg)
 {
@@ -576,6 +622,7 @@ uart_init(int uart_nr, int baudrate, int config, int mode, int tx_pin, size_t rx
         uart->tx_pin = (uart->tx_enabled)?2:255;  // GPIO7 as TX not possible! See GPIO pins used by UART
         if(uart->tx_enabled)
             pinMode(uart->tx_pin, SPECIAL);
+
         break;
 
     case UART_NO:
@@ -595,11 +642,11 @@ uart_init(int uart_nr, int baudrate, int config, int mode, int tx_pin, size_t rx
         USIE(uart->uart_nr) = 0;
     }
     if(uart->uart_nr == UART0) {
-        if(uart->rx_enabled && !gdbstub_has_uart_isr_control()) {
+        if(uart->rx_enabled) {
             uart_start_isr(uart);
         }
         if(gdbstub_has_uart_isr_control()) {
-            ETS_UART_INTR_ENABLE();
+            ETS_UART_INTR_ENABLE(); // Undo the disable in the switch() above
         }
     }
 
@@ -612,6 +659,8 @@ uart_uninit(uart_t* uart)
     if(uart == NULL)
         return;
 
+    uart_stop_isr(uart);
+
     if(uart->tx_enabled && (!gdbstub_has_uart_isr_control() || uart->uart_nr != UART0)) {
         switch(uart->tx_pin) 
         {
@@ -640,7 +689,6 @@ uart_uninit(uart_t* uart)
                 pinMode(13, INPUT);
                 break;
             }
-            uart_stop_isr(uart);
         }
     }
     free(uart);
@@ -852,11 +900,15 @@ uart_set_debug(int uart_nr)
         func = &uart_ignore_char;
         break;
     }
-    if(!gdbstub_has_putc1_control()) {
-        system_set_os_print((uint8)((uart_nr == UART0 || uart_nr == UART1)?1:0));
-        ets_install_putc1((void *) func);
-    } else {
+    if(gdbstub_has_putc1_control()) {
         gdbstub_set_putc1_callback(func);
+    } else {
+        if (uart_nr == UART0 || uart_nr == UART1) {
+            system_set_os_print(1);
+        } else {
+            system_set_os_print(0);
+        }
+        ets_install_putc1((void *) func);
     }
 }
 

libraries/GDBStub/src/internal/gdbstub.c

@@ -138,7 +138,7 @@ static void ATTR_GDBFN keepWDTalive() {
 //of the hex string, as far as the routine has read into it. Bits/4 indicates
 //the max amount of hex chars it gobbles up. Bits can be -1 to eat up as much
 //hex chars as possible.
-static long /*ATTR_GDBFN*/ gdbGetHexVal(unsigned char **ptr, int bits) {
+static long  gdbGetHexVal(unsigned char **ptr, int bits) {
 	int i;
 	int no;
 	unsigned int v = 0;
@@ -176,22 +176,22 @@ static long /*ATTR_GDBFN*/ gdbGetHexVal(unsigned char **ptr, int bits) {
 }
 
 //Swap an int into the form gdb wants it
-static int /*ATTR_GDBFN*/ iswap(int i) {
+static int iswap(int i) {
 	return ((i >> 24) & 0xff)
 		| (((i >> 16) & 0xff) << 8)
 		| (((i >> 8) & 0xff) << 16)
 		| (((i >> 0) & 0xff) << 24);
 }
 
 //Read a byte from the ESP8266 memory.
-static unsigned char /*ATTR_GDBFN*/ readbyte(unsigned int p) {
+static unsigned char readbyte(unsigned int p) {
 	if (p < 0x20000000 || p >= 0x60000000) return -1;
 	int *i = (int*)(p & ~3);
 	return *i >> ((p & 3) * 8);
 }
 
 //Write a byte to the ESP8266 memory.
-static void /*ATTR_GDBFN*/ writeByte(unsigned int p, unsigned char d) {
+static void writeByte(unsigned int p, unsigned char d) {
 	if (p < 0x20000000 || p >= 0x60000000) return;
 	int *i = (int*)(p & ~3);
 	if ((p & 3) == 0) *i = (*i & 0xffffff00) | (d << 0);
@@ -201,7 +201,7 @@ static void /*ATTR_GDBFN*/ writeByte(unsigned int p, unsigned char d) {
 }
 
 //Returns 1 if it makes sense to write to addr p
-static int /*ATTR_GDBFN*/ validWrAddr(int p) {
+static int validWrAddr(int p) {
 	return (p >= 0x3ff00000 && p < 0x40000000)
 		|| (p >= 0x40100000 && p < 0x40140000)
 		|| (p >= 0x60000000 && p < 0x60002000);
@@ -217,29 +217,29 @@ static inline bool ATTR_GDBFN gdbTxFifoIsFull() {
 }
 
 //Receive a char from the uart. Uses polling and feeds the watchdog.
-static inline int /*ATTR_GDBFN*/ gdbRecvChar() {
+static inline int gdbRecvChar() {
 	while (gdbRxFifoIsEmpty()) {
 		keepWDTalive();
 	}
 	return READ_PERI_REG(UART_FIFO(0));
 }
 
 //Send a char to the uart.
-static void /*ATTR_GDBFN*/ gdbSendChar(char c) {
+static void gdbSendChar(char c) {
 	while (gdbTxFifoIsFull())
 		;
 	WRITE_PERI_REG(UART_FIFO(0), c);
 }
 
 
 //Send the start of a packet; reset checksum calculation.
-static void /*ATTR_GDBFN*/ gdbPacketStart() {
+static void gdbPacketStart() {
 	chsum = 0;
 	gdbSendChar('$');
 }
 
 //Send a char as part of a packet
-static void /*ATTR_GDBFN*/ gdbPacketChar(char c) {
+static void gdbPacketChar(char c) {
 	if (c == '#' || c == '$' || c == '}' || c == '*') {
 		gdbSendChar('}');
 		chsum += '}';
@@ -250,7 +250,7 @@ static void /*ATTR_GDBFN*/ gdbPacketChar(char c) {
 }
 
 //Send a hex val as part of a packet. 'bits'/4 dictates the number of hex chars sent.
-static void /*ATTR_GDBFN*/ gdbPacketHex(int val, int bits) {
+static void gdbPacketHex(int val, int bits) {
 	static const char hexChars[] = "0123456789abcdef";
 	int i;
 	for (i = bits; i > 0; i -= 4) {
@@ -259,24 +259,24 @@ static void /*ATTR_GDBFN*/ gdbPacketHex(int val, int bits) {
 }
 
 //Send a hex val as part of a packet. 'bits'/4 dictates the number of hex chars sent.
-static void /*ATTR_GDBFN*/ gdbPacketSwappedHexInt(int val) {
+static void gdbPacketSwappedHexInt(int val) {
 	gdbPacketHex(iswap(val), 32);
 }
 
-static void /*ATTR_GDBFN*/ gdbPacketXXXXInt() {
+static void gdbPacketXXXXInt() {
 	for (int i=0; i<8; i++) gdbPacketChar('x');
 }
 
 //Finish sending a packet.
-static void /*ATTR_GDBFN*/ gdbPacketEnd() {
+static void gdbPacketEnd() {
 	gdbSendChar('#');
 	//Ok to use packet version here since hex char can never be an
 	//excape-requiring character
 	gdbPacketHex(chsum, 8);
 }
 
 // Send a complete packet containing str
-static void /*ATTR_GDBFN*/ gdbSendPacketStr(const char *c) {
+static void gdbSendPacketStr(const char *c) {
 	gdbPacketStart();
 	while (*c != 0) {
 		gdbPacketChar(*c);
@@ -303,13 +303,13 @@ static inline void ATTR_GDBEXTERNFN gdbSendOutputPacketChar(unsigned char c) {
 	gdbPacketEnd();
 }
 
-static long /*ATTR_GDBFN*/ gdbGetSwappedHexInt(unsigned char **ptr) {
+static long gdbGetSwappedHexInt(unsigned char **ptr) {
 	return iswap(gdbGetHexVal(ptr, 32));
 }
 
 
 //Send the reason execution is stopped to GDB.
-static void /*ATTR_GDBFN*/ sendReason() {
+static void sendReason() {
 	static const char exceptionSignal[] = {4,31,11,11,2,6,8,0,6,7,0,0,7,7,7,7};
 #if 0
 	char *reason=""; //default
@@ -400,7 +400,7 @@ struct regfile {
 */
 
 //Handle a command as received from GDB.
-static inline int /*ATTR_GDBFN*/ gdbHandleCommand() {
+static inline int gdbHandleCommand() {
 	//Handle a command
 	int i, j, k;
 	unsigned char *data = cmd + 1;
@@ -537,7 +537,7 @@ static inline int /*ATTR_GDBFN*/ gdbHandleCommand() {
 //It is not necessary for gdb to be attached for it to be paused
 //For example, during an exception break, the program is
 // paused but gdb might not be attached yet
-static int /*ATTR_GDBFN*/ gdbReadCommand() {
+static int gdbReadCommand() {
 	unsigned char chsum;
 	unsigned char sentchs[2];
 	size_t p;
@@ -606,7 +606,7 @@ static inline void ATTR_GDBFN setaregval(int reg, unsigned int val) {
 }
 
 //Emulate the l32i/s32i instruction we're stopped at.
-static inline void /*ATTR_GDBFN*/ emulLdSt() {
+static inline void emulLdSt() {
 	unsigned char i0 = readbyte(gdbstub_savedRegs.pc);
 	unsigned char i1 = readbyte(gdbstub_savedRegs.pc + 1);
 	unsigned char i2;
@@ -682,7 +682,7 @@ static void __attribute__((noinline)) gdbstub_handle_debug_exception_flash() {
 #if GDBSTUB_BREAK_ON_EXCEPTION || GDBSTUB_CTRLC_BREAK
 
 #if !GDBSTUB_FREERTOS
-static inline int /*ATTR_GDBFN*/ gdbReadCommandWithFrame(void* frame) {
+static inline int gdbReadCommandWithFrame(void* frame) {
 	//Copy registers the Xtensa HAL did save to gdbstub_savedRegs
 	os_memcpy(&gdbstub_savedRegs, frame, 5 * 4);
 	os_memcpy(&gdbstub_savedRegs.a[2], ((uint32_t*)frame) + 5, 14 * 4);