Fix infinite recursion issue in SERCOM::startTransmissionWIRE

README.md

@@ -3,6 +3,16 @@
 This repository contains the source code and configuration files of the Arduino Core
 for Atmel's SAMD21 processor (used on the Arduino/Genuino Zero, MKR1000 and MKRZero boards).
 
+## My mods contained in 'wire_timeout' branch
+
+This version of the core has the following changes:
+
+1. A modified version of jrowberg's excellent i2c wire timeout implementation. See pull request [#439](https://github.com/arduino/ArduinoCore-samd/pull/439). By the way I've made the default timeout 25ms. An example modification of Adafruits SSD1306 library for a 128x64 OLED is [here](https://github.com/acicuc/Adafruit_SSD1306/tree/wire_timeout). Albeit old, it still demonstrates one use for the i2c timeout implementation.
+2. A fix for the infamous infinite recursion SERCOM issue, see pull request [#535](https://github.com/arduino/ArduinoCore-samd/pull/535).
+3. The Serial.flush() blocks forever fix, see issue [#597](https://github.com/arduino/ArduinoCore-samd/issues/597).
+
+These are currently the main issues I've struck while working with the SAMD21 in the 'real' world. In particular 1&2 above have allowed for hot-swap and plug&play for devices on the i2c bus, and allows for complete device failure without locking up your entire system or blowing the stack! I'll add others as they raise their ugly heads within my projects.
+
 ## Installation on Arduino IDE
 
 This core is available as a package in the Arduino IDE cores manager.

cores/arduino/SERCOM.cpp

@@ -29,6 +29,8 @@
 SERCOM::SERCOM(Sercom* s)
 {
   sercom = s;
+  timeoutOccurred = false;
+  timeoutInterval = SERCOM_DEFAULT_I2C_OPERATION_TIMEOUT_MS;
 }
 
 /* 	=========================
@@ -111,12 +113,10 @@ void SERCOM::enableUART()
 
 void SERCOM::flushUART()
 {
-  // Skip checking transmission completion if data register is empty
-  if(isDataRegisterEmptyUART())
-    return;
+  // Wait for transmission to complete, if ok to do so.
+  while(!sercom->USART.INTFLAG.bit.TXC && onFlushWaitUartTXC);
 
-  // Wait for transmission to complete
-  while(!sercom->USART.INTFLAG.bit.TXC);
+  onFlushWaitUartTXC = false;
 }
 
 void SERCOM::clearStatusUART()
@@ -183,6 +183,10 @@ int SERCOM::writeDataUART(uint8_t data)
 
   //Put data into DATA register
   sercom->USART.DATA.reg = (uint16_t)data;
+
+  // indicate it's ok to wait for TXC flag when flushing
+  onFlushWaitUartTXC = true;
+
   return 1;
 }
 
@@ -393,11 +397,8 @@ void SERCOM::enableWIRE()
 
   // Setting bus idle mode
   sercom->I2CM.STATUS.bit.BUSSTATE = 1 ;
+  waitSyncWIRE();
 
-  while ( sercom->I2CM.SYNCBUSY.bit.SYSOP != 0 )
-  {
-    // Wait the SYSOP bit from SYNCBUSY coming back to 0
-  }
 }
 
 void SERCOM::disableWIRE()
@@ -433,10 +434,8 @@ void SERCOM::initSlaveWIRE( uint8_t ucAddress, bool enableGeneralCall )
                               SERCOM_I2CS_INTENSET_AMATCH | // Address Match
                               SERCOM_I2CS_INTENSET_DRDY ;   // Data Ready
 
-  while ( sercom->I2CM.SYNCBUSY.bit.SYSOP != 0 )
-  {
-    // Wait the SYSOP bit from SYNCBUSY to come back to 0
-  }
+  waitSyncWIRE();
+
 }
 
 void SERCOM::initMasterWIRE( uint32_t baudrate )
@@ -453,12 +452,12 @@ void SERCOM::initMasterWIRE( uint32_t baudrate )
   // Enable Smart mode and Quick Command
   //sercom->I2CM.CTRLB.reg =  SERCOM_I2CM_CTRLB_SMEN /*| SERCOM_I2CM_CTRLB_QCEN*/ ;
 
-
   // Enable all interrupts
 //  sercom->I2CM.INTENSET.reg = SERCOM_I2CM_INTENSET_MB | SERCOM_I2CM_INTENSET_SB | SERCOM_I2CM_INTENSET_ERROR ;
 
   // Synchronous arithmetic baudrate
   sercom->I2CM.BAUD.bit.BAUD = SystemCoreClock / ( 2 * baudrate) - 5 - (((SystemCoreClock / 1000000) * WIRE_RISE_TIME_NANOSECONDS) / (2 * 1000));
+
 }
 
 void SERCOM::prepareNackBitWIRE( void )
@@ -485,71 +484,67 @@ void SERCOM::prepareCommandBitsWire(uint8_t cmd)
 {
   if(isMasterWIRE()) {
     sercom->I2CM.CTRLB.bit.CMD = cmd;
+    waitSyncWIRE();
+  } else {
+    sercom->I2CS.CTRLB.bit.CMD = cmd;
+  }
+}
 
+void SERCOM::waitSyncWIRE() {
     while(sercom->I2CM.SYNCBUSY.bit.SYSOP)
     {
       // Waiting for synchronization
     }
-  } else {
-    sercom->I2CS.CTRLB.bit.CMD = cmd;
-  }
 }
 
 bool SERCOM::startTransmissionWIRE(uint8_t address, SercomWireReadWriteFlag flag)
 {
+  bool restart = false; // transmission restart flag, is set to true on loss of arbitration
+
   // 7-bits address + 1-bits R/W
   address = (address << 0x1ul) | flag;
 
-  // If another master owns the bus or the last bus owner has not properly
-  // sent a stop, return failure early. This will prevent some misbehaved
-  // devices from deadlocking here at the cost of the caller being responsible
-  // for retrying the failed transmission. See SercomWireBusState for the
-  // possible bus states.
-  if(!isBusOwnerWIRE())
+  // mark timeout timer start
+  initTimeout();
+
+  do
   {
-    if( isBusBusyWIRE() || (isArbLostWIRE() && !isBusIdleWIRE()) )
+    // Send start and address
+    sercom->I2CM.ADDR.bit.ADDR = address;
+    waitSyncWIRE();
+
+    // wait Address Transmitted
+    if ( flag == WIRE_WRITE_FLAG ) // Write mode
     {
-      return false;
+      while (!sercom->I2CM.INTFLAG.bit.MB && !testTimeout())
+      {
+          // wait transmission complete
+      }
     }
-  }
-
-  // Send start and address
-  sercom->I2CM.ADDR.bit.ADDR = address;
-
-  // Address Transmitted
-  if ( flag == WIRE_WRITE_FLAG ) // Write mode
-  {
-    while( !sercom->I2CM.INTFLAG.bit.MB )
+    else  // Read mode
     {
-      // Wait transmission complete
+      while (!sercom->I2CM.INTFLAG.bit.SB && !testTimeout())
+      {
+          // wait transmission complete
+      }
     }
+
     // Check for loss of arbitration (multiple masters starting communication at the same time)
-    if(!isBusOwnerWIRE())
+    if (!isBusOwnerWIRE() && !didTimeout())
     {
-      // Restart communication
-      startTransmissionWIRE(address >> 1, flag);
-    }
-  }
-  else  // Read mode
-  {
-    while( !sercom->I2CM.INTFLAG.bit.SB )
+      // small 1ms delay before restart
+      uint32_t start = micros();
+      while ((micros() - start) < 1000) yield();
+      restart = true;
+    } else
     {
-        // If the slave NACKS the address, the MB bit will be set.
-        // In that case, send a stop condition and return false.
-        if (sercom->I2CM.INTFLAG.bit.MB) {
-            sercom->I2CM.CTRLB.bit.CMD = 3; // Stop condition
-            return false;
-        }
-      // Wait transmission complete
+      restart = false;
     }
 
-    // Clean the 'Slave on Bus' flag, for further usage.
-    //sercom->I2CM.INTFLAG.bit.SB = 0x1ul;
-  }
-
+  } while (restart && !didTimeout());
 
-  //ACK received (0: ACK, 1: NACK)
-  if(sercom->I2CM.STATUS.bit.RXNACK)
+  //ACK received (0: ACK, 1: NACK) or timeout
+  if(sercom->I2CM.STATUS.bit.RXNACK || didTimeout())
   {
     return false;
   }
@@ -563,17 +558,18 @@ bool SERCOM::sendDataMasterWIRE(uint8_t data)
 {
   //Send data
   sercom->I2CM.DATA.bit.DATA = data;
+  waitSyncWIRE();
 
   //Wait transmission successful
-  while(!sercom->I2CM.INTFLAG.bit.MB) {
-
-    // If a bus error occurs, the MB bit may never be set.
-    // Check the bus error bit and bail if it's set.
-    if (sercom->I2CM.STATUS.bit.BUSERR) {
-      return false;
-    }
+  initTimeout();
+  while (!sercom->I2CM.INTFLAG.bit.MB && !testTimeout())
+  {
+      // Wait transmission complete
   }
 
+  // check for timeout condition
+  if ( didTimeout() ) return false;
+
   //Problems on line? nack received?
   if(sercom->I2CM.STATUS.bit.RXNACK)
     return false;
@@ -665,9 +661,10 @@ uint8_t SERCOM::readDataWIRE( void )
 {
   if(isMasterWIRE())
   {
-    while( sercom->I2CM.INTFLAG.bit.SB == 0 && sercom->I2CM.INTFLAG.bit.MB == 0 )
+    initTimeout();
+    while (!sercom->I2CM.INTFLAG.bit.SB && !testTimeout())
     {
-      // Waiting complete receive
+        // wait receive
     }
 
     return sercom->I2CM.DATA.bit.DATA ;
@@ -739,3 +736,26 @@ void SERCOM::initClockNVIC( void )
     /* Wait for synchronization */
   }
 }
+
+void SERCOM::setTimeout( uint16_t ms )
+{
+  timeoutInterval = ms;
+}
+
+bool SERCOM::didTimeout( void )
+{
+  return timeoutOccurred;
+}
+
+void SERCOM::initTimeout( void )
+{
+  timeoutOccurred = false;
+  timeoutRef = millis();
+}
+
+bool SERCOM::testTimeout( void )
+{
+  if (!timeoutInterval) return false;
+  timeoutOccurred = (millis() - timeoutRef) > timeoutInterval;
+  return timeoutOccurred;
+}

cores/arduino/SERCOM.h

@@ -24,6 +24,9 @@
 #define SERCOM_FREQ_REF      48000000
 #define SERCOM_NVIC_PRIORITY ((1<<__NVIC_PRIO_BITS) - 1)
 
+// timeout detection default length (zero is disabled)
+#define SERCOM_DEFAULT_I2C_OPERATION_TIMEOUT_MS 25
+
 typedef enum
 {
 	UART_EXT_CLOCK = 0,
@@ -191,10 +194,10 @@ class SERCOM
 
 		void resetWIRE( void ) ;
 		void enableWIRE( void ) ;
-    void disableWIRE( void );
-    void prepareNackBitWIRE( void ) ;
-    void prepareAckBitWIRE( void ) ;
-    void prepareCommandBitsWire(uint8_t cmd);
+		void disableWIRE( void );
+		void prepareNackBitWIRE( void ) ;
+		void prepareAckBitWIRE( void ) ;
+		void prepareCommandBitsWire(uint8_t cmd);
 		bool startTransmissionWIRE(uint8_t address, SercomWireReadWriteFlag flag) ;
 		bool sendDataMasterWIRE(uint8_t data) ;
 		bool sendDataSlaveWIRE(uint8_t data) ;
@@ -209,15 +212,31 @@ class SERCOM
 		bool isRestartDetectedWIRE( void ) ;
 		bool isAddressMatch( void ) ;
 		bool isMasterReadOperationWIRE( void ) ;
-    bool isRXNackReceivedWIRE( void ) ;
+		bool isRXNackReceivedWIRE( void ) ;
 		int availableWIRE( void ) ;
 		uint8_t readDataWIRE( void ) ;
+		void setTimeout( uint16_t ms );
+		bool didTimeout( void );
 
 	private:
 		Sercom* sercom;
 		uint8_t calculateBaudrateSynchronous(uint32_t baudrate) ;
 		uint32_t division(uint32_t dividend, uint32_t divisor) ;
 		void initClockNVIC( void ) ;
+
+		void waitSyncWIRE( void ) ;
+
+		// timeout detection and tx restart for I2C operations
+		void initTimeout( void );
+		bool testTimeout( void );
+		uint16_t timeoutInterval;
+		uint32_t timeoutRef;
+		bool timeoutOccurred;
+
+		// Flag set when data is loaded into sercom->USART.DATA.reg.
+		// Helps with preventing UART lockups when flushing on startup
+		// and the asyncronous nature of the DRE and TXC interrupt flags.
+		bool onFlushWaitUartTXC = false;
 };
 
 #endif

libraries/Wire/Wire.cpp

@@ -35,6 +35,9 @@ TwoWire::TwoWire(SERCOM * s, uint8_t pinSDA, uint8_t pinSCL)
 }
 
 void TwoWire::begin(void) {
+  // track baud clock for auto-restarting bus in timeout condition
+  activeBaudrate = TWI_CLOCK;
+
   //Master Mode
   sercom->initMasterWIRE(TWI_CLOCK);
   sercom->enableWIRE();
@@ -53,6 +56,9 @@ void TwoWire::begin(uint8_t address, bool enableGeneralCall) {
 }
 
 void TwoWire::setClock(uint32_t baudrate) {
+  // track baud clock for auto-restarting bus in timeout condition
+  activeBaudrate = baudrate;
+
   sercom->disableWIRE();
   sercom->initMasterWIRE(baudrate);
   sercom->enableWIRE();
@@ -70,6 +76,7 @@ uint8_t TwoWire::requestFrom(uint8_t address, size_t quantity, bool stopBit)
   }
 
   size_t byteRead = 0;
+  bool busOwner;
 
   rxBuffer.clear();
 
@@ -78,26 +85,33 @@ uint8_t TwoWire::requestFrom(uint8_t address, size_t quantity, bool stopBit)
     // Read first data
     rxBuffer.store_char(sercom->readDataWIRE());
 
-    bool busOwner;
     // Connected to slave
-    for (byteRead = 1; byteRead < quantity && (busOwner = sercom->isBusOwnerWIRE()); ++byteRead)
+    for (byteRead = 1; byteRead < quantity && !sercom->didTimeout() && (busOwner = sercom->isBusOwnerWIRE()); ++byteRead)
     {
-      sercom->prepareAckBitWIRE();                          // Prepare Acknowledge
-      sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_READ); // Prepare the ACK command for the slave
-      rxBuffer.store_char(sercom->readDataWIRE());          // Read data and send the ACK
-    }
-    sercom->prepareNackBitWIRE();                           // Prepare NACK to stop slave transmission
-    //sercom->readDataWIRE();                               // Clear data register to send NACK
+      // prepare and send ACK for the slave
+      sercom->prepareAckBitWIRE();
+      sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_READ);
 
-    if (stopBit && busOwner)
-    {
-      sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP);   // Send Stop unless arbitration was lost
+      if (quantity > 1) rxBuffer.store_char(sercom->readDataWIRE());    // Read next byte
     }
 
-    if (!busOwner)
+    sercom->prepareNackBitWIRE(); // prepare NACK for slave
+
+    if (stopBit || didTimeout() || !busOwner) sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP); // Send Stop
+
+    if (!busOwner || sercom->didTimeout())
     {
       byteRead--;   // because last read byte was garbage/invalid
     }
+
+  }
+
+  // catch and handle timeout condition
+  if (sercom->didTimeout())
+  {
+    // reset the bus
+    setClock(activeBaudrate);
+    return 0;
   }
 
   return byteRead;
@@ -121,35 +135,42 @@ void TwoWire::beginTransmission(uint8_t address) {
 //  1 : Data too long
 //  2 : NACK on transmit of address
 //  3 : NACK on transmit of data
-//  4 : Other error
+//  4 : Timeout
+//  5 : Other error
 uint8_t TwoWire::endTransmission(bool stopBit)
 {
-  transmissionBegun = false ;
+  uint8_t errCode = 0;
+  bool busOwner;
 
   // Start I2C transmission
-  if ( !sercom->startTransmissionWIRE( txAddress, WIRE_WRITE_FLAG ) )
+  if ( sercom->startTransmissionWIRE( txAddress, WIRE_WRITE_FLAG ) )
   {
-    sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP);
-    return 2 ;  // Address error
-  }
-
-  // Send all buffer
-  while( txBuffer.available() )
-  {
-    // Trying to send data
-    if ( !sercom->sendDataMasterWIRE( txBuffer.read_char() ) )
+    // successful start so transmit data
+    while ( txBuffer.available() && (busOwner = sercom->isBusOwnerWIRE()) )
     {
-      sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP);
-      return 3 ;  // Nack or error
+      // Trying to send data
+      if ( !sercom->sendDataMasterWIRE( txBuffer.read_char() ) )
+      {
+        errCode = 3; // Nack or error
+        txBuffer.clear();
+        break;
+      }
     }
+
+    if (stopBit || errCode || !busOwner) sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP); // Send Stop
+
+  } else errCode = 2; // Address error
+
+  // catch timeout condition
+  if (sercom->didTimeout()) {
+    // reset the bus
+    setClock(activeBaudrate);
+    errCode = 4;
   }
-
-  if (stopBit)
-  {
-    sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP);
-  }   
 
-  return 0;
+  transmissionBegun = false ;
+
+  return errCode;
 }
 
 uint8_t TwoWire::endTransmission()
@@ -219,7 +240,7 @@ void TwoWire::onService(void)
 {
   if ( sercom->isSlaveWIRE() )
   {
-    if(sercom->isStopDetectedWIRE() || 
+    if(sercom->isStopDetectedWIRE() ||
         (sercom->isAddressMatch() && sercom->isRestartDetectedWIRE() && !sercom->isMasterReadOperationWIRE())) //Stop or Restart detected
     {
       sercom->prepareAckBitWIRE();
@@ -230,7 +251,7 @@ void TwoWire::onService(void)
       {
         onReceiveCallback(available());
       }
-      
+
       rxBuffer.clear();
     }
     else if(sercom->isAddressMatch())  //Address Match
@@ -264,12 +285,12 @@ void TwoWire::onService(void)
         transmissionBegun = sercom->sendDataSlaveWIRE(c);
       } else { //Received data
         if (rxBuffer.isFull()) {
-          sercom->prepareNackBitWIRE(); 
+          sercom->prepareNackBitWIRE();
         } else {
           //Store data
           rxBuffer.store_char(sercom->readDataWIRE());
 
-          sercom->prepareAckBitWIRE(); 
+          sercom->prepareAckBitWIRE();
         }
 
         sercom->prepareCommandBitsWire(0x03);
@@ -334,4 +355,3 @@ void TwoWire::onService(void)
     Wire5.onService();
   }
 #endif
-

libraries/Wire/Wire.h

@@ -29,6 +29,9 @@
  // WIRE_HAS_END means Wire has end()
 #define WIRE_HAS_END 1
 
+ // WIRE_HAS_TIMEOUT means Wire implements timeout detection
+#define WIRE_HAS_TIMEOUT 1
+
 class TwoWire : public Stream
 {
   public:
@@ -45,6 +48,9 @@ class TwoWire : public Stream
     uint8_t requestFrom(uint8_t address, size_t quantity, bool stopBit);
     uint8_t requestFrom(uint8_t address, size_t quantity);
 
+    bool didTimeout() { return sercom->didTimeout(); }
+    void setTimeout(uint16_t ms) { sercom->setTimeout(ms); }
+
     size_t write(uint8_t data);
     size_t write(const uint8_t * data, size_t quantity);
 
@@ -70,6 +76,9 @@ class TwoWire : public Stream
 
     bool transmissionBegun;
 
+    // Used to re-initialize the clock rate after a timeout
+    uint32_t activeBaudrate;
+
     // RX Buffer
     RingBufferN<256> rxBuffer;