Neil bugfixes. (#186)

* Re-enable native I2C driver.

* Minor non-functional changes to native I2C Manager.

* Minor changes to make variable types explicit in comparisons.

* Fix IODevice::loop() to avoid null pointer dereference.

Strange problems with LCD driver tracked down to being caused by a call to p->_loop() when p is NULL.

* Correct sense of comparison in LCN support function Turnout::setClosedStateOnly()

* Remove code (now unused) from LCD driver.

* Add I2C textual error messages.

* Add I2C textual error messages.

* Fix compile error in 4809 I2C driver.

* Remove init function call from SSD1306 driver.

I2CManager.cpp

@@ -147,6 +147,25 @@ uint8_t I2CManagerClass::finishRB(I2CRB *rb, uint8_t status) {
   return status;
 }
 
+/***************************************************************************
+ * Get a message corresponding to the error status
+ ***************************************************************************/
+const FSH *I2CManagerClass::getErrorMessage(uint8_t status) {
+  switch (status) {
+    case I2C_STATUS_OK: return F("OK");
+    case I2C_STATUS_TRUNCATED: return F("Transmission truncated");
+    case I2C_STATUS_NEGATIVE_ACKNOWLEDGE: return F("No response from device (address NAK)");
+    case I2C_STATUS_TRANSMIT_ERROR: return F("Transmit error (data NAK)");
+    case I2C_STATUS_OTHER_TWI_ERROR: return F("Other Wire/TWI error");
+    case I2C_STATUS_TIMEOUT: return F("Timeout");
+    case I2C_STATUS_ARBITRATION_LOST: return F("Arbitration lost");
+    case I2C_STATUS_BUS_ERROR: return F("I2C bus error");
+    case I2C_STATUS_UNEXPECTED_ERROR: return F("Unexpected error");
+    case I2C_STATUS_PENDING: return F("Request pending");
+    default: return F("Error code not recognised");
+  }
+}
+
 /***************************************************************************
  *  Declare singleton class instance.
  ***************************************************************************/
@@ -158,12 +177,25 @@ I2CManagerClass I2CManager = I2CManagerClass();
 /////////////////////////////////////////////////////////////////////////////
 
 /***************************************************************************
- *  Block waiting for request block to complete, and return completion status
+ *  Block waiting for request block to complete, and return completion status.
+ *  Since such a loop could potentially last for ever if the RB status doesn't
+ *  change, we set a high limit (0.1sec, 100ms) on the wait time and, if it
+ *  hasn't changed by that time we assume it's not going to, and just return
+ *  a timeout status.  This means that CS will not lock up.
  ***************************************************************************/
 uint8_t I2CRB::wait() {
-  do
+  unsigned long waitStart = millis();
+  do {
     I2CManager.loop();
-  while (status==I2C_STATUS_PENDING);
+    // Rather than looping indefinitely, let's set a very high timeout (100ms).
+    if ((millis() - waitStart) > 100UL) { 
+      DIAG(F("I2C TIMEOUT I2C:x%x I2CRB:x%x"), i2cAddress, this);
+      status = I2C_STATUS_TIMEOUT;
+      // Note that, although the timeout is posted, the request may yet complete.
+      // TODO: Ideally we would like to cancel the request.
+      return status;
+    }
+  } while (status==I2C_STATUS_PENDING);
   return status;
 }
 

I2CManager.h

@@ -111,19 +111,22 @@
  * 
  */
 
-#define I2C_USE_WIRE
+//#define I2C_USE_WIRE
 #ifndef I2C_NO_INTERRUPTS
 #define I2C_USE_INTERRUPTS
 #endif
 
 // Status codes for I2CRB structures.
 enum : uint8_t {
+  // Codes used by Wire and by native drivers
   I2C_STATUS_OK=0,
   I2C_STATUS_TRUNCATED=1,
-  I2C_STATUS_DEVICE_NOT_PRESENT=2,
+  I2C_STATUS_NEGATIVE_ACKNOWLEDGE=2,
   I2C_STATUS_TRANSMIT_ERROR=3,
-  I2C_STATUS_NEGATIVE_ACKNOWLEDGE=4,
   I2C_STATUS_TIMEOUT=5,
+  // Code used by Wire only
+  I2C_STATUS_OTHER_TWI_ERROR=4, // catch-all error
+  // Codes used by native drivers only
   I2C_STATUS_ARBITRATION_LOST=6,
   I2C_STATUS_BUS_ERROR=7,
   I2C_STATUS_UNEXPECTED_ERROR=8,
@@ -151,14 +154,16 @@ typedef enum : uint8_t
 #define I2C_FREQ    400000L
 #endif
 
-// Struct defining a request context for an I2C operation.
-struct I2CRB {
+// Class defining a request context for an I2C operation.
+class I2CRB {
+public:
   volatile uint8_t status; // Completion status, or pending flag (updated from IRC)
   volatile uint8_t nBytes; // Number of bytes read (updated from IRC)
 
+  inline I2CRB() { status = I2C_STATUS_OK; };
   uint8_t wait();
   bool isBusy();
-  inline void init() { status = I2C_STATUS_OK; };
+
   void setReadParams(uint8_t i2cAddress, uint8_t *readBuffer, uint8_t readLen);
   void setRequestParams(uint8_t i2cAddress, uint8_t *readBuffer, uint8_t readLen, const uint8_t *writeBuffer, uint8_t writeLen);
   void setWriteParams(uint8_t i2cAddress, const uint8_t *writeBuffer, uint8_t writeLen);
@@ -213,6 +218,10 @@ public:
   // Loop method
   void loop();
 
+  // Expand error codes into text.  Note that they are in flash so 
+  // need to be printed using FSH.
+  static const FSH *getErrorMessage(uint8_t status);
+
 private:
   bool _beginCompleted = false;
   bool _clockSpeedFixed = false;
@@ -258,7 +267,9 @@ private:
     static void I2C_close();
     
   public:
-    void setTimeout(unsigned long value) { timeout = value;};
+    // setTimeout sets the timout value for I2C transactions.
+    // TODO: Get I2C timeout working before uncommenting the code below.
+    void setTimeout(unsigned long value) { (void)value; /* timeout = value; */ };
 
     // handleInterrupt needs to be public to be called from the ISR function!
     static void handleInterrupt();

I2CManager_Mega4809.h

@@ -105,14 +105,14 @@ void I2CManagerClass::I2C_handleInterrupt() {
     I2C_sendStart();   // Reinitiate request
   } else if (currentStatus & TWI_BUSERR_bm) {
     // Bus error
-    status = I2C_STATUS_BUS_ERROR;
+    state = I2C_STATUS_BUS_ERROR;
     TWI0.MSTATUS = currentStatus; // clear all flags
   } else if (currentStatus & TWI_WIF_bm) {
     // Master write completed
     if (currentStatus & TWI_RXACK_bm) {
       // Nacked, send stop.
       TWI0.MCTRLB = TWI_MCMD_STOP_gc;
-      status = I2C_STATUS_NEGATIVE_ACKNOWLEDGE;
+      state = I2C_STATUS_NEGATIVE_ACKNOWLEDGE;
     } else if (bytesToSend) {
       // Acked, so send next byte
       if (currentRequest->operation == OPERATION_SEND_P)
@@ -126,7 +126,7 @@ void I2CManagerClass::I2C_handleInterrupt() {
     } else {
       // No more data to send/receive. Initiate a STOP condition.
       TWI0.MCTRLB = TWI_MCMD_STOP_gc;
-      status = I2C_STATUS_OK;  // Done
+      state = I2C_STATUS_OK;  // Done
     }
   } else if (currentStatus & TWI_RIF_bm) {
     // Master read completed without errors
@@ -136,7 +136,7 @@ void I2CManagerClass::I2C_handleInterrupt() {
     } else { 
       // Buffer full, issue nack/stop
       TWI0.MCTRLB = TWI_ACKACT_bm | TWI_MCMD_STOP_gc;
-      status = I2C_STATUS_OK;
+      state = I2C_STATUS_OK;
     }
     if (bytesToReceive) {
       // More bytes to receive, issue ack and start another read
@@ -144,7 +144,7 @@ void I2CManagerClass::I2C_handleInterrupt() {
     } else {
       // Transaction finished, issue NACK and STOP.
       TWI0.MCTRLB = TWI_ACKACT_bm | TWI_MCMD_STOP_gc;
-      status = I2C_STATUS_OK;
+      state = I2C_STATUS_OK;
     }
   }
 }

I2CManager_NonBlocking.h

@@ -59,10 +59,9 @@ void I2CManagerClass::_setClock(unsigned long i2cClockSpeed) {
  ***************************************************************************/
 void I2CManagerClass::startTransaction() { 
   ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
-    I2CRB *t = queueHead;
-    if ((state == I2C_STATE_FREE) && (t != NULL)) {
+    if ((state == I2C_STATE_FREE) && (queueHead != NULL)) {
       state = I2C_STATE_ACTIVE;
-      currentRequest = t;
+      currentRequest = queueHead;
       rxCount = txCount = 0;
       // Copy key fields to static data for speed.
       operation = currentRequest->operation;
@@ -85,9 +84,9 @@ void I2CManagerClass::queueRequest(I2CRB *req) {
       queueHead = queueTail = req;  // Only item on queue
     else
       queueTail = queueTail->nextRequest = req; // Add to end
+    startTransaction();
   }
 
-  startTransaction();
 }
 
 /***************************************************************************
@@ -135,7 +134,7 @@ void I2CManagerClass::checkForTimeout() {
   unsigned long currentMicros = micros();
   ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
     I2CRB *t = queueHead;
-    if (state==I2C_STATE_ACTIVE && t!=0 && timeout > 0) {
+    if (state==I2C_STATE_ACTIVE && t!=0 && t==currentRequest && timeout > 0) {
       // Check for timeout
       if (currentMicros - startTime > timeout) { 
         // Excessive time. Dequeue request
@@ -150,7 +149,7 @@ void I2CManagerClass::checkForTimeout() {
         I2C_init();
         state = I2C_STATE_FREE;
         
-        // Initiate next queued request
+        // Initiate next queued request if any.
         startTransaction();
       }
     }
@@ -173,29 +172,31 @@ void I2CManagerClass::loop() {
  ***************************************************************************/
 void I2CManagerClass::handleInterrupt() {
 
+  // Update hardware state machine
   I2C_handleInterrupt();
 
-  // Experimental -- perform the post processing with interrupts enabled.
-  //interrupts();
-
-  if (state!=I2C_STATE_ACTIVE && state != I2C_STATE_FREE) {
+  // Check if current request has completed.  If there's a current request
+  // and state isn't active then state contains the completion status of the request.
+  if (state != I2C_STATE_ACTIVE && currentRequest != NULL) {
     // Remove completed request from head of queue
-    I2CRB * t;
     ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
-      t = queueHead;
-      if (t != NULL) {
+      I2CRB * t = queueHead;
+      if (t == queueHead) {
         queueHead = t->nextRequest;
         if (!queueHead) queueTail = queueHead;
         t->nBytes = rxCount;
         t->status = state;
-      }
+        
         // I2C state machine is now free for next request
+        currentRequest = NULL;
         state = I2C_STATE_FREE;
-    }
+
         // Start next request (if any)
         I2CManager.startTransaction();
       }
     }
+  }
+}
 
 // Fields in I2CManager class specific to Non-blocking implementation.
 I2CRB * volatile I2CManagerClass::queueHead = NULL;

IODevice.cpp

@@ -69,8 +69,10 @@ void IODevice::loop() {
   unsigned long currentMicros = micros();
   // Call every device's loop function in turn, one per entry.
   if (!_nextLoopDevice) _nextLoopDevice = _firstDevice;
+  if (_nextLoopDevice) {
     _nextLoopDevice->_loop(currentMicros);
     _nextLoopDevice = _nextLoopDevice->_nextDevice;
+  }
   
   // Report loop time if diags enabled
 #if defined(DIAG_LOOPTIMES)

IO_GPIOBase.h

@@ -145,7 +145,8 @@ void GPIOBase<T>::_loop(unsigned long currentMicros) {
       _deviceState = DEVSTATE_NORMAL;
     } else {
       _deviceState = DEVSTATE_FAILED;
-      DIAG(F("%S I2C:x%x Error:%d"), _deviceName, _I2CAddress, status);
+      DIAG(F("%S I2C:x%x Error:%d %S"), _deviceName, _I2CAddress, status, 
+        I2CManager.getErrorMessage(status));
     }
     _processCompletion(status);
 
@@ -174,7 +175,7 @@ void GPIOBase<T>::_loop(unsigned long currentMicros) {
     if (digitalRead(_gpioInterruptPin)) return;
   } else
   // No interrupt pin.  Check if tick has elapsed.  If not, finish.
-  if (currentMicros - _lastLoopEntry < _portTickTime) return;
+  if (currentMicros - _lastLoopEntry < (unsigned long)_portTickTime) return;
 
   // TODO: Could suppress reads if there are no pins configured as inputs!
 

IO_HCSR04.h

@@ -102,7 +102,7 @@ protected:
 
   // _loop function - read HC-SR04 once every 50 milliseconds.
   void _loop(unsigned long currentMicros) override {
-    if (currentMicros - _lastExecutionTime > 50000) {
+    if (currentMicros - _lastExecutionTime > 50000UL) {
       _lastExecutionTime = currentMicros;
 
       _value = read_HCSR04device();

LiquidCrystal_I2C.cpp

@@ -59,8 +59,6 @@ LiquidCrystal_I2C::LiquidCrystal_I2C(uint8_t lcd_Addr, uint8_t lcd_cols,
     backlight();
     lcdDisplay = this;
   }
-  // Initialise request block for comms.
-  requestBlock.setWriteParams(lcd_Addr, outputBuffer, sizeof(outputBuffer));
 }
 
 void LiquidCrystal_I2C::begin() {
@@ -192,8 +190,6 @@ void LiquidCrystal_I2C::send(uint8_t value, uint8_t mode) {
   mode |= _backlightval;
   uint8_t highnib = (((value >> 4) & 0x0f) << BACKPACK_DATA_BITS) | mode;
   uint8_t lownib = ((value & 0x0f) << BACKPACK_DATA_BITS) | mode;
-  // Wait for previous request to complete before writing to outputbuffer.
-  requestBlock.wait();
   // Send both nibbles
   uint8_t len = 0;
   outputBuffer[len++] = highnib|En;
@@ -209,8 +205,6 @@ void LiquidCrystal_I2C::write4bits(uint8_t value) {
   // Enable must be set/reset for at least 450ns.  This is well within the
   // I2C clock cycle time of 2.5us at 400kHz. Data is clocked in to the
   // HD44780 on the trailing edge of the Enable pin.
-  // Wait for previous request to complete before writing to outputbuffer.
-  requestBlock.wait();
   uint8_t len = 0;
   outputBuffer[len++] = _data|En;
   outputBuffer[len++] = _data;
@@ -220,8 +214,6 @@ void LiquidCrystal_I2C::write4bits(uint8_t value) {
 // write a byte to the PCF8574 I2C interface.  We don't need to set
 // the enable pin for this.
 void LiquidCrystal_I2C::expanderWrite(uint8_t value) {
-  // Wait for previous request to complete before writing to outputbuffer.
-  requestBlock.wait();
   outputBuffer[0] = value | _backlightval;
   I2CManager.write(_Addr, outputBuffer, 1);  // Write command synchronously
 }

LiquidCrystal_I2C.h

@@ -75,10 +75,8 @@ public:
   void backlight();
   
   void command(uint8_t);
-  void init();
 
 private:
-  void init_priv();
   void send(uint8_t, uint8_t);
   void write4bits(uint8_t);
   void expanderWrite(uint8_t);
@@ -88,10 +86,9 @@ private:
   uint8_t _displaymode;
   uint8_t _backlightval;
 
-  I2CRB requestBlock;
   uint8_t outputBuffer[4];
   // I/O is synchronous, so if this is called we're not busy!
-  bool isBusy() { return false; } 
+  bool isBusy() override { return false; } 
 };
 
 #endif

SSD1306Ascii.cpp

@@ -99,9 +99,6 @@ SSD1306AsciiWire::SSD1306AsciiWire(int width, int height) {
   lcdRows = height / 8;
   lcdCols = width / 6;
 
-  // Initialise request block for I2C
-  requestBlock.init();
-
   I2CManager.begin();
   I2CManager.setClock(400000L);  // Set max supported I2C speed
   for (byte address = 0x3c; address <= 0x3d; address++) {

Turnouts.cpp

@@ -107,7 +107,7 @@
 
   bool Turnout::setClosedStateOnly(uint16_t id, bool close) {
     Turnout *tt = get(id);
-    if (tt) return false;
+    if (!tt) return false;
     tt->_turnoutData.closed = close;
     return true;
   }