Strange code in I2C code…

October 14, 2011 | Arduino | By: Mark VandeWettering

I noticed something while reading code that uses the Wire library for the Arduino, such as you might find below…

Arduino playground – I2CEEPROM

Check out this two line fragment from the read function:

    for ( c = 0; c < length; c++ )
      if (Wire.available()) buffer[c] = Wire.receive();

Is anyone else struck by the fact that it is, well, just wrong? The I2C device is expected to be sending "length" bytes. This loop checks to see if a byte is available, and if it is, then sticks it in the buffer. But what happens if it is not? Then the index increments, and when we try again, any particular byte we write will be placed at a strange location in the buffer.

The only way this code makes sense is if Wire.available() never fails. If it does, then it will leave an empty slot in the output array.

I'll have to step through the Wire library source to make sure, but this seems wrong to me. I'll let you know what I find.

Comments

Comment from madscifi
Time 10/14/2011 at 10:10 am

I don’t think it is nearly as bad as it appears. Take a look at requestFrom() – essentially requestFrom() fills the receive buffer and the chunk of code you refer to empties the receive buffer into the user buffer. All of the bytes that are going to be placed in the receive buffer are in the receive buffer by the time requestFrom() returns. Inefficient in the case that all of the requested bytes are not received, but otherwise it works.

Comment from Mark VandeWettering
Time 10/14/2011 at 10:13 am

If requestFrom() fills the buffer, then why bother checking with isAvailable()? Just read the bytes out.

Comment from madscifi
Time 10/14/2011 at 11:48 am

Absolutely. I’m not defending the code fragment as optimal or well written, only as functioning correctly. Based on looking only at the fragment above I assumed the problem you identified was actually an issue. I assumed, as I think you did as well, that the bytes where being received asynchronously while the for loop was executing. I was surprised to discover that the code actually works given the environment in which it is executed. It is inefficient, but it is functionally correct.

I’d be much more concerned about the fact that i2c_eeprom_read_buffer does not handle errors nor does it provide any indication of success/failure/length. Consequently, there is no way to determine if the destination buffer contains data read from the eeprom or not.