I2C/TWI Library: Buffer Overflow

[neophob]

Tested on Arduino v1.0.3, Using Arduino Duemillanove/Seeeduino

I did some I2c testing between RPI and Arduino (http://neophob.com/2013/04/i2c-communication-between-a-rpi-and-a-arduino/) and observed that when a I2C master (RPI) sends data to an Arduino I2C slave device that is larger than the default buffer (32 bytes) the Arduino just freeze. After sending a i2c packet larger than the default buffer a i2cdetect scan shows no more devices on the bus.
The Arduino needs a reset after this event.

[neophob]

I just checked the source and I guess the error is in the file libraries/Wire/Wire.cpp:

// copy twi rx buffer into local read buffer
// this enables new reads to happen in parallel
for(uint8_t i = 0; i < numBytes; ++i){
rxBuffer[i] = inBytes[i];
}

I guess it should be
// copy twi rx buffer into local read buffer
// this enables new reads to happen in parallel
for(uint8_t i = 0; i < numBytes && i < BUFFER_LENGTH; ++i){
rxBuffer[i] = inBytes[i];
}

I'll test that tomorrow.

[neophob]

Nope, this didn't help. I also checked the twi.c file but didn't spot the error.

any hints about this?

[matthijskooijman]

To me it looks like the code you mentioned is indeed susceptible to overflow. However, the twi.c file also keeps a buffer, so if those buffers are the same length, I guess this code shouldn't trigger a problem, but it's still good to fix it. I would however fix it by putting the following two lines before the loop, instead of the change you proposed:

  if (numBytes > BUFFER_LENGTH)
      numBytes = BUFFER_LENGTH;

This makes sure rxBufferLength gets set to the proper (truncated) value.

However, twi.c also performs correct buffer checks, and both buffer sizes are equal, so it is unlikely this is the cause for your problem. Looking at twi.c, I see the following code in the interrupt handler:

case TW_SR_DATA_ACK: // data received, returned ack
case TW_SR_GCALL_DATA_ACK: // data received generally, returned ack
    // if there is still room in the rx buffer
    if(twi_rxBufferIndex < TWI_BUFFER_LENGTH){
        // put byte in buffer and ack
        twi_rxBuffer[twi_rxBufferIndex++] = TWDR;
        twi_reply(1);
    }else{
        // otherwise nack
        twi_reply(0);
    }
    break;

This means that the code sends a NAK when the received data would overflow the buffer. Perhaps this somehow messes up the state of the i2c hardware causing a deadlock? You'd have to check the datasheet to be sure, I don't know how the i2c hardware is supposed to work.

[neophob]

Thanks for your reply. Well I also added a check like:

if(BUFFER_LENGTH < numBytes){
  return;
}

However this didn't resolve the issue. As there are 4 other buffers used for the i2c data, the error must be somewhere else.

About the i2c reply, someone knows more about this? Maybe this helps: http://processors.wiki.ti.com/index.php/I2C_Tips#Detecting_and_handling_NACK

Edit:
Maybe there is no buffer overflow at all but the code wait in a wait loop. there are other reports about this issue, see
http://arduino.cc/forum/index.php/topic,19624.0.html
https://github.com/steamfire/WSWireLib

[matthijskooijman]

I just had a look at your blogpost, seems like your Arduino sketch isn't doing anything else except reading i2c data. You are saying the "arduino just freeze", but are you sure the Arduino is really frozen, or is just the i2c hardware no longer receiving. You can check by adding a serial print in the loop, for example.

It seems more likely to me that the i2c hardware is somehow ending up in an undefined state. I wonder if the problem might be with the rpi instead of the Arduino. Perhaps the rpi is not properly freeing the bus (sending a STOP signal) after receiving the ack from the Arduino? This is just a guess, though.

[neophob]

what I did was a "i2cdetect -y 1" on the RPI. after sending more bytes than the i2c buffer on the arduino can h andle the arduino was not longer visible via i2c.

yes maybe just the i2c is in a fancy state. I'll try to add serial debug output and see if the arduino itself is just alive. thanks for the idea. I think this would relate to the other links I posted. It looks like the arduino is waiting infinitely in a while loop.

http://arduino.cc/forum/index.php/topic,19624.0.html:
Investigating, the problem is <twi.c> blocking behavior without timeouts. (endless loops "while")

 while(TWI_READY != twi_state){

if this is the case, the serial output would also stop and the arduino would be blocked.

[matthijskooijman]

Looking at the code and the datasheet, I think that the NAK condition is
not handled properly. I haven't figured it all out yet, but could you
try this patch I quickly whipped up?

diff --git a/libraries/Wire/utility/twi.c b/libraries/Wire/utility/twi.c
index 6b2db3c..4dfe8f1 100644
--- a/libraries/Wire/utility/twi.c
+++ b/libraries/Wire/utility/twi.c
@@ -475,10 +475,16 @@ SIGNAL(TWI_vect)
       break;
     case TW_SR_DATA_NACK:       // data received, returned nack
     case TW_SR_GCALL_DATA_NACK: // data received generally, returned nack
-      // nack back at master
-      twi_reply(0);
+      // We replied a nack, this happens when the buffer overflows.
+      // After this interrupt we don't get a TW_SR_STOP interrupt, so
+      // clean up now
+
+      // Discard the data, it is incomplete
+      twi_rxBufferIndex = 0;
+      // ack future responses and leave slave receiver state
+      twi_releaseBus();
       break;
-    
+
     // Slave Transmitter
     case TW_ST_SLA_ACK:          // addressed, returned ack
     case TW_ST_ARB_LOST_SLA_ACK: // arbitration lost, returned ack

Not sure if this is a proper fix, but it could work.

(PS: Better not to edit your comments after submitting them, since edits
won't get emailed)

[neophob]

Bingo! Thanks for that, this fixed the issue. I created a pull request (#1363).

I sended 8 times 32 bytes: everything ok. then
I sended 8 time 512 bytes: fail after first data packet. then
I sended 8 times 32 bytes: everything ok.

Thanks again for your help!

[matthijskooijman]

One more thing I noticed in the code: It seems that the twi library is one byte "late" with signalling a NAK. If I'm right about this sending a 33 byte message would cause 32 bytes to be received without error, with the last byte being silently dropped (I suspect this happens with and without my patch above). Could you see if this is indeed the case?

[matthijskooijman]

I just took some time to hook up my own rpi to my Arduino and managed to reproduce your problem here. I also tested writing 33 bytes and as I expected that works, while silently dropping the 33rd byte. I'll see if I can improve the code a bit now I have a test environment.

For future reference, here is the C test program I'm using on the rpi.

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <linux/i2c-dev.h>
#include <sys/ioctl.h>
#include <fcntl.h>

int main(int argc, char **argv) {
    int file;
    char *filename = "/dev/i2c-1";
    int count = (argc > 1 ? atoi(argv[1]) : 1);
    int retval;

    if ((file = open(filename, O_RDWR)) < 0) {
        perror("Failed to open the i2c bus");
        exit(1);
    }

    int addr = 4;          // The I2C address to talk to
    if (ioctl(file, I2C_SLAVE, addr) < 0) {
        printf("Failed to acquire bus access and/or talk to slave.\n");
        exit(1);
    }

    char *buf = calloc(count, 1);

    retval = write(file,buf,count);
    printf("Wrote %d bytes\n", retval);

    if (retval != count) {
        printf("Not all bytes written: ", strerror(errno));
        printf("\n\n");
    }
}

/* set vim:sw=4 sts=4: */

[mofux]

Hi, I am chasing this bug for days now, so glad I found this post. Unfortunately my arduino uno is still crashing, even if I send less than 32 bytes using the test program above :( My arduino is directly wired to the rpi, no extra pull-up resistors (wired like here: http://neophob.com/2013/04/i2c-communication-between-a-rpi-and-a-arduino/).

The arduino is running the following test code (compiled with the bugfix introduced here) :

#include <Wire.h>

void setup() {
  Wire.onReceive(onReceive);
  Wire.begin(4);
  Serial.begin(9600);
}

void loop() {
}

void onReceive(int numBytes) {
  while (Wire.available()) {
    Serial.println(Wire.read());
  }
}

With this code I can repeatedly write 21 bytes without crashing the arduino uno. Any more bytes will always crash it (arduino appears hanging, i2cdetect fails to detect any devices, test programm reports "Wrote 22 bytes" but on the next attempt writes "Wrote -1 bytes"). Interesting for me: Introducing a Serial.println("LOOP") inside the loop will crash it immediately on the first byte. I have tested this with multiple Arduino Unos and they all behave the same.

I am not sure if this problem is related to my setup/code or a flaw in the Wire library - I'm a bloody beginner I have to admit.

Hope someone can guide me into the right direction...

[matthijskooijman]

@mofux, you're not seeing a problem inside the i2c library, but in the Serial library (and/or a side effect of the way you wrote your test program). I you use the test program from the article at neophob.com), things will work as expected.

What happens is that onReceive runs from the interrupt servicing routines, with interrupts disabled. When you write to Serial, its buffer fills up. Once the buffer is full, Serial.println() will wait for some space to appear in the buffer, but this never happens (since the "byte sent" interrupt from the serial hardware never triggers). The limit of 21 bytes comes from the Serial buffer size of 64 bytes (I suspect you're sending one digit and a CRLF, so three bytes through Serial per received byte through I2C, and 21 * 3 = 63, but 22 * 3 = 66 == buffer full).

See #672 which describes this particular issue in detail and offers some suggestions for fixes that could be implemented.

[mofux]

I just replaced the Serial.println(...) by Serial.print(...) and bam, no more crashes. Thanks for pointing this out and for the detailed explanation. You really made my day.

[sandeepmistry]

@mofux @matthijskooijman are there any action items or can we close this issue?

[seebs]

I am not entirely sure, but so far as I can tell, the patch in question (1) is actually needed, (2) is not present in arduino 1.8.1's copy of that library yet.