Expert Comment Needed on a New I2C Library API

I am writing a new library for the AVR Two Wire Interface. The Wire library is not very RTOS friendly.

The library is for master mode. I plan to write a separate slave mode library later.

This new library is designed to be used with various RTOSs and the calling thread will sleep while the I2C transfer occurs.

Is this API adequate for slave devices that may be used with Arduino?

The class API has three functions (detailed documentation follows later):

Public Member Functions

void begin (bool speed=I2C_400KHZ, bool pullups=I2C_NO_PULLUPS);
bool transfer (uint8_t addrRW, void *buf, size_t nbytes, uint8_t option=I2C_STOP);
bool transferContinue (void *buf, size_t nbytes, uint8_t option=I2C_STOP);

Here is an example for a DS1307 RTC chip that reads the DS1307 registers and prints the data and time.

#include <NilTwi.h>

// Two Wire Interface instance.
NilTwiMaster twi;

// DS1307 I2C address in high bits, R/W in low bit.
const uint8_t DS1307ADDR = 0XD0;
// DS1307 address and read bit.
const uint8_t RTC_READ = DS1307ADDR | I2C_READ;
// DS1307 address and write bit
const uint8_t RTC_WRITE = DS1307ADDR | I2C_WRITE;
//------------------------------------------------------------------------------
void setup() {
  Serial.begin(9600);
  
  // Use standard 100 kHz speed and no internal pullups.
  twi.begin(I2C_100KHZ);
}
//------------------------------------------------------------------------------
void loop() {
  uint8_t add = 0;   // DS1307 time/date register address.
  uint8_t buf[7];    // Buffer for DS1307 register values.
  
  // Write DS1307 register address.  
  // Demo of option argument, repeat start is not required here.
  twi.transfer(RTC_WRITE, &add, 1, I2C_REP_START);
  
  // Read DS1307 time/date registers.
  twi.transfer(RTC_READ, buf, 7);

  // Print YYYY-MM-DD hh:mm:ss
  Serial.print("20");
  for (int i = 6; i >= 0; i--){
    // Skip day of week.
    if (i == 3) continue;
    
    // Always print two digits
    if (buf[i] < 10) Serial.print('0');
    
    // Ds1307 is BCD.
    Serial.print(buf[i], HEX);
    
    // Print field separator.
    if (i == 6 || i == 5) Serial.print('-');
    if (i == 4) Serial.print(' ');
    if (i == 2 || i == 1) Serial.print(':');

  }
  Serial.println();
  delay(1000);
}

Here is more detailed documentation for the class members.

void TwiMaster::begin(bool speed = I2C_400KHZ, bool pullups = I2C_NO_PULLUPS)

Initialize the AVR 2-wire Serial Interface.

Parameters:
[in] speed I2C bus speed, one of:

I2C_400KHZ
I2C_100KHZ

[in] pullups State of internal AVR pullups, one of:

I2C_NO_PULLUPS
I2C_PULLUPS

bool TwiMaster::transfer(uint8_t addrRW, void* buf,
size_t nbytes, uint8_t option = I2C_STOP)

Start an I2C transfer with possible continuation.

Parameters:
[in] addrRW I2C slave address plus R/W bit.
[in,out] buf Source or destination data address for transfer.
[in] nbytes Number of bytes to transfer (may be zero).
[in] option Option for ending the transfer, one of:

I2C_STOP end the transfer with an I2C stop condition.
I2C_REP_START end the transfer with an I2C repeat start condition.
I2C_CONTINUE allow additional transferContinue() calls.

Returns:
true for success else false.

bool TwiMaster::transferContinue(void* buf, size_t nbytes,
uint8_t option = I2C_STOP)

Continue an I2C transfer.

Parameters:
[in,out] buf Source or destination data address for transfer.
[in] nbytes Number of bytes to transfer (may be zero).
[in] option Option for ending the transfer, one of:

I2C_STOP end the transfer with an I2C stop condition.
I2C_REP_START end the transfer with an I2C repeat start condition.
I2C_CONTINUE allow additional transferContinue() calls.

Returns:
true for success else false.

bool transfer(...)

The bool indicates success or failure.

Is there a more detailed error available?
In the current implementation there are a few (lowlevel) error codes that indicate why communication failed. I find them helpful for debugging.
/*

  • Function twi_writeTo
    ...
  • Output 0 .. success
  • 1 .. length to long for buffer
  • 2 .. address send, NACK received
  • 3 .. data send, NACK received
  • 4 .. other twi error (lost bus arbitration, bus error, ..)
    */

idea:
to solve address issues it might be useful to have a I2C scanner equivalent in the core of the new lib? (master only?)

// both return -1 if none found
int TwiMaster::scanGetFirst();  
int TwiMaster::scanGetNext(); 

// bit like this
int addr = twi.scanGetFirst();
while (addr > -1)
{
  Serial.println(addr);
  addr = twi.scanGetNext();
}

Some getter functions? have no direct use for them, but makes it complete
bool getPullUps();
int getSPeed();

Question pops up?
How would the new lib communicate with an I2C EEPROM? (esp eeprom address handling, page boundaries ...)

Let me try to handle some of your comments.

1 .. length to long for buffer

This can't happen since I don't copy the data to/from an internal buffer. I just setup the address and count in static variables and the ISR moves data directly to/from the user's buffer.

  • 2 .. address send, NACK received
  • 3 .. data send, NACK received
  • 4 .. other twi error (lost bus arbitration, bus error, ..)

The ISR saves the TWI state on entrance so the you can get the initial and current state of the bus. Here are the possible codes:

// General TWI Master state codes
#define TWI_START 0x08 // START has been transmitted
#define TWI_REP_START 0x10 // Repeated START has been transmitted
#define TWI_ARB_LOST 0x38 // Arbitration lost

// TWI Master Transmitter state codes
#define TWI_MTX_ADR_ACK 0x18 // SLA+W has been transmitted and ACK received
#define TWI_MTX_ADR_NACK 0x20 // SLA+W has been transmitted and NACK received
#define TWI_MTX_DATA_ACK 0x28 // Data byte has been transmitted and ACK received
#define TWI_MTX_DATA_NACK 0x30 // Data byte has been transmitted and NACK received

// TWI Master Receiver state codes
#define TWI_MRX_ADR_ACK 0x40 // SLA+R has been transmitted and ACK received
#define TWI_MRX_ADR_NACK 0x48 // SLA+R has been transmitted and NACK received
#define TWI_MRX_DATA_ACK 0x50 // Data byte has been received and ACK transmitted
#define TWI_MRX_DATA_NACK 0x58 // Data byte has been received and NACK transmitted

// TWI Miscellaneous state codes
#define TWI_NO_STATE 0xF8 // No relevant state information available; TWINT = “0”
#define TWI_BUS_ERROR 0x00 // Bus error due to an illegal START or STOP condition

For example if you call transferContinue() and it fails you can check the bus state.

idea:
to solve address issues it might be useful to have a I2C scanner equivalent in the core of the new lib? (master only?)

I am not sure exactly what you mean but the main reason that transfer() will allows zero length is to probe the bus.

On my DS1307 test setup this sketch:

#include <NilTwi.h>

// Two Wire Interface instance.
NilTwiMaster twi;

void setup() {
  Serial.begin(9600);
  twi.begin();

  uint8_t add = 0;

  // try read
  do {
    if (twi.transfer(add | I2C_READ, 0, 0)) {
      Serial.print("Add read: 0X");
      Serial.println(add, HEX);
    }
    add += 2;
  } 
  while (add);

  // try write
  add = 0;
  do {
    if (twi.transfer(add | I2C_WRITE, 0, 0)) {
      Serial.print("Add write: 0X");
      Serial.println(add, HEX);
    }
    add += 2;
  } 
  while (add);
  Serial.println("Done");  
}
void loop() {
}

Prints this:

Add read: 0XD0
Add write: 0XD0
Done

Some getter functions? have no direct use for them, but makes it complete
bool getPullUps();
int getSPeed();

Unfortunately I set the pull-ups and baud rate in begin() but don't save it. I guess I could.

Question pops up?
How would the new lib communicate with an I2C EEPROM? (esp eeprom address handling, page boundaries ...)

The user has total control of the transfer size. I don't have internal buffers.

You could send all day without a stop or repeat start by calling transferContinue(). There might be a time gap.

I didn't demo transferContinue() in my example. Here is a DS1307 write function:

  bool rtcWrite(uint8_t regAdd, uint8_t* buf, uint8_t count) {
  // Set place to write in DS1307 and don't end transfer.
  if (!twi.transfer(RTC_WRITE, &regAdd, 1, I2C_CONTINUE)) return false;

  // Write DS1307 registers or memory.  Default is to end with a stop condition.
  return twi.transferContinue(buf, count);
}

On getter functions.

I could read the baud rate register to get the speed.

I think the pull-ups could be determined by reading the PORT register.

I already have the state functions and just need to include them in the class wrapper.

Do we need to mess with internal pullups? Those pullups are off the recommended values for i2c, so you need external resistors anyway. And APIs - do you consider to have the same as we have today or shall we ie. rewrite sensors drivers with the new i2c lib?

pito,

I provide the option of internal pull-ups because of this statement in the AVR datasheet.

Note that the internal pull-ups in the AVR pads can be enabled by setting the PORT bits corresponding to the SCL and SDA pins, as explained in the I/O Port section. The internal pull-ups can in some systems eliminate the need for external ones.

I have run some I2C chips, like a RTC, with only internal pull-ups in past projects with no problems.

I didn't use the Wire style API since I don't want an internal buffer and limited transfer sizes for I2C master. I am designing for maximum performance with RTOSs.

In my first tests at 100kHz with a DS1307, Wire takes 1084 microseconds to read the seven date time registers. This library, in a NilRTOS thread, takes 1008 microseconds but the thread sleeps while the ISR does the read and lower priority threads get about 850 microseconds of extra CPU time.

At 400 kHz, the read takes 324 microseconds and lower priority threads get an extra 180 microseconds of CPU time.

I may investigate a Wire style wrapper class with an internal buffer but I don't know if that would be valuable.

The internal pull-ups can in some systems eliminate the need for external ones.

The arduino ecosystem shall be designed robust by default, imho. I would recommend to use the i2c pullups with lower resistor values, ie 2k2-4k7. Most of the users do not possess techniques for debugging such potential hw-related issues.

APIs - so your message is the existing drivers utilizing old wire lib have to be rewritten for the new lib.

The arduino ecosystem shall be designed robust by default, imho. I would recommend to use the i2c pullups with lower resistor values, ie 2k2-4k7. Most of the users do not possess techniques for debugging such potential hw-related issues.

I agree so the default is no internal pull-ups. I will add a warning to the documentation for begin().

Simple versions of begin(), like these, do not enable pull-ups.

  twi.begin();  // 100 kHz, no pull-ups
  twi.begin(I2C_100KHZ);  // 100 kHz, no pull-ups
  twi.begin(I2C_400KHZ);  // 400 kHz, no pull-ups

APIs - so your message is the existing drivers utilizing old wire lib have to be rewritten for the new lib.

To repeat, a wrapper to present the Wire API is easy to write. This may make it easy to use existing drivers by changing the include file from Wire.h to the wrapper include. I don't want the standard API to be the Wire API because of Wire's limited internal buffering.

robtillaart,

I like the idea of providing more diagnostic information. I included functions to get more info and in addition I wrote a "printInfo" routine.

I am not sure how helpful the messages are. Any suggestions?

The program prints these five items.

Last return status: <return status for last request, Success or Failure>

Transfer request size:

Bytes transfered:

Previous State:

Current State:

Here are some example messages:

After transfer(addRW, buf, 7) when no slave responds to a read request:

Last return status: Failure
Transfer request size: 7
Bytes transfered: 0
Previous State: 0X48, SLA+R has been transmitted and NACK received.
Curent State: 0X48, SLA+R has been transmitted and NACK received.

A transferContinue() call when the previous call ended with a stop condition:

Last return status: Failure
Transfer request size: 7
Bytes transfered: 0
Previous State: 0XF8, Stop condition.
Curent State: 0XF8, Stop condition.

A transferContinue() call when the previous call ended with a repeated start:

Last return status: Failure
Transfer request size: 7
Bytes transfered: 0
Previous State: 0X10, A Repeated START has been transmitted.
Curent State: 0X10, A Repeated START has been transmitted.

Here are example messages after successful transfer calls:

Last return status: Success
Transfer request size: 1
Bytes transfered: 1
Previous State: 0X28, Data byte has been transmitted and ACK received.
Curent State: 0X10, A Repeated START has been transmitted.

Last return status: Success
Transfer request size: 7
Bytes transfered: 7
Previous State: 0X58, Data byte has been received and NACK transmitted.
Curent State: 0XF8, Stop condition.

pito,

The pull-up issue is bothering me and now I am not sure what to do.

I tried not enabling internal pull-ups on a bare Uno and it hangs on an start command.

The reason is that SDA and SCl never go high so the controller thinks another master has the bus.

The Wire library avoids this by always enabling internal pull-ups. This means hardware without external pull-ups sort of works with Wire. Unless the bus has very low capacitance it will be flaky.

I think I will read the SDA and SCL pins in begin() and return an error if they are not high. So if there are no external pull-ups and internal pull-ups are not enabled, begin() will likely return a failure.

Now I need to decide if I should even allow internal pull-ups to be enabled. Internal pull-ups only work on systems with very low bus capacitance.

I am leaning toward your view, any good design has external pull-ups.

Bill, the i2c's sda and sck work as open drain/collectors (!), where they need pullups. The atmega's internal pullups are 20k+ (it could be up to ~50k). Recommended values are 2k2-4k7 for 100kHz, with lower resistances @400kHz. With longer wires, lot of stray capacitance, lot of devices (ie. I have 5 slaves just now) you need to be careful with pullups values.
Below you may see signal shapes on the i2c bus with 2k2, 4k7 and 20k(internal pullup) @400kHz speed and 100pF bus capacitance..

pito,

I understand how the bus works and how to size pull-ups. The I2C standard covers that.

I looked at several cases on my scope today. For an MCP3422 ADC at 100 kHz with 4 pf pin capacitance on a small board that plugs into the I2C pins on an Uno, the signals are O.K. with internal pull-ups.

For a 400 kHz system with many devices and lots of capacitance, internal pull-ups don't work.

Now I am trying to decide how to cope with users who won't know what's happening if I don't enable internal pull-ups.

If I don't enable the internal pull-ups, the first I2C start command hangs if there are no external pull-ups.

The Wire library enables pull-ups to avoid this problem but will be flaky if there are no external pull-ups.

I think I won't allow enabling of internal pullups but if I do this I need to return a failure to the user if there are no external pull-ups.

I have been testing and it appears I can detect the lack of pullups by doing a digitalRead of SDA and SCL before enabling the TWI controller. For devices I have tested, SDA and SCL read low if there are no external pull-ups.

So I will have begin() return a fail status if there are no external pull-ups.

Wire runs at 100 kHz and is more tolerant. For my default 400 kHz, external pull-ups are the only way to go.

You can enable internal pull-ups ... that doesn't prevent the user from also using external pull-ups. A comment with a short table of recommendations would be useful. Something like "External pull-ups are recommended unless the bus length is very short. Recommended values are: 10KHz ..."

David
Victoria, BC

pito:
Do we need to mess with internal pullups? Those pullups are off the recommended values for i2c, so you need external resistors anyway.

Yes, always enable internal pullups - that is the default setting (but no guarantee it will work because the internal pullups are huge in value). You need to (you must) add external one to comply with i2c recommendation. That must be written somewhere..
So you must not deal/mess with internal pullups within your APIs ("enabled" or "disabled" - it makes no sense to deal with in practice, imho). See for example (the internal pullup is 20k, external pullup resistor 2k2 for 400kHz speed):

Int. pullup enabled: 20k || 2k2 = 1.99kohm 
Int. pullup disabled: ~inf || 2k2 = 2.2kohm

I vote for not enabling internal pull-ups as the default. This will provide a hard failure if there are no external pull-ups.

My printInfo() function will indicate a pull-up problem like this:

Last return status: Failure
Failure state: 0XD0, Start condition timeout. Pull-up problem?
Transfer request size: 1
Bytes transfered: 0
Curent state: 0XF8, Stop condition or busy; TWINT == 0.

The user can then decide whether to try enabling pull-ups with a begin() call like this:

  twi.begin(I2C_100KHZ, I2C_INTERNAL_PULLUPS);

Hopefully that will cause the user to read the description of begin() and the warnings about pull-ups.

The printInfo() function provides a way to diagnose problems. It takes about 1200 bytes of flash program space when you use it. It has a lot of text stored in flash. There is no overhead if you don't use it.

For a board with external pull-ups, the these calls:

  twi.transfer(RTC_WRITE, &rtcAdd, 1, I2C_REP_START);
  twi.printInfo(&Serial);

Result in this message:

Last return status: Success
Transfer request size: 1
Bytes transfered: 1
Curent state: 0X10, A Repeated START has been transmitted.

I completed the "Wire style wrapper". The wrapper is defined in WireMaster.h.

Here is an example that works with Wire or the new library. All you need to do is change the include files at the top of the sketch.

The new Wire wrapper library is RTOS friendly with the calling thread sleeping while the transfer takes place.

//#include <Wire.h>
#include <NilTwi.h>
#include <WireMaster.h>

// Wire style address for DS1307
const int DS1307ADDR = (0XD0 >>1);
//------------------------------------------------------------------------------
void setup() {
  Serial.begin(9600);
  Wire.begin();
}
//------------------------------------------------------------------------------
void loop() {
  int rtn;
  uint8_t add = 0;   // DS1307 time/date register address.
  uint8_t buf[7];    // Buffer for DS1307 register values.

  // Write DS1307 time/date rgister address.
  Wire.beginTransmission(DS1307ADDR);
  Wire.write(add);
  if (rtn = Wire.endTransmission()) {
    Serial.print("write error: ");
    Serial.println(rtn);
  }
  // Read DS1307 time/date registers.
  if (Wire.requestFrom(DS1307ADDR, 7) != 7) {
    Serial.println("read error");
  }
  Wire.readBytes((char*)buf, 7);
  
  // Print YYYY-MM-DD hh:mm:ss
  Serial.print("20");
  for (int i = 6; i >= 0; i--){
    // Skip day of week.
    if (i == 3) continue;

    // Always print field with two digits.
    if (buf[i] < 10) Serial.print('0');

    // Ds1307 is BCD.
    Serial.print(buf[i], HEX);

    // Print field separator.
    if (i == 6 || i == 5) Serial.print('-');
    if (i == 4) Serial.print(' ');
    if (i == 2 || i == 1) Serial.print(':');
  }
  Serial.println();
  delay(1000);
}

Here is the same program with the new API.

#include <NilTwi.h>

// Two Wire Interface instance.
NilTwiMaster twi;

// DS1307 I2C address in high bits, R/W in low bit.
const uint8_t DS1307ADDR = 0XD0;
// DS1307 address and read bit.
const uint8_t RTC_READ = DS1307ADDR | I2C_READ;
// DS1307 address and write bit
const uint8_t RTC_WRITE = DS1307ADDR | I2C_WRITE;
//------------------------------------------------------------------------------
void setup() {
  Serial.begin(9600);
  
  // Use standard 100 kHz speed and no internal pullups.
  twi.begin(I2C_100KHZ);
}
//------------------------------------------------------------------------------
void loop() {
  uint8_t add = 0;   // DS1307 time/date register address.
  uint8_t buf[7];    // Buffer for DS1307 register values.
  
  // Write DS1307 time/date rgister address.
  if (!twi.transfer(RTC_WRITE, &add, 1)) {
    twi.printInfo(&Serial);
  }

  // Read DS1307 time/date registers.
  if twi.transfer(RTC_READ, buf, 7) {
    twi.printInfo(&Serial);
  }

  // Print YYYY-MM-DD hh:mm:ss
  Serial.print("20");
  for (int i = 6; i >= 0; i--){
    // Skip day of week.
    if (i == 3) continue;
    
    // Always print field with two digits.
    if (buf[i] < 10) Serial.print('0');
    
    // Ds1307 is BCD.
    Serial.print(buf[i], HEX);
    
    // Print field separator.
    if (i == 6 || i == 5) Serial.print('-');
    if (i == 4) Serial.print(' ');
    if (i == 2 || i == 1) Serial.print(':');

  }
  Serial.println();
  delay(1000);
}

Great, I may test it with my BMP085 nilSdLogger.

pito,

What software/libraries are you using with the BMP085? I don't have a BMP085 but I downloaded SparkFun and Adafruit software and libraries and they compile by only changing the include.

#include <Wire.h>

to

#include <NilTwi.h>

I am testing, cleaning up code, writing documentation, and writing examples.

I did a test reading the seven date/time registers from a DS1307. At 100 kHz I2C speed this takes about 1024 microseconds, Wire takes a bit longer.

I measured how much of the 1024 microseconds is recovered by sleeping during the I2C transfer. The result is 84% of the 1025 microseconds is saved.

At 400 kHz the read takes 324 microseconds and 56% of the 324 microseconds is saved for other threads.

I didn't try Wire since it doesn't have a 400 kHz option.