Pages: [1] 2   Go Down
Author Topic: Expert Comment Needed on a New I2C Library API  (Read 3635 times)
0 Members and 1 Guest are viewing this topic.
0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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):
Quote
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.
Code:
#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.
Quote
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.
Logged

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13471
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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, ..)
 */

Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13471
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Code:
// 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();
}
« Last Edit: February 09, 2013, 04:49:53 pm by robtillaart » Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13471
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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 ...)
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Let me try to handle some of your comments.
Quote
    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.

Quote
*          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:
Quote
// 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.

Quote
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:
Code:
#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:
Quote
Add read: 0XD0
Add write: 0XD0
Done

Quote
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.
Quote
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:
Code:
  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);
}
Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Rapa Nui
Offline Offline
Edison Member
*
Karma: 60
Posts: 2061
Pukao hats cleaning services
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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?
« Last Edit: February 10, 2013, 03:59:39 am by pito » Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

pito,

I provide the option of internal pull-ups because of this statement in the AVR datasheet.
Quote
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.
Logged

Rapa Nui
Offline Offline
Edison Member
*
Karma: 60
Posts: 2061
Pukao hats cleaning services
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
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.
« Last Edit: February 10, 2013, 08:54:32 am by pito » Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
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.
Code:
  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


Quote
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.
Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:  <Number of bytes requested in last transfer>

Bytes transfered: <Number of bytes actually transferred in the last request>

Previous State: <Last state that causes an interrupt>

Current State:  <Current state of the controller>

Here are some example messages:

After transfer(addRW, buf, 7) when no slave responds to a read request:
Quote
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:
Quote
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:
Quote
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:
Quote
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.
Quote
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.
Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Rapa Nui
Offline Offline
Edison Member
*
Karma: 60
Posts: 2061
Pukao hats cleaning services
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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..


* oc1.jpg (54.84 KB, 770x652 - viewed 34 times.)
« Last Edit: February 10, 2013, 06:22:30 pm by pito » Logged

0
Online Online
Edison Member
*
Karma: 63
Posts: 1599
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 62
Arduino rocks / OpenLCB rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

Dr. David Harris
OpenLCB Dev Team

Pages: [1] 2   Go Up
Jump to: