Preventing Wire transmit errors  

I have been running into a number of problems trying to use the wire library to handle communication between two arduinos. One issue is that there is no indication that a call to Wire.send in the master will be ignored if the transmit buffer is full. There is a patch discussed here http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1206621511 to return an error code on failure after a transmission has ended, but I don’t see any way to check the buffer so the error can be prevented.

I am tempted to add a method in wire.cpp to return true iff the tx buffer is not full

bool TwoWire::txReady()
{
return(txBufferLength < BUFFER_LENGTH) ;
}

and in my sketches:

void mySend(uint8_t data)
{
// wait up to 10 ms for a call to the tx buffer to be ready
// 10ms is a guess, not sure what is a reasonable time to wait?
for( uint8_t i=0; (!Wire.txReady()) && (i < 10); i++)
delay(1);
Wire.send(data);
}

Is there a better way to handle this issue?

And is there any convention for a software handshake over Wire to prevent receive buffer overflow?

Hi, looks like the Wire-lib needs some more error-handling.

Concerning a buffer-overflow in sending data to a slave, your delay trick does not work. Here is why. In the first step you call Wire.beginTransmission(address). This simply sets a flag (we are transmitting!) and clears the buffer for outgoing data.

Then you call Wire.send(data). This does not actually send the data it simply copies the data into the buffer for outgoing bytes. Here is the first flaw in the Wire lib. There is only one[/one] error condition in this method : The buffer is full, but instead of returning an error for this the send()-Method discards you data silently :o The actual hardware-transmission of the data takes place when you call Wire.endTransmission(), so your hack does not work. The send-buffer will not be drained before endTransmission() runs so there is nothing to be waited for in Wire.send(). But there is a solution for your problem. The size of the buffer is defined in file arduino-0011/hardware/libraries/Wire/utility/twi.h ``` ** #ifndef TWI_BUFFER_LENGTH  #define TWI_BUFFER_LENGTH 32  #end** ``` So you can simply monitor how many bytes you hand over to Wire.send() and call Wire.endTransmission() before you exceed the max buffer-length. And don't forget to vote for an improvement on this (just added it to the end of the list): http://www.arduino.cc/playground/Main/SuggestionsBugs Eberhard

Hi Eberhard, thanks for clarifying that, but I am still not clear on how one should handle flow control with the wire library. How does the wire library handle the case where a previous transmission is still in progress during subsequent call to begin a new transmission?

yep, addressing this gets a vote from me :wink:

Hi, as far as I can see from the code the transmission of the data called from inside Wire.endTransmission() will block until either all data has been transmitted, or an error occurred during the transmission.

Again the Wire lib (in its current form) will not tell you if the transmission was successful or not (even though this status is known inside the library). This .. ahem let's call it bug, is addressed in the other thread you already found.

The patch mentioned in the other thread hasn't made it into the Arduino-sources at http://svn.berlios.de/viewcvs/arduino/trunk/hardware/libraries/Wire/ yet.

Maybe the best thing would be to contact either Mellis or the original poster on the other thread (gohai) for sending you the patches. (Could you also please forward the patch to my mail address, since I lost my copy of it)

BTW: Since updates of the Arduino-sources for libraries seems to be a very slow process, what do you think about a "Proposed patches" page on the playground where people can download the sources for improved library code. The LCD-libs also seem to be a good candidate for things that could be made a bit easier to apply.

Eberhard

BTW: Since updates of the Arduino-sources for libraries seems to be a very slow process, what do you think about a "Proposed patches" page on the playground where people can download the sources for improved library code.

+1

--Phil.

BTW:
Since updates of the Arduino-sources for libraries seems to be a very slow process, what do you think about a “Proposed patches” page on the playground where people can download the sources for improved library code.

+1

–Phil.

‘Error-handling for Wire’++

Phil, thanks for the reminder but I don’t think your vote registered in the playground.

Guys, is this patch from the other thread good enough? Or are there other changes you'd like to see as well? I'll be applying the one I have soon (again, sorry for the delay), but if you want other changes, a patch would be useful.

Phil, thanks for the reminder but I don't think your vote registered in the playground.

That was actually intended to be:

'Proposed patches page on the playground'++

:)

--Phil.

Guys, is this patch from the other thread good enough? Or are there other changes you'd like to see as well? I'll be applying the one I have soon (again, sorry for the delay), but if you want other changes, a patch would be useful.

The patch from the other thread, solves some of the problems but also introduces some changes that would make it incompatible with the current Wire-implementation (The patch alters the way device-addresses are handled which breaks every existing code).

I would be able to come up with some beta-code and documentation by next week. Or is there anybody out there who is in desperate need for error-handling code?

Eberhard

I would be able to come up with some beta-code and documentation by next week. Or is there anybody out there who is in desperate need for error-handling code?

Eberhard

No problem me waiting a week and would be happy to test your code if that would help move things along

The revised version of the patch doesn’t affect the addresses. I’ve applied to my copy of the source and will commit it so you can take a look.

Hi,

The revised version of the patch doesn’t affect the addresses. I’ve applied to my copy of the source and will commit it so you can take a look.

just tested the code with my nunchuck and it works fine. Now there are ways to detect wether the IIC-device is available or unplugged, which will be the most common error-condition by far.

But there is still the original problem that started the topic the internal buffer-overflow in Wire.send().
There are three ways to handle the overflow condition (no matter

  • The new data is discarded and not added to the buffer, but the previous contents of the buffer remain unchanged
  • Wire.send() adds as much bytes of new data as possible to the buffer without overflowing it. This will end up in situations where you want to send 6 bytes to a slave but only 4 are accepted. (This is the current implementation but you have no way of knowing that an overflow occured).
  • All the data in the buffer is deleted and the buffer is cleared in reaction to the overflow

my personal favorite is solution 1 (discard the new data that would make the buffer overflow) since we can not simply return the number of bytes copied to the buffer when we want to to stick to a return type uint8_t.
If the return type would be defined as int, that would solve the problem return<0 signals an error.
A return-value >=0 equals the number of bytes added to the buffer without an overflow.

Another still open problem is the way error-codes for the Wire-lib should be defined. This could be done either with #define statements in Wire.h

#define WIRE_OK 0
#define WIRE_ERR_BAD_ADDRESS 1
#define WIRE_ERR_INVALID_SIZE 2
#define WIRE_ERR_BUFFER_OVERFLOW 3
#define WIRE_ERR_NOT_IN_SLAVE_MODE 4

or as public static const variables inside class Wire.

switch (Wire.endTransmission()) {
case Wire.OK :
   break;
case Wire.ERR_BAD_ADDRESS :
  ...

Any preferences for these two options (#defines are more common so maybe easier for newbies?)
Eberhard

I think it should be fine to change the return types of some functions from uint8_t to int if it makes it easier to indicate error conditions. (Unless there's a good reason not to?)

Eberhard, I agree that the first option is best: new data is discarded and not added to the buffer, and the previous contents of the buffer remain unchanged. I think the return for this case could be worded something more like: #define WIRE_ERR_BUFFER_NO_ROOM 3

My personal preference for the return type would be a typdefed enum, but I think #defines may seem friendlier for most arduino users so I vote for that.

I would think uint8_t return would suffice.

Rereading Eberhard's post, I not sure if it is proposed to return status of the send method as well as endTranmission. If the intent is just to have the return status on endTransmission then IMO this method should not actually send anything placed in the local buffer if the buffer was overflowed by a previous call to send.

Part of my problem with Wiring is the confusing function naming and documentation, the functions simply don't do what they say they do.

I think the documentation link to the wiring.org site hinders rather then helps with the arduino wire library. The descriptions for the following methods really need to be clarified: wire.beginTransmission does not start a transmission, it clears the transmit buffer ready for subsequent calls to Wire.send.

wire.send adds data to the send buffer, this method does not actually send

wire.endTransmission starts rather then ends a transmission .

IMO it would be good to rename the Arduino library functions to something more evocative of their function, for example: Wire.clear rather than Wire.beginTransmission Wire.write rather than Wire.send Wire.transmit rather than Wire.endTransmission Macros could be used so the old names would still compile in legacy code.

On a separate point, I noticed that calloc is used in wire.cpp and twi.cpp even though only a single instance is created. Replacing calloc with static buffers would save over 600 bytes of program memory for sketches not otherwise using calloc/malloc).

We definitely need to document the Wire library on the Arduino site. I’ll try to make the descriptions more accurate and clear. I’m inclined to leave the function names as they are though, because changing them would probably create more confusion than the current names do.

It's your call re the naming, but finding alternate names that are not misleading while keeping legacy support via macros for the old names to me seems less confusing going forward.

I remember when I first visited the UK in the late 60's there was a debate about changing from the old pounds shillings and pence currency to decimalization. The argument for sticking with the old arcane system was 'changing it after all these years would probably create more confusion'. Comfortable as farthings, shillings and sixpence pieces were to the old-timers, I think everyone now is grateful that long term clarity won over short term inconvenience ;)

Hi,

Rereading Eberhard's post, I not sure if it is proposed to return status of the send method as well as endTranmission.

Yes, we need a return value for both calls! The situation with the send()-method is a bit confusing, since it works a bit differently for Master and Slave mode. Master-Mode When you act as a IIC-Master the send() method simply stores the bytes from the argument in an internal buffer of fixed size. The return-value from this method should return 0 if the data was sucessfully stored (enough room in the buffer). When your code wants to store too many bytes the method returns 1 and does not save any of the data (which can be an array of char for instance) in the buffer. The confusing part is that even though the method is called send() no data is going down the wire if you are in IIC-Master mode. The actual transfer of the data happens in endTransmission(). The is why the return value of endTransmission() tells you wether you supplied a bad address from the slave-device, or if there had been any errors in the transmission.

Slave-Mode Her the situation is different. You don't have the calls to beginTransmission() or endTransmission() the library itself will take care that the data gets delivered to the Master requesting it. All you have to do is fill up the buffer. But since the slave-device buffers capacity is limited too, we have the same return values (0->success; 1->Not enough room in the buffer)

If the intent is just to have the return status on endTransmission then IMO this method should not actually send anything placed in the local buffer if the buffer was overflowed by a previous call to send.

With the two return values you can't overflow the buffer by accident. So its safe to split you data into two or more parts and send them off individually.

Part of my problem with Wiring is the confusing function naming and documentation, the functions simply don't do what they say they do.

Yes. The Wire lib needs a new documentation page on the Arduino-(Playground)-Site.

IMO it would be good to rename the Arduino library functions to something more evocative of their function, for example: Wire.clear rather than Wire.beginTransmission Wire.write rather than Wire.send Wire.transmit rather than Wire.endTransmission Macros could be used so the old names would still compile in legacy code.

With the return-values you have everything you need for error-handling without breaking any of the old code that is floating around in tutorials and examples. I rather keep the old names and work on the documentation instead.

Eberhard

Eberhard, thanks for your helpful reply.

Are you making the changes or is mellis? Do either of you want help tesing the new library?

Any idea when the new code could be available for testing?

Hi, I have uploaded a zip-file http://www.wayoda.org/share/code/Wire.zip with a patched version of the lib. The patch consists of the changes already in the arduino svn-repository plus some error handling for inapprobiate uses of Wire.send().

Don't forget to delete the Wire.o and uitility/twi.o files after unzipping the patched sources, so the library get recompiled.

Just have a look at the errorcode #defines in Wire.h and the updated comments in Wire.cpp

I checked the patch with the Nunchuck-demo and it nicely detects a bad address (wrong address or device unplugged) of the slave-device and also buffer overflows when using Wire.send() in Master-Mode.

Finding test cases for other transmission-errors might be hard. How do you force a NACK on a request for data?

If you (or anybody else) has problems with the code and needs urgent help, please use my mail-address since arduino.cc seems to be more up than down in the last two days.

Its up to Mellis to decide what gets into the svn-repo. Code is there, but the documentation in the source probably needs to be improved. I could fix this too...

Eberhard