Issue with Wire library - sending 0 causes an error

[icarusbop]

Discussed in https://github.com/SpenceKonde/megaTinyCore/discussions/891

^{Originally posted by **icarusbop** January 6, 2023} Hello I have a Raspberry Pi connected to a test circuit with an ATTINY 1624, using the I2c interface. The Rpi is using the SMBUS2 python library, the 1624 is using the Wire Library I can successfully send data from the ATTINY 1624 to the Rpi over I2c, with one small exception - if I try to send a value of zero(0), I get an error on the Rpi receiving side. I have tested with a loop and can send every value from 255 down to 1, it only fails at zero. I can't be sure if the issue is on the Rpi side or the 1624 side, but I have tried testing different functions on the Rpi library side to achieve the same effect (receiving a string of bytes, but only keeping the first byte - this uses a different function and therefore presumably different code), and I get the same error. This makes me wonder if the issue might be on the wire library for the 1624? Here is the minimum code for the 1624 to replicate my issue (I can post the Rpi test code if required) ``` #include //I2c byte ToSend=255; //I set this to 255 to test every value, issue only occurs when we get to zero unsigned char CharBuff; //I copy the value to send into this char buffer char SendBuffer[1] ; //make a buffer to put the send data in void RequestEvent() { /*on interrupt send data to I2c master */ CharBuff=ToSend--; //copy the value sent then decrease for next time. memcpy(&(SendBuffer), &CharBuff, 1); //copy data into the send buffer Wire.write(SendBuffer); //send the data to the I2c } void setup() { Wire.begin(58); //open the I2C bus Wire.onRequest(RequestEvent); // register event for master reading } void loop() { } ``` The error I receive on the Rpi side is: > File "/usr/local/lib/python3.7/dist-packages/smbus2/smbus2.py", line 396, in read_byte > ioctl(self.fd, I2C_SMBUS, msg) > OSError: [Errno 121] Remote I/O error

[MX682X]

I'm not sure how the compiler handles an array with the length of 1, but if it does it like with any other array, you are likely calling Wire.write((char*)SendBuffer);, this is handled by the print class like this:

size_t write(const char *str) {
      if (str == NULL) {
        return 0;
      }
      return write((const uint8_t *)str, strlen(str));
    }

strlen(SendBuffer) equals 0 (as strlen increments until it finds 0x00), thus no data is being written. And the RPi is requesting 1 byte that is never send, thus the remote I/O error, I guess. And it works with other values because the initialization sets all unused ram to 0x00, thus the next byte after SendBuffer is 0x00. So, technically, your code probably had an out-of-bounds bug.

Anyway, the best way to confirm it, is to try this:

void RequestEvent() {
    uint8_t data = ToSend--;
    Wire.write(data); //send the data to the I2c    
}   

P.S.: The Arduino library has it's own buffer, it does not require a user supplied buffer. The write functions just copy the supplied data.

[SpenceKonde]

MX682X has nearly hit the nail on the head. (I say nearly because of his comments about the array size 1 being relevant. It isn't. When you pass a pointer to a char array to Wire.write() all characters until the next zero byte are sent. If the first character is a 0 (0x00, not the '0' ascii character), nothing will be added to the wire buffer, because write'ing a c string does not include the null terminator in what it writes. (If one wants to die on that hill, I would direct them to the arduino github; defining the API of the standard library is not our department. Throughout core developmentm, there are basically three things that are often in conflict which we are trying to do: We want to match the Arduino API closely. We want to provide a performant, memory efficient solution (for reasons that are challenging to describe politely, this goal is almost always at odds with the first. We want to maximize the functionality of the core by exposing the exciting new features available, and to that end we include a number of libraries that provide a minimal interface for controlling almost all the peripherals on the chip not supported by something else (hence Event, Comparator, and a few other libraries being included, plus a few extended functions that manipulate systems which are manipulated by the stock core in analogous ways (that's sort of my test for whether it goes in megaTinyCore.h or Arduino.h) Hence the stuff for the advanced ADC is not part of any library, while the event, logic, comparators, and so on are.

What appears to be happening here, determined from examining the assembly listing is that it's going through this:

   size_t write(const char *str) {
      if (str == NULL) {
        return 0;
      }
      return write((const uint8_t *)str, strlen(str));
    }

As you can see, it does just what he described.

And... of course it does! What would you have expected it to do? You are passing in a pointer to an array of characters with no length specified. That's a C string! It interprets it as such. When it sees a 0x00, that means the string has ended. If that's the first byte, it's an empty string. The string == NULL. That function does nothing and returns a 0. Otherwise, it does strlen() to find the first zero byte, knowing it needs to send all the bytes before that 0 (and conveniently, strlen exactly that number), then casts the pointer to a uint8_t pointer, and calls write() with that as the first arg and the strlen return value as the second arg, asking to write that many bytes starting from the start of the string.

What I sort of wonder is - what does wire do in this case, that is when it empties the slave transmit buffer and the master is still trying to clock out data? If the buffer contained anything at all it could repeatedly send that, but it contains nothing... what will it do? You're backing it into a bad corner... as far as it understands, there is no data to send. But its standing here with it's foot on the SCL line holding the clock line low, because it knows that as soon as it releases it, the master would expect data. Normally it releases the line when you return from onRequest(). But if you write no data to it (as you did) then after onRequest, it's no better off than it was before, it still knows that it needs a byte ready before it lets go of SCL, it hasn't gotten one and the point when it could get one just finished - Now what? It's clearly a situation in which there is no correct response that will do what the user wanted, but I'm somewhat curious about what it does :-P

Really what surprises me is that that works for other numbers. I guess the buffer gets filled with up to the next 32 bytes in memory unless one of them is a 0, and you just don't see that because the master only asks for one byte. Actually, looking at the memory map, I think the next byte is a 0, because that's the only thing in .data and it likes to word align section boundaries, and I think it may zero out such padding just like is zeros out declared global variables - in fact, i'm almost sure it does, and if it doesn't, that's just because the compiler is lousy at it's job, because the C standard doesn't say anything about the values of uninitialized memory in the padding of word-aligned smaller than word variables, at least as far as I know, so it's implementation defined, and it's faster and smaller to do an ST +[X Y or Z], r1 [the zero_reg] than any mechanism for skipping some bytes would be - ST is 1 clock on AVRxt. You can't increment or decrement a pointer any faster than that (there's one option thats the same size, but 2 clocks.... plus some sort of test - so you'd need to be able to skip much more than 1 byte to make that worth it))

One issue this code raises in my mind is that you may not understand how the wire slave interrupts work.

There are two interrupts that the user actually sees, onRequest and onReceive. onReceive is fired when, after an address match with the read/write bit set 0 (write), followed by the master writing whatever data, and then generating a stop condition. Then, when you have that data in the library's buffer. it calls the supplied onReceive. when you get the interrupt with all that data. This one is simple.

The onRequest is fired after an address match with the read/write bit set 1 (read). Immediately. Because that is a declaration from the master that it intends to read one or more bytes - you need to tell wire what to send, even though you have no idea how many bytes the master is going to try to get (per the I2C standard, the master indicates the end of a read by responding to the final byte with a NACK instead of an ACK), so you need to prepare for the longest read that you support.

The I2C buffer is 32 bytes, so that's the most you can write to it (further character are ignored, and write returns 0's). That size limit applies to all parts supported by DxCore or mTC with two exceptions: Dx/Ex-series part with 4096b ram or more, in which case it's bigger (which does not cause compatibility problems, just wastes ram, but like, 4k ram is the ram of the SMALLEST DA/DB parts, and the biggest have 16k! They can take some extra memory overhead and they are the AVRs most likely to be generating large blocks of data), or it's a 2k flash 0/1-series tiny, in which case it has just 128b of ram and we had to reduce the buffer size (to 16b in this case), since there are multiple buffers that need to exist). That means that some libraries won't work there, but nobody will know because the binary for a library like that is gonna come out too large to fit in the 2k flash that those parts have too)

There are myriad other interrupts in the I2C process (I think only 2 vectors, but they're the kind where you then check a status register to figure out which of the many reasons is the one that it was called for, and that code is pretty efficient). Most of the time, these buffers do not call a user handle, though - only the two situations above (very start of read, very end of write operations) actually call the user code, the rest is handled under the hood. (which is mostly putting a byte into or pulling one from the buffer, and deciding if it needs to call the user code handler. And as noted, the I2C standard does not involve the master ever telling the slave how much it will read, only that a read will happen, and when that last byte it received is the last one it wants for now.

(worth noting, as it's also not clear to all users, on the arduino side as master, the data transfers occur when endTransfer() or requestFrom() are called.... and on the master side, those are both blocking operations which don't return until it's done handling the data. )

So in summary: This is an issue with your code, as you using a pointer to a char array, yet what you appear to intend to send is a single byte value. If you want to just write the byte, just write a uint8_t, not a pointer to one. If you want to send an array of bytes, you want a pointer to an array of uint8_t's (not a pointer to int8_t's or chars), and because C doesn't have much awareness of the size of arrays, you need to provide a second argument: the number of bytes that are present in that buffer and which you desire to write). That is, assuming that you didn't want the buffer to have 1 length forever and were just testing

#define MY_BUF_LEN
// globalscope
uint8_t myBuffer[MY_BUF_LEN]]; //optionally, you could initialize it with starting values

// in onRequest
Wire.write(myBuffer, MY_BUF_LEN);// This results in the contents of myBuffer, which are MY_BUF_LEN long, being copied to the wire array. This is done such that a copy is made, so once Wire.write() returns, the data that will be sent in response to this request is fixed, and the only unknown is how many bytes the master will clock out, which can't be known until the final byte is done transferring.

[MX682X]

I'm somewhat curious about what it does

Fair enough. When data is requested, the ISR calls onRequest(). Next it checks if the head is not zero. This is useful, in case of a Register like implementation - master writes a register number and then tries to read that register. If onRequest hasn't written any data, like in the OP's case, a NACK is issued.

[SpenceKonde]

Oh! Of course! during onRequest it's stretching the clock so if it has nothing to send at the end of it, obviously it just nacks. Cool. Duh

[SpenceKonde]

Close because conclusively demonstrated to to incorrect user code rather than a defect in the core.

[icarusbop]

I'm not sure how the compiler handles an array with the length of 1, but if it does it like with any other array, you are likely calling Wire.write((char*)SendBuffer);, this is handled by the print class like this:

size_t write(const char *str) {
      if (str == NULL) {
        return 0;
      }
      return write((const uint8_t *)str, strlen(str));
    }

strlen(SendBuffer) equals 0 (as strlen increments until it finds 0x00), thus no data is being written. And the RPi is requesting 1 byte that is never send, thus the remote I/O error, I guess. And it works with other values because the initialization sets all unused ram to 0x00, thus the next byte after SendBuffer is 0x00. So, technically, your code probably had an out-of-bounds bug.

Anyway, the best way to confirm it, is to try this:

void RequestEvent() {
  uint8_t data = ToSend--;
  Wire.write(data); //send the data to the I2c    
} 

P.S.: The Arduino library has it's own buffer, it does not require a user supplied buffer. The write functions just copy the supplied data.

Hello and thanks for your response - I have had to read it a few times and take some time before I (think) I've figured out what you are saying (hence my delayed response)

I tried the little sample replacement you provided and it works and can transmit all values including zero - so I guess that means the issue is with my code somehow and not the library. Thanks very much for your help.

Regarding the buffer - in my main project the transmit buffer is set up as a union as I need to be able to send different types of data down it. I'll look into figuring out an alternative way of doing this.

[MX682X]

Hello and thanks for your response - I have had to read it a few times and take some time before I (think) I've figured out what you are saying (hence my delayed response)

If there are uncertainties, feel free to ask about them. I know that my thoughts can be kinda chaotic and all over the place and so can be my explainations.

Regarding the buffer - in my main project the transmit buffer is set up as a union as I need to be able to send different types of data down it. I'll look into figuring out an alternative way of doing this.

then I would suggest to try following. It will treat the union as array and copy all bytes to the internal buffer (up to the size of the buffer, see post above).

Wire.write((uint8_t*)&(*name of union*), sizeof(*name of union*));

[icarusbop]

Hello and thanks for your response - I have had to read it a few times and take some time before I (think) I've figured out what you are saying (hence my delayed response)

If there are uncertainties, feel free to ask about them. I know that my thoughts can be kinda chaotic and all over the place and so can be my explainations.

Regarding the buffer - in my main project the transmit buffer is set up as a union as I need to be able to send different types of data down it. I'll look into figuring out an alternative way of doing this.

then I would suggest to try following. It will treat the union as array and copy all bytes to the internal buffer (up to the size of the buffer, see post above).

Wire.write((uint8_t*)&(*name of union*), sizeof(*name of union*));

Hello - this works great, thanks very much. I did just try sending the union, without the '(uint8_t*)' part, but that didn't work. If you have a moment please could you explain it to me?

Thank you.

[SpenceKonde]

Wouldn't sending the union directly just give an error that there was no matching function, or would it get implicitly converted to something dumb? Doesn't wire, for some reason, truncate types larger than a byte when passed to write? And hence the there are two equivalent ways out - either you cast the pointer to the union to a uint8_t *, and then you can write() the bytes as long as you tell it how many, or you have a byte array in your union, of length equal to the longest element, and use that. (Why would arduino have it truncate things to bytes? Likely because the master and slave write are consistent on this point, and if they tried it with automatically splitting up larger datatypes, they found that as master it was a disaster because people use int16_t's when they should be using uint8_t and int8_t, at least in arduinoland, so things would get two bytes instead of one, hence writing to the intended register but also the one located after it, and everyhing would spiral down from there.

[MX682X]

I did just try sending the union, without the '(uint8_t*)' part, but that didn't work.

As I don't know what didn't work (I can come up with three things: Compiler gave an error, Wrong data sent, or no data sent) I can not tell what happend with the code.

Based on your initial post, I guess you are not familiar with pointers. To sum up: Pointers are integers, in the case of an AVR, that are 16 bit wide. Every byte in the RAM has it's own address. They are used to pass around big variables, without having to copy the value itself. To get the address of the first byte of a variable, you can use the & operator.

Why to use '(uint8_t*)' - this is called casting, and tells the compiler that the value or variable after the cast is, in this case, a pointer to an array with 1 byte long elements. It does not chnage the value however. If the compiler complains, that means it has noticed that a pointer to a union is not the same as a pointer to a char/uint8_t array. It wants to make sure that the data a function gets can be processed by said function. But as we want to send an array of single bytes we can safely tell the compiler to treat the union simply as an array in this case. (Sizeof then calculates how many bytes there are to send)

I do not belive the code is using Wire.write(uint16_t) and truncating it, as it has a size argument that is also passed

[icarusbop]

Thanks to both of you for your explanations.

I am new to pointers, this has really helped me understand what is going on with the casting to a pointer. It all makes more sense now.

Thanks for all the help.