arduino / ArduinoCore-avr

The Official Arduino AVR core
https://www.arduino.cc
1.24k stars 1.05k forks source link

Wire.h: Wrong number of bytes returned from the slave device (I2C) #384

Open OpreaFlorin opened 3 years ago

OpreaFlorin commented 3 years ago

Hi. According to the documentation, the two functions below will return the number of bytes that the master has received from the slave and which are available in the buffer.

Wire.requestFrom() Description Used by the master to request bytes from a slave device. Returns byte : the number of bytes returned from the slave device

Wire.available() Description Returns the number of bytes available for retrieval with read(). Returns The number of bytes available for reading.

But if slave will respond with a less number of bytes than master requested (as is described in Master Reader/Slave Sender Example), then we have a problem because both functions (Wire.requestFrom() and Wire.available()) will return the requested number of bytes, not the number of bytes received from slave. So, the output will be hello ⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮⸮.

master_reader.ino

#include <Wire.h>

void setup() {
  Wire.begin();        // join i2c bus (address optional for master)
  Serial.begin(9600);  // start serial for output
}

void loop() {
  Wire.requestFrom(8, 32);    // request 32 bytes from slave device #8

  while (Wire.available()) { // slave may send less than requested
    char c = Wire.read(); // receive a byte as character
    Serial.print(c);         // print the character
  }

  delay(500);
}

slave_sender.ino

#include <Wire.h>

void setup() {
  Wire.begin(8);                // join i2c bus with address #8
  Wire.onRequest(requestEvent); // register event
}

void loop() {
  delay(100);
}

// function that executes whenever data is requested by master
// this function is registered as an event, see setup()
void requestEvent() {
  Wire.write("hello "); // respond with message of 6 bytes
  // as expected by master
}

It will be great if it will be fixed.

Regards, Florin.

matthijskooijman commented 3 years ago

Hm, is this even possible? I haven't checked, but IIRC an ACK is only sent by the device receiving data, so when reading data from a slave, it's the master that sends ACKs, not the slave. So AFAICS, there's no way that the slave can actually tell the master how many bytes to return, so the master always reads as much as he expects to read.

As I said, I haven't doublechecked, but the I2C spec should be able to confirm this (or prove me wrong :-p).

Koepel commented 3 years ago

This is part of the I2C standard, therefor this can not be fixed in hardware nor in the Wire library. However, the documentation can be better. I think all that is left is to assign the label "Documentation" to this issue.

Related to: https://github.com/arduino/ArduinoCore-avr/issues/171

Ragebone commented 2 years ago

I agree and i disagree.

From the i2c spec, section 3.1.6, page 10 of UM10204:

The acknowledge bit allows the receiver to signal the transmitter that the byte was successfully received and another byte may be sent.

That makes me solely think of a Master writing to a Slave. But it appears to indeed be against the i2c specification, since that text is otherwise pretty clear. And it seems to actually be implemented that way in multiple places including the PMBus protocol.

But i also find that less reasonable and i think that this is a case of squishing specification for compactness and neatness. For me, it ignores the inherent power the master has. I don't see a reason for why the master needs to indicate receiving the byte successfully to the slave.

I think that it is technically possible to implement it the other way. At least with the current implementation and Arduino hardware. The only question is if someone wants that. One would only need to modify the ISR in line 503, specifically 548-578 and make a departure from the specified behavior. Though that will very very likely break functionality with existing and conforming i2c slaves.

Of note for that is that the current Arduino Slave Transmit implementation breaks the i2c specification already and ACKs if there is a Byte available to be transmitted, visible in lines 643-648. This is of no consequence but technically a violation of the spec. In combination with an Arduino as i2c slave, it should therefore be enough to make the case in line 561 always NACK. If the TWI Hardware reacts to the set Register and not the Bus, this will not work. I doubt that but i have not confirmed it with tests yet.

Koepel commented 2 years ago

@Ragebone, it is not possible to change the standard I2C behavior, that is not implemented in hardware.

Who is sending the ACK makes sense at hardware level. See the 'ACK' as a signal that the next byte can be latched into the shift register. The SCL clock shifts the bits out or in the shift register. The I2C bus was designed 40 years ago, so it is with basic logic hardware. When the Master sends data, then the Slave gives the ACK between the data bytes. When the Master requests data, then the Master gives the ACK between the data bytes.

You are right about the code in Slave Transmitter mode. In the source code, the Slave uses ACK or NACK. I assume that it is ignored by the hardware, because the Master does the ACK and NACK.

Ragebone commented 2 years ago

@Koepel

it is not possible to change the standard I2C behavior, that is not implemented in hardware.

I'm sorry but that does not really make sense to me. Can you please elaborate on that or rephrase it?

Who is sending the ACK makes sense at hardware level. See the 'ACK' as a signal that the next byte can be latched into the shift register. The SCL clock shifts the bits out or in the shift register. The I2C bus was designed 40 years ago, so it is with basic logic hardware. When the Master sends data, then the Slave gives the ACK between the data bytes. When the Master requests data, then the Master gives the ACK between the data bytes.

That seems reasonable to me and would explain why it was made that way. At least as far as i understand what you wrote.

You are right about the code in Slave Transmitter mode. In the source code, the Slave uses ACK or NACK. I assume that it is ignored by the hardware, because the Master does the ACK and NACK.

Be aware that the used mycrocontrollers don't have i2c hardware. Instead they have TWI hardware that can be used with i2c. So i would not assume that the hardware ignores the ACK and NACK but only tests will show what really happens. That is also the reason why i think that it can be changed the way i described.

Koepel commented 2 years ago

The TWI hardware is not a universal programmable block of hardware (such as the USI of the ATtiny, or with newer processors). The TWI hardware is only for I2C. I think that things outside the I2C standard are not implemented in hardware.

Ragebone commented 2 years ago

After some tests with two pro micro, i can say that the Master can be made to NACK as stated previously. Where as the Slave appears to ignore any instruction to ACK on left Bytes to read. Though there is some evident play in the TWI hardware, it appears that @Koepel was right with his assumption of Slaves ignoring instructions to ACK Data-Bytes. But i could be making mistakes and other Arduino as slave might behave differently.

Anyway, i'd count this as another nail in the coffin of this issue, So Arduino are apparently following spec, hardware appears to be implemented that way and hence, this is not a Bug but a Feature.

Fun-fact, the Master can be made to always ACK, which results in funny bogus.

Stumbled over the UP9512 GPU VRM Controller that reads like it breaks i2c spec and ACKs as well.