Wire needs high level write_to and read_from methods

[GoogleCodeExporter]

Hi everybody,

Having now quite some experience in working with I2C devices I think that the 
current implementation of Wire lacks some very helpful routines to do something 
which is quite standard with I2C devices: write to a register and read from a 
register of the slave device (when arduino is the master).

The results is that, if you have a look at the various I2C devices Arduino 
libraries you'll see often the repetition of the following lines:

For slave register write:
  Wire.beginTransmission(DEVICE_ADDRESS);
  Wire.send(REG_ADDRESS);
  Wire.send(VAL_TO_WRITE);
  Wire.endTransmission();

For slave register read:
  Wire.beginTransmission(DEVICE_ADDRESS);
  Wire.send(REG_ADDRESS);
  Wire.endTransmission();
  Wire.beginTransmission(DEVICE_ADDRESS);
  Wire.requestFrom(DEVICE_ADDRESS,NUM_BYTES_TO_READ);
  int i = 0;
  while(Wire.available()) { //ACC may send less than requested (abnormal)
    buff[i] = Wire.receive(); // receive a byte
    i++; }
  Wire.endTransmission();

I saw code similar to that in almost all the I2C Arduino libs out there.. Of 
course, this results in huge code repetition if you are using more than one I2C 
device comporting increase in program memory and additional unneeded code 
complexity. This can also be seen in the Wire lib examples!

But let's make practical examples..

-----
Example SFRRanger_reader in Wire lib examples:
// step 1: instruct sensor to read echoes
  Wire.beginTransmission(112); // transmit to device #112 (0x70)
                               // the address specified in the datasheet is 224 (0xE0)
                               // but i2c adressing uses the high 7 bits so it's 112
  Wire.send(0x00);             // sets register pointer to the command register (0x00)  
  Wire.send(0x50);             // command sensor to measure in "inches" (0x50) 
                               // use 0x51 for centimeters
                               // use 0x52 for ping microseconds
  Wire.endTransmission();      // stop transmitting

****
// step 3: instruct sensor to return a particular echo reading
  Wire.beginTransmission(112); // transmit to device #112
  Wire.send(0x02);             // sets register pointer to echo #1 register (0x02)
  Wire.endTransmission();      // stop transmitting

  // step 4: request reading from sensor
  Wire.requestFrom(112, 2);    // request 2 bytes from slave device #112

  // step 5: receive reading from sensor
  if(2 <= Wire.available())    // if two bytes were received
  {
    reading = Wire.receive();  // receive high byte (overwrites previous reading)
    reading = reading << 8;    // shift high byte to be high 8 bits
    reading |= Wire.receive(); // receive low byte as lower 8 bits
    Serial.println(reading);   // print the reading
  }

--------------------
digital_potentiometer example in Wire lib

  Wire.beginTransmission(44); // transmit to device #44 (0x2c)
                              // device address is specified in datasheet
  Wire.send(0x00);            // sends instruction byte  
  Wire.send(val);             // sends potentiometer value byte  
  Wire.endTransmission();     // stop transmitting

--------------------
ADXL345 arduino driver, http://code.google.com/p/adxl345driver/

// Writes val to address register on device
void ADXL345::writeTo(byte address, byte val) {
  Wire.beginTransmission(DEVICE); // start transmission to device 
  Wire.send(address);             // send register address
  Wire.send(val);                 // send value to write
  Wire.endTransmission();         // end transmission
}

// Reads num bytes starting from address register on device in to _buff array
void ADXL345::readFrom(byte address, int num, byte _buff[]) {
  Wire.beginTransmission(DEVICE); // start transmission to device 
  Wire.send(address);             // sends address to read from
  Wire.endTransmission();         // end transmission

  Wire.beginTransmission(DEVICE); // start transmission to device
  Wire.requestFrom(DEVICE, num);    // request 6 bytes from device

  int i = 0;
  while(Wire.available())         // device may send less than requested (abnormal)
  { 
    _buff[i] = Wire.receive();    // receive a byte
    i++;
  }
  if(i != num){
    status = ADXL345_ERROR;
    error_code = ADXL345_READ_ERROR;
  }
  Wire.endTransmission();         // end transmission
}

-----------
ITG3200 http://code.google.com/p/itg-3200driver/

void ITG3200::writemem(byte _addr, byte _val) {
  Wire.beginTransmission(_dev_address);   // start transmission to device 
  Wire.send(_addr); // send register address
  Wire.send(_val); // send value to write
  Wire.endTransmission(); // end transmission
}

void ITG3200::readmem(byte _addr, int _nbytes, byte __buff[]) {
  Wire.beginTransmission(_dev_address); // start transmission to device 
  Wire.send(_addr); // sends register address to read from
  Wire.endTransmission(); // end transmission

  Wire.beginTransmission(_dev_address); // start transmission to device 
  Wire.requestFrom(_dev_address, _nbytes);// send data n-bytes read
  byte i = 0; 
  while (Wire.available()) {
    __buff[i] = Wire.receive(); // receive DATA
    i++;
  }
  Wire.endTransmission(); // end transmission
}

----
HMC5843 library https://launchpad.net/hmc58x3

void HMC5843::writeReg(unsigned char reg, unsigned char val) {
  Wire.beginTransmission(HMC5843_ADDR);
  Wire.send(reg);        // send register address
  Wire.send(val);        // send value to write
  Wire.endTransmission(); //end transmission
}

void HMC5843::getValues(float *x,float *y,float *z) {
  int xr,yr,zr;
  Wire.beginTransmission(HMC5843_ADDR);
  Wire.send(HMC5843_R_XM); // will start from DATA X MSB and fetch all the others
  Wire.endTransmission();

  Wire.beginTransmission(HMC5843_ADDR);
  Wire.requestFrom(HMC5843_ADDR, 6);
  if(6 == Wire.available()) {
    // read out the 3 values, 2 bytes each.
    xr = Wire.receive();
    xr = (xr << 8) + Wire.receive();
    *x= ((float) xr) / x_scale;
    yr = Wire.receive();
    yr = (yr << 8) + Wire.receive();
    *y = ((float) yr) / y_scale;
    zr = Wire.receive();
    zr = (zr << 8) + Wire.receive();
    *z = ((float) zr) / z_scale;
    // the HMC5843 will automatically wrap around on the next request
  }
  Wire.endTransmission();
}

Same happens for 
http://interactive-matter.eu/2009/12/arduino-barometric-pressure-sensor-bmp085/
http://www.neufeld.newton.ks.us/electronics/?p=241
http://naneau.nl/2009/01/18/arduino-and-an-i2c-temperature-sensor/
http://bazaar.launchpad.net/~nxti2cdev/nxti2cdevice/trunk/view/head:/BaseNXTI2CD
evice.cpp
http://www.arduino.cc/playground/Code/I2CEEPROM

and basically any code from Googleing "Arduino I2C" shows some examples of that.

Now, in the attached patch I implemented two simple functions which implements 
those functionalities. I'll gladly add documentation and write to the various 
maintainers of the I2C libs if this get inserted into Wire.

Interested in hearing your opinions,

Thanks,

Fabio Varesano

Original issue reported on code.google.com by fabio.va...@gmail.com on 23 Jan 2011 at 4:08

Attachments:

• Wire_WriteTo_ReadFrom_fabio_varesano_1.patch

[GoogleCodeExporter]

+1

Excellent idea!

Actually, I'm using Fabio's writeTo and readFrom routine on each of my I2C 
libraries, resulting on duplicated code and lost space.

Thank you for considering Fabio's idea.

Original comment by Vilo.Rei on 24 Jan 2011 at 7:13

[GoogleCodeExporter]

Clearly, this is a valuable refactoring.  How do we help push the changes into 
the Wire library?

Original comment by clinton....@gmail.com on 24 Jan 2011 at 4:26

[GoogleCodeExporter]

Give the patch a try, look at the patch code and suggest improvements, if 
everything looks ok, say so!

Original comment by fabio.va...@gmail.com on 24 Jan 2011 at 5:40

[GoogleCodeExporter]

Hi!

Fabio idea is good, it solves code repetition and especially solves the problem 
of having multiple approaches (some better than others) to read/write registers 
resulting in more stable code. This should be part of wire library.

-Has anyone noticed sketch size increase by a few bytes when compiling with 
patched wire?
-Any reason having int* buff and not uint8_t*? 
-I would suggest Overloading writeTo to allow using send(uint8_t*, uint8_t), 
something like:
void TwoWire::writeTo(int dev_address, int reg_address, uint8_t* buff, int 
quantity).

Regards,
Filipe Vieira

Original comment by fil.vie...@gmail.com on 27 Jan 2011 at 1:34

[GoogleCodeExporter]

1: if the size increase is just a few bytes.. that's ok.. as as soon as we run 
more I2C devices they will share the writeTo and readFrom decreasing the total 
memory consumption.
2: uint8_t is for unsigned ints.. there are I2C devices which gives you 
negative values and you can write negative values to their registers.. that's 
why I choose int .. what do you think?
3: yeah, me too. Anyone care to submit a patch?

Original comment by fabio.va...@gmail.com on 27 Jan 2011 at 10:39

[GoogleCodeExporter]

Probably that’s why there is void TwoWire::send(uint8_t data) and then void 
TwoWire::send(int data) a simple type-cast to handle signed values... but in 
the end it’s uint8_t just check twi_transmit(uint8_t* data, uint8_t length). 
Nicer for writeTo and readFrom follow same design? (also less casting)
I've attached a modified patch.

Fil.

Original comment by fil.vie...@gmail.com on 1 Feb 2011 at 2:31

Attachments:

• Wire_WriteTo_ReadFrom.patch

[GoogleCodeExporter]

Sure, internally it will use uint8_t but, as I think of readFrom and writeTo as 
high level I used int. Your approach is good but isn't it slower? If users use 
int you will call everytime two functions instead of one, isn't so?

Original comment by fabio.va...@gmail.com on 1 Feb 2011 at 9:45

[GoogleCodeExporter]

On Arduino forum (http://arduino.cc/forum/index.php/topic,50082.0.html), 
wayneft advised that in twi.c there are functions twi_writeTo() and 
twi_readFrom() so we should probably use them instead of relying on Wire 
methods.

Original comment by fabio.va...@gmail.com on 1 Feb 2011 at 12:54

[GoogleCodeExporter]

I like these functions generally.  The only request I would make it to pull the 
available() and receive() calls out of readFrom().  That is, readFrom() should 
request the data from the slave, but not actually read it into a buffer.  That 
makes it more flexible, for example if we add other receive methods.  Also, do 
you need the second  beginTransmission() / endTransmission() pair in the 
readFrom() code?  I think you can just call requestFrom().

Also, the twi_writeTo() and twi_readFrom() in twi.c don't do the same thing 
(they don't write to or read from a register within the slave device), so you 
don't need to change the code to use them.

Original comment by dmel...@gmail.com on 11 Feb 2011 at 5:31

• Changed state: Accepted
• Added labels: Component-Core, Milestone-0023

[GoogleCodeExporter]

Following dmellis comment and based on last patch I've made the following 
changes to readFrom():

void TwoWire::readFrom(uint8_t dev_address, uint8_t reg_address, uint8_t *data, 
uint8_t quantity)
{
  beginTransmission(dev_address);
  send(reg_address);
  endTransmission(); 
  requestFrom(dev_address, quantity);
  *data = receive(); // receive a byte
}

works for me! Comments please...

Filipe

Original comment by fil.vie...@gmail.com on 11 Feb 2011 at 11:36

[GoogleCodeExporter]

Thank you David.

Filipe, how could that work with only a call to receive? What about quantity? 
Or, am I missing something?

Original comment by fabio.va...@gmail.com on 12 Feb 2011 at 9:54

[GoogleCodeExporter]

Filipe: the readFrom() should receive any data at all, and shouldn't take a 
*data parameter.  It should just do the requestFrom() and let the caller get 
the data using receive() and available().  

Fabio: do you want to update and test the patch?

Original comment by dmel...@gmail.com on 12 Feb 2011 at 5:05

[GoogleCodeExporter]

Oh.. ok.. now I get what you mean David.

Personally, I think of readFrom() and writeTo() as high level access to reading 
and writing from/to a register (or more) on a I2C device. The idea behind them 
was avoiding code repetition and improving ease of use.

But, if we remove receive() from readFrom() we fail at both of them, requiring 
more code to be written and making everything more complex.

My idea was to add those functions so that the user could just rely on them for 
interacting with a I2C device and forget about anything else.

Also, would you explain what different receive methods could be implemented? I 
don't see many different ways to do so.

Original comment by fabio.va...@gmail.com on 12 Feb 2011 at 6:02

[GoogleCodeExporter]

The problem is that you don't necessarily want to read the data into a buffer.  
In general the Wire API is inconsistent with the rest of the libraries (e.g. 
doesn't implement Stream).  I might try to make it more consistent with them, 
which would require change receive(), etc.  It would easier to do that if it 
wasn't integrated into this new function.  Basically, the part we'd factor out 
is the part for requesting the data (which is always the same), not the part 
for actually reading it (which could happen in a few different ways, 
potentially).

Original comment by dmel...@gmail.com on 12 Feb 2011 at 6:22

[GoogleCodeExporter]

Original comment by dmel...@gmail.com on 24 Aug 2011 at 6:50

• Added labels: Milestone-1.0
• Removed labels: Milestone-0023

[GoogleCodeExporter]

I've got an I2Cdev library that is built to do exactly this kind of 
abstraction, up on GitHub here:

https://github.com/jrowberg/i2cdevlib

The only important bits are the I2Cdev.cpp and I2Cdev.h files. They might be a 
little overkill for what you're going for, but they're working great for me and 
the rest of those device libraries so far.

Original comment by jeffrowb...@gmail.com on 31 Aug 2011 at 8:18

[GoogleCodeExporter]

I don't know.. the more I dig into Wire and twi.c the more I think that they 
are all an over complication.. and IMHO, adding yet another layer it's not a 
good idea. When dealing with I2C performance and program memory are key factors 
and your code (well designed and documented BTW) doesn't really help.

For what's my use of Wire in my own projects, the following library does about 
98% of what I need while being extremely smaller, simpler and faster than Wire.
http://code.google.com/p/simplo/source/browse/trunk/libraries/fastwire/Fastwire.
cpp

Original comment by fabio.va...@gmail.com on 31 Aug 2011 at 10:40

[GoogleCodeExporter]

Original comment by dmel...@gmail.com on 16 Dec 2011 at 10:08

• Removed labels: Milestone-1.0