Requesting multiple bytes: the correct way

[dhunink]

Hi,

I've been spending a few days now on reading into TinyWireS, I2C protocols and standards.
After seeing the interesting discussions in different issues I do believe that TinyWireS offers support for multiple bytes to be requested by it's master. Is this correct?

I tried a million ways so far, but can't seem to get more then a single byte returned from the slave, using Wire on the Master. The code is below. If the assumption that TinyWireS is capable of handeling a master calling Wire.requestFrom(0x01, N), where am I going wrong with the code below?

Master (Arduino Uno)

/*
 * 
 */

#include <Wire.h>

#define I2C_MASTER_ADDR 0x04
#define I2C_SLAVE_ADDR 0x05

int pollInterval = 700;//Milliseconds
unsigned long lastPoll = 0;


void setup() 
{
  Wire.begin(I2C_MASTER_ADDR);  // join i2c bus
  Serial.begin(115200); 
  Serial.println("Setup complete");
}

/*
 * The main loop
 */
int i = 0;

void loop() 
{
  
  //Writing to the slave
  if( (millis()-lastPoll) > pollInterval)
  {
    Wire.beginTransmission(I2C_SLAVE_ADDR);
    Wire.write(0x01);//Register to start at
    switch(i)
    {
      case 0:
        Wire.write(255);
        Wire.write(0);
        Wire.write(0);
        i++;
        break;
      case 1:
        Wire.write(0);
        Wire.write(255);
        Wire.write(0);
        i++;
        break;
      case 2:
        Wire.write(0);
        Wire.write(0);
        Wire.write(255);
        i = 0;
        break;
    }
    Wire.endTransmission();

    delay(1);//Dont let the slave panic
    
    //Set the register pointer back to 0x01, preparing for a read
    Wire.beginTransmission(I2C_SLAVE_ADDR);
    Wire.write(0x00);//Register to start at
    Wire.endTransmission();
    delay(1);//Dont let the slave panic

    //Get values from the three registers up from 0x01
    Wire.requestFrom(I2C_SLAVE_ADDR, 4);//Request N bytes
    while (Wire.available())
    {
      uint8_t next_byte = Wire.read();
      Serial.print(next_byte);Serial.print(" ");    
    }
    Serial.println("\n");
    
    lastPoll = millis();
  }//End if time to poll again
  
}//End loop

Slave - ATTiny85

#include <EEPROM.h>
#include <TinyWireS.h>
#include <Bounce2.h>
#include <WS2812.h>
#ifdef __AVR__ //Which will be true for ATtiny85
  #include <avr/power.h>
#endif

#define I2C_SLAVE_DEFAULT_ADDR 0x05
#define BUTTON_DEBOUNCE 5//Debounce milliseconds

#define NEOPIXEL_PIN  1
#define BUTTON_PIN    3

#define NUMPIXELS     1

/*
 * I2C Registers
 * 
 * Register map:
 * 0x00 - Button state
 * 0x01 - led value red
 * 0x02 - led value green
 * 0x03 - led value blue
 * 
 * Total size: 4
 */
const byte reg_size = 4;
volatile uint16_t i2c_regs[reg_size];

/*
 * Internal variables
 */
cRGB value;
volatile boolean led_needs_update = false;
volatile byte reg_position;

/*
 * Initialize instances/classes
 */
Bounce button = Bounce();
WS2812 led(NUMPIXELS);

void setup() 
{
  //Start I2C
  //uint8_t _device_addr = EEPROM_DATA::get_device_addr();
  TinyWireS.begin(I2C_SLAVE_DEFAULT_ADDR);
  TinyWireS.onReceive(i2cReceiveEvent);
  TinyWireS.onRequest(i2cRequestEvent);

  //Start Led
  led.setOutput(NEOPIXEL_PIN);
  value.b = 255; value.g = 0; value.r = 0;
  led.set_crgb_at(0, value); //Set value at LED found at index 0
  led.sync(); // Sends the value to the LED

  //Start Button
  pinMode(BUTTON_PIN, INPUT_PULLUP);
  button.attach(BUTTON_PIN);
  button.interval(BUTTON_DEBOUNCE);

}

void loop() 
{
  button.update();

  if(led_needs_update)
  {
    led_update();
    led_needs_update = false;
  }
 
  if(button.fell())
  {
    i2c_regs[0x00] = true;
  }
  if(button.rose())
  {
    i2c_regs[0x00] = false;
  }
  
  // This needs to be here for the TinyWireS lib
  TinyWireS_stop_check();
  
}

/*
 * I2C Handelers
 */
void i2cReceiveEvent(uint8_t howMany)
{
    if (howMany < 1)
    {
        return;// Sanity-check
    }

    reg_position = TinyWireS.receive();
    howMany--;
    if (!howMany)
    {
        return;// This write was only to set the buffer for next read
    }
    
    while(howMany--)
    {
        //Store the recieved data in the currently selected register
        i2c_regs[reg_position] = TinyWireS.receive();
        
        //Proceed to the next register
        reg_position++;
        if (reg_position >= reg_size)
        {
            reg_position = 0;
        }
    }
    led_needs_update = true;
}//End i2cReceiveEvent()

void i2cRequestEvent()
{
    //Send the value on the current register position
    TinyWireS.send(i2c_regs[reg_position]);
    
    //WORKAROUND -> send all bytes, starting from the current registers
    /*int n = reg_size - reg_position;//Number of registers to return
    for(int i = reg_position; i < n; i++)
    {//Return all bytes from the reg_position to the end
      TinyWireS.send(i2c_regs[i]);
    }*/

    // Increment the reg position on each read, and loop back to zero
    reg_position++;
    if (reg_position >= reg_size)
    {
        reg_position = 0;
    } 
}//End i2cRequestEvent

/*
 * Helper functions
 */
void led_update()
{
  cRGB val;
  val.r = i2c_regs[0x01];
  val.g = i2c_regs[0x02];
  val.b = i2c_regs[0x03];
  led.set_crgb_at(0, val);
  led.sync(); // Sends the value to the LED
}

[rambo]

You code looks correct to me on the first look (apart from the i2c_regs being declared as uint16_t (it should of course be uint8_t since we're transferring single byte at a time, but that should not really affect anything but efficiency)

Do you have a logic analyser to trace the actual signals on the I2C bus ?

[dhunink]

Thanks for your swift response!
I do not own a logic analyzer I’m afraid. Up to this pint I never considered needing one. What is it you would like to be messeaured with it? Perhaps I can find other ways to debug...

[Koepel]

@rambo The master is requesting 4 bytes and the slave should send 4 bytes. Is it possible to use TinyWireS.send four times in the request handler ?

[dhunink]

@Koepel that’s something that can be done. It did it by sending all bytes from the current register up to the last register. It’s no problem for the slave to send more bytes then requested. But for me, although it is working, it still feels like a workaround that should be avoided and therefore I opened this issue.

I've added the workaround code to the initial post:

//WORKAROUND
    /*int n = reg_size - reg_position;//Number of registers to return
    for(int i = reg_position; i < n; i++)
    {//Return all bytes from the reg_position to the end
      TinyWireS.send(i2c_regs[i]);
    }*/

[Koepel]

I mean not calling the request handler 4 times, but sending 4 bytes in the request handler. If that is working, that I don't understand what the problem is.

A slave can always send more that the master requested. It is even valid. Suppose the master requests 1 byte or more, up to 8 bytes. The slave does not know how much data the master wants and can put all 8 bytes in the buffer. That is okay and that is normal. It is a result of how i2c works.

When the master requests data, the master defines how much bytes are transferred (from the slave to the master). The slave has no clue. That is i2c.

[rambo]

Note that like in Wire .send() just puts stuff to the send-buffer, the data is actually sent when master starts clocking data bits. There should be a requestEvent for each byte the master is clocking for (the first one comes right after the slave read-address, then once every byte the master is clocking, master never tells slave beforehand how many bytes it wants it just gives the slave read-address and starts clocking bits until it signals a STOP condition (or REPEATED START).

If for each requestEvent you put 4 bytes to the send buffer you will soon find yourself running out of buffer space since you're putting 4 times as much data to the buffer as is actually getting clocked out. The code will probably block when buffer is full (or it might overrun old values [ring buffer] and super weird bugs ensue)

if you know for certain that after each write there will be X reads, you can in receiveEvent push the bytes the master is going to read to the send-buffer, no need to receiveEvent at all then, but should the master ever for any reason request less or more bytes -> super weird bugs.

[Koepel]

I see. Thanks. I will add a link to your answer in this list: https://github.com/Testato/SoftwareWire/wiki/Arduino-I2C-libraries

@dhunink's problem is still not fixed I think.

[JasperHorn]

There should be a requestEvent for each byte the master is clocking for

Are you sure about that? When I run my code, the event handler only seems to be called once rather than for each byte. Reading usiTwiSlave.c brings me to the same conclusion: the callback is only called once, which is when the address is found in combination with a read bit. Even the examples don't do what you're saying.

[rambo]

There should be a requestEvent for each byte the master is clocking for

Are you sure about that?

Not anymore...

When in doubt, use the source (Luke)

https://github.com/rambo/TinyWire/blob/master/TinyWireS/TinyWireS.cpp#L51
https://github.com/rambo/TinyWire/blob/master/TinyWireS/usiTwiSlave.c#L251
https://github.com/rambo/TinyWire/blob/master/TinyWireS/usiTwiSlave.c#L271 (wait, what...)
https://github.com/rambo/TinyWire/blob/master/TinyWireS/usiTwiSlave.c#L480 (more weird)
which is only called here https://github.com/rambo/TinyWire/blob/master/TinyWireS/usiTwiSlave.c#L576

Short version is that I'm now super-confused about what's actually going on, would have to take a long hard look at the history of changes to that file, someone (maybe even past me) has made some probably misguided changes...

I'm not going to have time for that deep-dive in the foreseeable future though.

[JasperHorn]

Looks to me like this is the offending commit: a503849#diff-4fb58037efdfa694d9867fafad8efe2a

In fact, what that commit does is exactly that: change the callback from being called every byte to it being called only once per request (and add a test that relies on this behavior).

It even says so in the commit message: "If you move USI_REQUEST_CALLBACK() to line 583, then the callback is called only once per master request (when the request is initially received)."

[JasperHorn]

Alright, with that change undone, I've got it working locally (based on the interrupt_driven branch, though). I am actually able to send messages longer than the buffer now (which was impossible before).

I'll see if I can send three pull requests your way coming weekend:

1. Undoing the change that changed the behavior away from the intended behavior
2. My fixed interrupt_driven branch
3. Some changes I thought up to make the library more flexible and less error prone (including changes that I think will actually achieve what a503849 intended without breaking the "event per byte" workflow)

[dersimn]

Surprise, as of f0eb04f the communication doesn't work anymore.

Master (Arduino Micro):

#include <Wire.h>

void setup() {
  Wire.begin();
  Serial.begin(9600);
}

void loop() {
  Wire.requestFrom(10, 4);    // request 4 bytes

  while (Wire.available()) {
    uint8_t c = Wire.read();
    Serial.print(c, HEX);
  }
  Serial.println();

  delay(500);
}

Slave (Digispark USB Board):

#include <TinyWireS.h>

byte own_address = 10;

volatile uint8_t i2c_regs[] =
{
  0xDE, 
  0xAD, 
  0xBE, 
  0xEF, 
};
volatile byte reg_position = 0;
const byte reg_size = sizeof(i2c_regs);

void setup() {
	TinyWireS.begin( own_address );
	TinyWireS.onRequest( onI2CRequest );
  TinyWireS.onReceive( onI2CReceive );
}

void loop() {

}

void onI2CRequest() {
  TinyWireS.send(i2c_regs[reg_position]);
  reg_position++;
  if (reg_position >= reg_size)
  {
    reg_position = 0;
  }
}

void onI2CReceive(uint8_t howMany) {
  if (howMany < 1)
  {
    return;
  }
  if (howMany > reg_size+1)
  {
    return;
  }

  reg_position = TinyWireS.receive();
  howMany--;
  if (!howMany)
  {
    return;
  }
  while(howMany--)
  {
    i2c_regs[reg_position] = TinyWireS.receive();
    reg_position++;
    if (reg_position >= reg_size)
    {
      reg_position = 0;
    }
  }
}

Output with most recent commit (returns always 255 aka "nothing"):

Output with f0eb04f (returns first byte correctly, the remaining bytes are wrong):

[skozmin83]

I completely agree with @dersimn, f0eb04f commit broke simplest case with non-i2c-hardware supported devices. I have wemos d1 (esp8266) talking to attiny85. All esp8266 does: reads 1 byte from attiny85, requestEvent method on attiny85 is never getting called and from the code & saleae logs I can see it always returns 255, NACK (can provide SALEAE data if needed).