Patches to improve the Wire library

[GoogleCodeExporter]

This was submitted to the mailing list by Christopher K. Johnson. I'm just
adding it to the issue list since the decision was to have patches moved
here to keep things organized.

-----------
In testing use of the i2c (twi) interface to control a PCA9505 I had to dig
into the Wire library to implement the correct twi bus sequence for reads
from the device.  The 9505 like some other devices such as eeproms and 9535
expects that after the slave address+w a single byte is written to go in
the control register specifying what register offset the read should start
at, and whether the addressed register should be automatically incremented
with each byte read.  Then the master should without sending a stop, send a
repeat start on the twi bus and the slave address+r and begin the read.

Write operations similarly require register offset and AI bit, but that was
possible to accommodate with the existing Wire functions by adding it as
the first byte on the write, and increasing the count of bytes to be
written by one.

I added support for the correct read sequence in twi.c and a readFromReg
function in Wire.c, and have fully tested read and write operation with the
9505.

I also found a pre-existing bug in twi.c wherein it would actually read via
twi one more byte than was requested, and save it in the input buffer due
to the fact that on reads an ack/nack occurs before the ISR is invoked, so
when the next to last byte is received the bit needs to be set to cause a
nack of the last byte.  Previously it was waiting until the last byte
requested was received (and ACK'd) before setting the bit to nack when the
next byte (one too many) was received.

I improved some comments and indentation.

I made a couple of other code tweaks in twi.c that should not alter behavior.

I set the default twi bus clock (TWI_FREQ) to 400KHz instead of 100KHz
because I believe support for that speed to be the norm, not the exception.
 That is the only change which might adversely affect existing uses of the
Wire library.

A recursive diff of changes is attached.  I am new to this list and not
sure what the best way is to submit such changes for review and
incorporation in future releases.  If there is a more acceptable way for me
to do so, please advise.  Furthermore I am not sure where to begin to make
changes to the online documentation for the library if someone could point
me in the right direction.

Other changes I would like to discuss:
1) Consider changes to enable twi hardware response to twi bus general call
sla+r/w requests.
2) In a multi-master twi bus environment there are some weaknesses in the
current design - race conditions on testing then altering twi_state in
functions not protected from interruption as well as changes to twi_state
within the twi isr, and the lack of a way to indicate failure of a read
operation due to multiple possible factors such as bus contention.

Sincerely,
Christopher K. Johnson

Original issue reported on code.google.com by josiah.ritchie on 7 Jun 2009 at 8:15

[GoogleCodeExporter]

Original comment by josiah.ritchie on 7 Jun 2009 at 8:19

Attachments:

• Wire.diff

[GoogleCodeExporter]

Oh yeah, @dmellis, could you tag this one as having a patch and needing review?

Original comment by josiah.ritchie on 7 Jun 2009 at 9:29

[GoogleCodeExporter]

Ignore the earlier Wire.diff.  Since that was posted I have implemented further
changes, and have now tested against a 24LC1025 EEPROM as well as the PCA9505.  

Given that some devices use an 8-bit slave-side offset to access registers and 
others
use a 16-bit offset to access memory, I decided to rename the functions I 
added, and
add more for the 16-bit variations.  The added functions are now:
// set up write with 8-bit slave-side offset for registered I/O devices
beginTransmissionAt(twi_addr,offset)
// set up write with 16-bit slave-side offset for EEPROM devices
beginTransmissionAt2(twi_addr,offset)
// initiate read with 8-bit slave-side offset for registered I/O devices
requestFromAt(twiaddr,offset,count)
// initiate read with 16-bit slave-side offset for EEPROM devices
requestFromAt2(twiaddr,offset,count)

Attached are diffs from the original Arduino-0016 version of Wire library, and a
sample sketch of EEPROM read and write using it.

Please review.
Chris

Original comment by ckjohn...@gwi.net on 14 Jun 2009 at 7:11

Attachments:

• Wire.diff
• TWI_EEPROM_Sample.pde

[GoogleCodeExporter]

Thanks for digging into this.  I didn't write the Wire library and I'm not an 
expert on I2C, but here are my 
thoughts.

It sounds like the basic problem, besides the bug of reading one too many 
bytes, is that there's no way to 
send an address to read from without following it with a stop.  Is that right?  

The requestFromAt() and beginTransmissionAt() are just convenience functions to 
combine automatically send 
the address (within the slave) to read from or write to, correct?  

Couldn't we just, for example, add a boolean parameter to endTransmission() 
that specifies whether or not to 
send a stop at the end of the transmission?

I see the value in the convenience functions, but it would be nice if they 
could be implemented on top of 
something close to the existing API.  It would also be good to discuss the API 
on the developers list. 

In any case, we could start by creating a simple way for people to be able to 
do writes without stops (for 
specifying an address to read from).

What do you think?

Original comment by dmel...@gmail.com on 20 Jun 2009 at 12:18

• Added labels: Component-Core

[GoogleCodeExporter]

While technically a way to inhibit sending the stop after writing a slave-side
address/offset could work, there are a couple of good reasons for not taking 
that
approach.

1) The TWI bus can be multi-master, and we are in essence reserving the bus 
still
until the stop is sent.  So it is not a good idea to use separate function 
calls to
begin a bus I/O with start condition, and subsequently complete the I/O with 
stop
condition.
2) On the slave side the GPIO needs to always receive the slave-side offset for
predictable operation.  Some devices would continue a read/write at the last
auto-incremented position, but some would roll over into reserved offsets with
unspecified results.
3) If someone can tell endTransmission() to not send a stop condition, they are 
bound
to do so when it is not advisable simply from a lack of understanding.  I 
recommend
we do not give them a control that is not necessary and allows them to do 
things they
should not do.  Encapsulating the transmit, repeat-start, receive in the 
functions I
added to twi.c underlying requestFromAt() and requestFromAt2() is enforcing 
correct
use of the bus.   

Since these devices need their slave-side address/offset (8-bit for some, 
16-bit for
others) it is a simpler API to use the ..At or ..At2 functions to specify them
instead of calling two functions.

This is different hardware from most I/O in that there is a shared 
communication bus
involved with its own semantics to achieve arbitration of master, addressing of
slaves, and control of I/O direction.  What the existing Wire implementation 
did not
support is the slaves also require differing variations on those semantics. I
honestly think that the added functions are as simple an encapsulation of the 
slave
variations as possible while enforcing correct bus use.

Regardless of what changes developers want in Wire.cpp, I feel strongly that the
functions in twi.c should remain essentially unchanged.  Better coding, bugs? - 
yes
please improve them, but different functions - please don't since they would no
longer enforce bus use correctly.

Original comment by ckjohn...@gwi.net on 20 Jun 2009 at 1:48

[GoogleCodeExporter]

So the basic idea is that the whole write/read sequence needs to be queued up 
before it starts, right?  How do 
we know that the only combined messages we need to support are those that 
consist of a one- or two-byte 
register address / command followed by a read?  The description of I2C on 
Wikipedia (I told you I'm not an 
expert!) seems to imply that devices can support arbitrary combined messages.  
Do they not do so in practice?  

Original comment by dmel...@gmail.com on 20 Jun 2009 at 4:20

[GoogleCodeExporter]

A very good question David:
How do we know that the only combined messages we need to support are those that
consist of a one- or two-byte register address / command followed by a read?

I would throw that out to everyone - does anyone know of devices which use other
combined sequences?

I am not aware of any from my research, but it may just be a spec sheet I 
haven't read.

Original comment by ckjohn...@gwi.net on 20 Jun 2009 at 4:45

[GoogleCodeExporter]

Further revised:
Added Wire.setSpeed() that takes a long unsigned int for the twi speed, but 
also has
literals STANDARD and FAST defined which set 100kbps and 400kbps respectively.
Reverted the Wire.begin() to set the speed to 100kbps.  Wire.setSpeed also 
returns
the formerly set speed.

EXAMPLE:
long unsigned int oldspeed = Wire.setSpeed(FAST);
// when executed the first time after Wire.begin, oldspeed will be 100000.
oldspeed = Wire.setSpeed(250000);  // set 250kbps
// oldspeed will be 400000 (FAST)

Also I changed the speed calculations to use F_CPU instead of a literal value of
16000000 defined in twi.h which twi_init() previously used.

Tested.

Original comment by ckjohn...@gwi.net on 21 Jun 2009 at 1:30

Attachments:

• Wire.diff

[GoogleCodeExporter]

I forgot to say regarding the comment 8 diff, that I also changed twi_init() to
allocate the buffers *before* enabling the twi interrupt instead of afterward.

Original comment by ckjohn...@gwi.net on 21 Jun 2009 at 1:46

[GoogleCodeExporter]

Strangely arduino-0017 incorporated the fix for reading one too many bytes, but 
not
the fix for TWI clock rate setting being based on a fixed assumption of a 16mbps
clock, nor enabling the interrupt before allocating buffers.

I have attached an updated unified diff of my update in relation to the Wire 
lib in 0017.

Original comment by ckjohn...@gwi.net on 13 Aug 2009 at 12:09

Attachments:

• Wire.diff

[GoogleCodeExporter]

Can you post a separate patch with just the obvious bug fixes so that can be 
incorporated while we consider the 
larger changes?

Original comment by dmel...@gmail.com on 15 Aug 2009 at 8:31

[GoogleCodeExporter]

Folks, we've obviously exhausted the OP's patience.  This is a major issue in 
the
i2c, twi library and it is very clearly stated in the i2c spec that repeated 
start is
allowable with any combination of write/read. 
http://www.nxp.com/acrobat_download2/literature/9398/39340011.pdf page 14.  If 
you
google around you'll see other cases of people having to chuck out the Wire lib 
and
hacking up direct access code.

WRT to the choice of API, I would guess that the OP's functions handle 99% of 
the
cases in a nice and elegant manner and therefore should be committed.  

Additionally (and we can wrangle out this API later) it is a common complaint 
that it
is hard to translate a chip spec into Wire calls because Wire is so high level. 
 So
there really ought to be a "raw" API that lets you specify exactly what goes on 
the
bus.  Obviously this API is not going to be for the typical Arduino user (often 
a
programming noob).  Instead it is for experienced people to write their own chip
libraries on top, and can handle both the spec xlat issue and provide support 
for
arbitrary i2c.

However, the most import thing is to get the commonly-used "write address, then 
read
data" functionality working ASAP, and as the OP suggested, do it in a manner 
that
holds the bus for as short a time as possible!

Original comment by G.Andrew...@gmail.com on 1 Mar 2010 at 3:47

[GoogleCodeExporter]

Update: I've manually patched in the OP's diff to the latest git version of 
Arduino because this problem was preventing me from connecting Arduino <=> 
OpenServo.

I will submit a pull request to Arduino as soon as I have tested it with 
hardware to verify I did not screw up the patch in any way.

see: 
https://github.com/D1plo1d/Arduino/commit/b147c09a06b5599a98de6a7687dadd2a4d9c9c
0e

Original comment by d1plo1d%...@gtempaccount.com on 26 Jan 2011 at 1:20

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Update: I have rebased the patch to 1.0, removed the part about setSpeed as I 
saw that was handled elsewhere.
To ease pull request it is now split into two parts, one for requestFromAt:
https://github.com/arduino/Arduino/pull/50
and one for beginTransmissionAt:
https://github.com/arduino/Arduino/pull/51

Please consider taking at least pull/50 in to Arduino as this is a major issue 
in the Wire library.
For the future a new requestFromAt function that takes a pointer and a length 
of the write data is probably desirable, but this current patch fixes the issue 
for most cases. 

Original comment by backst...@akafugu.jp on 5 Dec 2011 at 9:57

[GoogleCodeExporter]

Dont understand why this fix is not committed by couple of years.
Last time i need to read rest RAM from pcf8583 clock and need to hack this out.
Here we have clean sollution for this and this is more readable and 
selfcontained.

So what is blocking to pull this out?

Original comment by cactus.l...@gmail.com on 10 Jan 2012 at 1:17

[GoogleCodeExporter]

Reading and Writing from/to an internal register are very common i2c 
operations. The latest Wire library implementation has added a new parameter 
sendStop, to TwoWire::requestFrom and TwoWire::endTransmission. This, 
theoretically, would allow sending a repeated start for reading/writing a 
register inside the slave device. Unfortunately the Wire implementation for the 
Arduino Due boards ignores the sendStop parameter.  Furthermore, I think that 
implementing a "repeated start" on the Due would be complicated (from what I 
could understand from the Atmel SAM3X8E ARM Cortex-M3 CPU  spec). Nevertheless, 
an implementation for readFromReg and writeToReg would be straight forward 
using the functions already provided by libsam in the twi.c file:

void TWI_StartRead(
    Twi *pTwi,
    uint8_t address,
    uint32_t iaddress,
    uint8_t isize)

void TWI_StartWrite(
    Twi *pTwi,
    uint8_t address,
    uint32_t iaddress,
    uint8_t isize,
    uint8_t byte)

iaddress is the internal addres of the register we want to read/write
isize is the size in bytes of the internal address

Original comment by dan.corneanu@gmail.com on 19 Dec 2012 at 10:17

[GoogleCodeExporter]

The sendStop parameter does fix the problem.  The following routines 
demonstrate how Wire can be used to implement the write addr then read or write 
data protocol.  These code snippets were tested on a i2c digital 3 axis 
accelerometer, so I recommend that this bug be closed as fixed.

unsigned char I2c::writeAddrWriteData(unsigned char deviceAddress, unsigned 
char addr, const unsigned char* buf, int length)
  {
    Wire.beginTransmission(deviceAddress);
    Wire.write(addr);
    for (int i=0;i<length;i++) Wire.write(buf[i]);
    Wire.endTransmission();
    return length;
  }

unsigned char I2c::writeAddrReadData(unsigned char deviceAddress, unsigned char 
addr, unsigned char* buf, int length)
  {
    //Wire.requestFromAt((unsigned char)deviceAddress,(unsigned char)addr,(unsigned char)length);
    Wire.beginTransmission(deviceAddress);
    Wire.write(addr);
    Wire.endTransmission(false);  // Don't issue a stop because we need to read the value.
    Wire.requestFrom((uint8_t) deviceAddress, (uint8_t) length, (uint8_t) false);   
    for (int i=0;i<length;i++,buf++) *buf = Wire.read();
    Wire.endTransmission(true);  // Now issue the stop
    return length;
  }

Original comment by G.Andrew...@gmail.com on 9 Jan 2013 at 4:03

[GoogleCodeExporter]

Hello,
I forgot to mention that I was using an Arduino DUE for tests.
Have you tested it on Arduino DUE ? and did you check the output signals with a 
scope?

Best regards,
Dan.

Original comment by dan.corneanu@gmail.com on 9 Jan 2013 at 9:07

[GoogleCodeExporter]

Hi friends,

i am moni from delhi Arduino is an open-source electronics prototyping platform 
based on flexible, easy-to-use hardware and software.

Thank you, 

Original comment by monikuma...@gmail.com on 7 Jul 2015 at 11:37