Merge pull request #204 from tessel/stability

Stability fixes

Author: johnnyman727

Diff for: common/dma.c

@@ -21,8 +21,24 @@ void dma_init() {
 }
 
 void dma_abort(DmaChan chan) {
+    __disable_irq();
     DMAC->CHID.reg = chan;
     DMAC->CHCTRLA.reg = 0;
+    __enable_irq();
+}
+
+void dma_start(DmaChan chan) {
+    __disable_irq();
+    DMAC->CHID.reg = chan;
+    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    __enable_irq();
+}
+
+void dma_enable_interrupt(DmaChan chan) {
+    __disable_irq();
+    DMAC->CHID.reg = chan;
+    DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR;
+    __enable_irq();
 }
 
 u32 dma_remaining(DmaChan chan) {
@@ -78,24 +94,21 @@ void dma_link_chain(DmacDescriptor* chain, u32 count) {
 }
 
 void dma_start_descriptor(DmaChan chan, DmacDescriptor* chain) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     memcpy(&dma_descriptors[chan], &chain[0], sizeof(DmacDescriptor));
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }
 
 void dma_sercom_start_tx(DmaChan chan, SercomId id, u8* src, unsigned size) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     dma_fill_sercom_tx(&dma_descriptors[chan], id, src, size);
     dma_descriptors[chan].DESCADDR.reg = 0;
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }
 
 void dma_sercom_start_rx(DmaChan chan, SercomId id, u8* dst, unsigned size) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     dma_fill_sercom_rx(&dma_descriptors[chan], id, dst, size);
     dma_descriptors[chan].DESCADDR.reg = 0;
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }

Diff for: common/hw.h

@@ -187,6 +187,7 @@ void dma_init();
 void dma_sercom_start_tx(DmaChan chan, SercomId id, u8* src, unsigned size);
 void dma_sercom_start_rx(DmaChan chan, SercomId id, u8* dst, unsigned size);
 void dma_abort(DmaChan chan);
+void dma_enable_interrupt(DmaChan chan);
 void dma_fill_sercom_tx(DmacDescriptor* desc, SercomId id, u8 *src, unsigned size);
 void dma_fill_sercom_rx(DmacDescriptor* desc, SercomId id, u8 *dst, unsigned size);
 void dma_sercom_configure_tx(DmaChan chan, SercomId id);

Diff for: common/samd21g18a_firmware_partition.ld

@@ -50,7 +50,7 @@ SEARCH_DIR(.)
 MEMORY
 {
   rom    (rx)  : ORIGIN = 0x00001000, LENGTH = 0x0003efff
-  ram    (rwx) : ORIGIN = 0x20000000, LENGTH = 0x00001000
+  ram    (rwx) : ORIGIN = 0x20000000, LENGTH = 0x00008000
 }
 
 /* The stack size used by the application. NOTE: you need to adjust according to your application. */

Diff for: firmware/bridge.c

@@ -120,13 +120,15 @@ void bridge_handle_sync() {
         for (u8 chan=0; chan<BRIDGE_NUM_CHAN; chan++) {
             u8 size = ctrl_rx.size[chan];
             if (ctrl_tx.status & (1<<chan) && size > 0) {
+                out_chan_ready &= ~ (1<<chan);
                 dma_fill_sercom_tx(&dma_chain_data_tx[desc], SERCOM_BRIDGE, NULL, size);
                 dma_fill_sercom_rx(&dma_chain_data_rx[desc], SERCOM_BRIDGE, out_chan_ptr[chan], size);
                 desc++;
             }
 
             size = ctrl_tx.size[chan];
             if (ctrl_rx.status & (1<<chan) && size > 0) {
+                in_chan_size[chan] = 0;
                 dma_fill_sercom_tx(&dma_chain_data_tx[desc], SERCOM_BRIDGE, in_chan_ptr[chan], size);
                 dma_fill_sercom_rx(&dma_chain_data_rx[desc], SERCOM_BRIDGE, NULL, size);
                 desc++;
@@ -161,26 +163,33 @@ void bridge_handle_sync() {
 void bridge_dma_rx_completion() {
     if (bridge_state == BRIDGE_STATE_DATA) {
 
+        // Copy the global state to this stack frame in case SYNC changes and the ISR overwrites these
+        uint8_t rx_status = ctrl_rx.status;
+        uint8_t tx_status = ctrl_tx.status;
+        uint8_t rx_size[BRIDGE_NUM_CHAN];
+        memcpy(rx_size, ctrl_rx.size, sizeof(rx_size));
+        uint8_t tx_size[BRIDGE_NUM_CHAN];
+        memcpy(tx_size, ctrl_tx.size, sizeof(tx_size));
+        __asm__ __volatile__ ("" : : : "memory");
+
         #define CHECK_OPEN(x) \
-            if ((ctrl_rx.status & (0x10<<x)) && !(was_open & (0x10<<x))) { \
-                bridge_open_##x(ctrl_rx.size[x]); \
+            if ((rx_status & (0x10<<x)) && !(was_open & (0x10<<x))) { \
+                bridge_open_##x(rx_size[x]); \
             }
 
         #define CHECK_COMPLETION_OUT(x) \
-            if (ctrl_tx.status & (1<<x) && ctrl_rx.size[x] > 0) { \
-                out_chan_ready &= ~ (1<<x); \
-                bridge_completion_out_##x(ctrl_rx.size[x]); \
+            if (tx_status & (1<<x) && rx_size[x] > 0) { \
+                bridge_completion_out_##x(rx_size[x]); \
             }
 
         #define CHECK_COMPLETION_IN(x) \
-            if (ctrl_rx.status & (1<<x) && ctrl_tx.size[x] > 0) { \
-                in_chan_size[x] = 0; \
+            if (rx_status & (1<<x) && tx_size[x] > 0) { \
                 bridge_completion_in_##x(); \
             }
 
         #define CHECK_CLOSE(x) \
-            if (!(ctrl_rx.status & (0x10<<x)) && (was_open & (0x10<<x))) { \
-                bridge_close_##x(ctrl_rx.size[x]); \
+            if (!(rx_status & (0x10<<x)) && (was_open & (0x10<<x))) { \
+                bridge_close_##x(rx_size[x]); \
             }
 
         CHECK_OPEN(0)
@@ -211,24 +220,32 @@ void bridge_dma_rx_completion() {
 }
 
 void bridge_start_in(u8 channel, u8* data, u8 length) {
+    __disable_irq();
     in_chan_ptr[channel] = data;
     in_chan_size[channel] = length;
+    __enable_irq();
     pin_high(PIN_BRIDGE_IRQ);
 }
 
 void bridge_start_out(u8 channel, u8* data) {
+    __disable_irq();
     out_chan_ptr[channel] = data;
     out_chan_ready |= (1<<channel);
+    __enable_irq();
     pin_high(PIN_BRIDGE_IRQ);
 }
 
 void bridge_enable_chan(u8 channel) {
+    __disable_irq();
     out_chan_ready |= (0x10<<channel);
+    __enable_irq();
     pin_high(PIN_BRIDGE_IRQ);
 }
 
 void bridge_disable_chan(u8 channel) {
+    __disable_irq();
     out_chan_ready &= ~(0x11<<channel); // Also clears the "ready to accept data" bit
     in_chan_size[channel] = 0; // Clears any data that was waiting to be sent
+    __enable_irq();
     pin_high(PIN_BRIDGE_IRQ);
 }

Diff for: firmware/firmware.h

@@ -59,13 +59,16 @@ void flash_usb_in_completion();
 void flash_usb_out_completion();
 void flash_disable();
 
+#define FLASH_BUFFER_SIZE 512
+extern u8 flash_buffer[FLASH_BUFFER_SIZE];
+
 // bridge.c
 
 #define BRIDGE_NUM_CHAN 3
 #define BRIDGE_USB 0
 #define BRIDGE_PORT_A 1
 #define BRIDGE_PORT_B 2
-#define BRIDGE_BUF_SIZE 256
+#define BRIDGE_BUF_SIZE 255
 #define BRIDGE_ARG_SIZE 5
 
 void bridge_init();
@@ -114,25 +117,61 @@ typedef struct UartBuf {
 } UartBuf;
 
 typedef struct PortData {
+    /// Pin mappings
     const TesselPort* port;
+
+    /// Buffers for data from the host
     USB_ALIGN u8 cmd_buf[BRIDGE_BUF_SIZE];
+
+    /// Buffers for data to the host
     USB_ALIGN u8 reply_buf[BRIDGE_BUF_SIZE];
+
+    /// Bridge channel
     u8 chan;
+
+    /// DMA channel for TX
     DmaChan dma_tx;
+
+    /// DMA channel for RX
     DmaChan dma_rx;
+
+    /// Parser state (PortState in port.c)
     u8 state;
+
+    /// Port mode (SPI/UART/etc, PortMode in port.c)
     u8 mode;
+
+    /// Length of valid data in cmd_buf
     u8 cmd_len;
+
+    /// Current position in cmd_buf
     u8 cmd_pos;
+
+    /// Current write position in reply_buf (length of valid data written)
     u8 reply_len;
+
+    /// Currently executing command (PortCmd in port.c)
     u8 cmd;
+
+    /// Parsed arguments
     u8 arg[BRIDGE_ARG_SIZE];
+
+    /// Length of arguments
     u8 arg_len;
+
+    /// Position into arguments
     u8 arg_pos;
-    u8 len;
+
+    /// GCLK channel for this port
     u8 clock_channel;
+
+    /// TCC channel for this port
     u8 tcc_channel;
+
+    /// True if the port is waiting for a packet from the host
     bool pending_out;
+
+    /// True if the port is sending a packet to the host
     bool pending_in;
     UartBuf uart_buf;
 } PortData;
@@ -147,7 +186,7 @@ void port_bridge_out_completion(PortData* p, u8 len);
 void port_bridge_in_completion(PortData* p);
 void port_dma_rx_completion(PortData* p);
 void port_dma_tx_completion(PortData* p);
-void bridge_handle_sercom_uart_i2c(PortData* p);
+void port_handle_sercom_uart_i2c(PortData* p);
 void port_handle_extint(PortData *p, u32 flags);
 void port_disable(PortData *p);
 void uart_send_data(PortData *p);

Diff for: firmware/flash.c

@@ -21,9 +21,6 @@
 //    the SPI bus.
 // 4. `in_count` bytes are read from SPI and sent to the IN endpoint
 
-// Buffers are shared with the port, because they are not used simultaneously
-#define FLASH_BUFFER_SIZE BRIDGE_BUF_SIZE*2
-u8* const flash_buffer = port_a.cmd_buf;
 u32 flash_in_count;
 u32 flash_out_count;
 bool flash_flag_sr_poll;
@@ -62,7 +59,7 @@ void flash_init() {
     sercom_spi_master_init(SERCOM_BRIDGE, FLASH_DIPO, FLASH_DOPO, 0, 0, SERCOM_SPI_BAUD_12MHZ);
     dma_sercom_configure_tx(DMA_FLASH_TX, SERCOM_BRIDGE);
     dma_sercom_configure_rx(DMA_FLASH_RX, SERCOM_BRIDGE);
-    DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR; // ID depends on prev call
+    dma_enable_interrupt(DMA_FLASH_RX);
 
     pin_low(PIN_SOC_RST);
     pin_out(PIN_SOC_RST);

Diff for: firmware/main.c

@@ -3,16 +3,23 @@
 PortData port_a;
 PortData port_b;
 
+// TODO: flash buffer could be shared with the port, because they are not used simultaneously
+USB_ALIGN u8 flash_buffer[FLASH_BUFFER_SIZE];
+
 // Indicates whether the SPI Daemon is listening for USB traffic
 volatile bool booted = false;
 // LED Chan: TCC1/WO[0]
 #define PWR_LED_TCC_CHAN 1
 // CC channel 0 on TCC instance 1
 #define PWR_LED_CC_CHAN 0
-// The maximum counter value was chosen to get a 2s period heartbeat
-#define MAX_COUNTER 0xFFFF
+// The maximum counter value of both the TCC and the pattern counter
+#define MAX_COUNTER 0x10000
 // We have 16 slices of a sine wave
 #define NUM_POINT_SLICES 0x10
+// Pattern period in millisecond
+#define PATTERN_PERIOD_MS 1000
+#define TICKS_PER_MS 48000
+
 // Number of loop iterations in a single slice
 #define COUNTS_IN_SLICE MAX_COUNTER/NUM_POINT_SLICES
 // Evenly spaced points along a sine wave, shifted up by 1, scaled by 0.5
@@ -71,15 +78,19 @@ uint32_t interpolate(uint32_t position) {
     Handler for the POWER LED breathing animation
 */
 void TCC1_Handler() {
+    tcc(PWR_LED_TCC_CHAN)->INTFLAG.reg = TCC_INTFLAG_OVF;
+
     // booted is true when the coprocess first gets
     // a status packet from the spi daemon
     if (booted == true) {
         // Stop this breathing animation and cancel interrupts
         cancel_breathing_animation();
     }
 
+    counter += MAX_COUNTER / PATTERN_PERIOD_MS * MAX_COUNTER / TICKS_PER_MS;
+
     // Take that proportion and extract a point along the sudo sine wave
-    tcc(PWR_LED_TCC_CHAN)->CCB[PWR_LED_CC_CHAN].bit.CCB = interpolate(++counter);
+    tcc(PWR_LED_TCC_CHAN)->CCB[PWR_LED_CC_CHAN].bit.CCB = interpolate(counter);
 }
 
 /*
@@ -95,15 +106,11 @@ void init_breathing_animation() {
 
     // Reset the TCC
     tcc(PWR_LED_TCC_CHAN)->CTRLA.reg = TCC_CTRLA_SWRST;
+    while (tcc(PWR_LED_TCC_CHAN)->SYNCBUSY.reg != 0);
 
     // Enable the timer
     timer_clock_enable(PWR_LED_TCC_CHAN);
 
-    /* Set the prescalar setting to the highest division so we have more time
-        in between interrupts to complete the math
-    */
-    tcc(PWR_LED_TCC_CHAN)->CTRLA.bit.PRESCALER = TCC_CTRLA_PRESCALER_DIV1024_Val;
-
     // Set the waveform generator to generate a PWM signal
     // It uses polarity setting of 1 (switches from DIR to ~DIR)
     tcc(PWR_LED_TCC_CHAN)->WAVE.reg = TCC_WAVE_WAVEGEN_NPWM | TCC_WAVE_POL0;
@@ -112,10 +119,7 @@ void init_breathing_animation() {
     tcc(PWR_LED_TCC_CHAN)->PER.reg = MAX_COUNTER;
 
     // Set the counter number, starting at 0% duty cycle
-    tcc(PWR_LED_TCC_CHAN)->CC[PWR_LED_CC_CHAN].reg = counter;
-
-    // Set the second CCB value value be dark for simplicity
-    tcc(PWR_LED_TCC_CHAN)->CCB[PWR_LED_CC_CHAN].bit.CCB = counter;
+    tcc(PWR_LED_TCC_CHAN)->CC[PWR_LED_CC_CHAN].reg = 0;
 
     // Enable IRQ's in the NVIC
     NVIC_EnableIRQ(TCC1_IRQn);
@@ -128,7 +132,7 @@ void init_breathing_animation() {
     while (tcc(PWR_LED_TCC_CHAN)->SYNCBUSY.reg > 0);
 
     // Enable the TCC
-    tcc(PWR_LED_TCC_CHAN)->CTRLA.reg = TCC_CTRLA_ENABLE;
+    tcc(PWR_LED_TCC_CHAN)->CTRLA.bit.ENABLE = 1;
 }
 
 void boot_delay_ms(int delay){
@@ -285,11 +289,11 @@ void EVSYS_Handler() {
 }
 
 void SERCOM_HANDLER(SERCOM_PORT_A_UART_I2C) {
-    bridge_handle_sercom_uart_i2c(&port_a);
+    port_handle_sercom_uart_i2c(&port_a);
 }
 
 void SERCOM_HANDLER(SERCOM_PORT_B_UART_I2C) {
-    bridge_handle_sercom_uart_i2c(&port_b);
+    port_handle_sercom_uart_i2c(&port_b);
 }
 
 void bridge_open_0() {}