Slave send more than one byte in response

[brendanarnold]

It does not appear possible to send more than one byte in response to a request as a slave since send() should only be called once per request.

If I have a number greater than 256 that I need to send, what is the best way to do this?

I could make the device spec that the call should be made twice to get the upper and lower byte but this seems a little ugly.

[rambo]

Having the upper and the lower byte in separate "registers" is exactly the way it's done in all I2C devices. In any case you will want to have an array for the I2C registers and put your data (or read your commands) from there, and have the I2C callbacks only to work with that array.

See https://github.com/rambo/TinyWire/blob/master/TinyWireS/examples/attiny85_i2c_analog/attiny85_i2c_analog.ino

[brendanarnold]

Is there a way though to send all the values in those registers in one go? I am trying to do something similar to this example ...

https://www.arduino.cc/en/Tutorial/MasterReader

In this 6 bytes are requested from the slave, the slave then send 6 bytes in one request

[rambo]

That example looks seriously wrong to me, unless they completely changed the API when I wasn't looking requestEvent is called every time the master does a read, so that example would buffer "hello" to the I2C send buffer 6 times (at some point it would simply hang when the buffer fills) But the "Wire" API is horrible in many other ways as well, look at this I2C master implementation https://github.com/rambo/I2C

Wire.requestFrom(2, 6); is just a shorter way to write [ 2 r r r r r r ] ( using Bus-Pirate semantics, [=START r=READ ]=STOP )

Also look at this example where 16-bit ints are read from the accelerometer https://github.com/rambo/I2C/blob/master/examples/HMC5883L/HMC5883L.ino

[brendanarnold]

Ok, I can use registers.

I was a bit reluctant since it seems more stateful and potentially prone to sync problems e.g.

• I have a 6 byte message to return from a slave
• The master calls my slave code 6 times but one is dropped because noise/whatever
• The slave will now deliver future requests off by one

I could though have a maintenance routine that resets the pointer when idle somehow to avoid this occurring.

[rambo]

If you use the convention that first write from master is the register address (like every other I2C device with more than one register ever) then the master can always do [ 3 0 [ 2 r r r r r r ] (actually device with 7-bit address of 2 is 4 and 5 as 8-bit address with the R/W bit) or it can request just a particular two-byte nibble or something.

Most importantly it works just like any other I2C device without any magic pointer resets etc.

Remember that the slave can only ever send anything when the master is explicitly requesting stuff, since master takes care of the clocking there is no way the slave can ever send anything to the master the master is not requesting and the protocol only deals in bytes. It's a bad idea to pre-emptively queue more bytes to the send buffer than we see the master requesting (because there might be some reason the transfer is cut short on master end, then next time you send whatever was in the buffer and then queue more data there and suddenly everything is off by random amounts).

[stickbreaker]

I have looked at the TinyWireS.Send() function, which calls void usiTwiTransmitByte( uint8_t data);, this function will buffer up to TWI_TX_BUFFER_SIZE before it hangs. Why would you think it could only send 1 byte?

Chuck.

[rambo]

I said it's a bad idea to queue more than one byte per requestevent. That callback is called each time the master requests a byte so if you queue more than one in response you will soon fill your buffer and then everything will hang.

Of course if you're not using the callback and are simply buffering stuff every once in a while and trusting the master issue reads regularly to clear that buffer you have no problem (as long as all your assumptions hold true).

But if you want your attiny to act like just about every other I2C device out there (it's not a standard and even standards can be broken for a reason, just a convention almost everyone follows), then you have something representing registers and support the usual access conventions (first write in transaction is the register address, auto-increment registers on reads/writes etc) and only ever queue anything on the transmit buffer if the master requests for it (which it can only do one byte at a time).

tldr: it's not about what's possible to do, but what one should do, especially if one wants to avoid weird bugs.

edit "Remember that the slave can only ever send anything when the master is explicitly requesting stuff" part: as in on the actual I2C protocol level, you can of course queue stuff to be sent whenever until you run out of buffer, but it's not a good idea.

[stickbreaker]

ok, I agree with your preference to multiple calls of onRequestEvent(), the default Wire library will only call onRequest event once per requestFrom() cycle. I modified Wire to support the multiple requestEvent() calls. I did not fully review the TinyWireS to Identify if it had this same single call structure. I assumed, and of course is made an ass of me. So with TinyWireS, as long as a STOP or Repeated-START is not detected on the next clock the library calls onRequestEvent()? the Wire simple mod I did to Wire calls it when the buffer is empty.
Of course the Master control how many bytes are clocked out, and anything in the buffer is discarded. Is there any explicit descriptions of either behavior for the libraries? Or are we left to infer actions?

Have you contemplated using user supplied buffer for endTransmission() and requestFrom()? (master coding).
How about changing onRequestEvent( bool firstRequest); firstRequest could be set by a START, a Repeated-START would be false, as would all secondary onRequestEvent()'s . Some I2C devices use write(set addrptr) restart(read from adrptr), or start(read from 0);

chuck.

[rambo]

the default Wire library will only call onRequest event once per requestFrom() cycle.

Huh ? I need to re-read the code... But how would it even be possible for (even) stock Wire in slave mode on arduino proper (which has full HW support for I2C) to know how many bytes the master is going to request (without time travel) ? We can know how many bytes the master requested after the last read, but then it's too late to send the correct bytes in response...

There is no need to look for STOP or START, the master replies with NACK to the byte if this was the last one it wanted to read. I have not doublechecked this but I think that when we are the selected slave in read mode, as soon as master starts clocking tinywire asserts clock-stretch, calls the requestEvent callback (if any), releases the clock and then sends the next byte in the tx buffer.

Also I don't think the buffers are ever automatically discarded, in fact when I was first trying tinywires out I managed to mess up in exactly this way, queuing multiple bytes on each requestevent...

as for the suggested change, feel free to make a patch, just make sure it doesn't waste too many cycles to do the checking (maybe even put the whole feature behind a macro), there's a whole bunch of "this is time critical" comments in the tinywires source...

[stickbreaker]

@rambo >>Huh ? I need to re-read the code... But how would it even be possible for (even) stock Wire in slave on arduino proper (which has full HW support for I2C) mode to know how many bytes the master is going to request (without time travel) ? We can know how many bytes the master requested after the last read, but then it's too late to send the correct bytes in response...

The stock Wire library is poorly written. Check out the state machine in the Switch/Case statements of the ISR. When the START -Slave id is acknowledge it executes the onRequest callback, then it switches into transmit mode, never again calling onRequest. and to top it off, during the OnRequest callback, all Wire.write() calls reinitialized the txBuffer, effectively overwriting all previous calls to write(). I create a modified version that changes this behavior, and also times out on a SCL LOW bus failure. The release version locks up in a while(SCL Low){} that will hang forever. (both endTransmission, requestFrom)

@rambo >>as for the suggested change, feel free to make a patch, just make sure it doesn't waste too many cycles to do the checking (maybe even put the whole feature behind a macro), there's a whole bunch of "this is time critical" comments in the tinywires source...

I haven't used any of the Tiny processors yet, I was just researching the limitations to help someone on the Arduino network/I2C/SPI forum. I was curious about how the TinyWireS implemented onRequest. I was hoping it did not have the same bad coding. I assumed it also followed the single write() call 'feature' of the Uno,Mega library. You wrote it correctly to issue the callback for each byte the Master requests. That is good.

I'm going to push that 'correct' behavior into my modified Wire library. I will also add the Start/ReStart flag, and user buffers, the normal library uses 5 x 32byte buffers, twiWrite, twiRead,WireWrite,WireRead, twiOnReceive. I haven't figured out why all of these are needed, I had thought it might handle arbitration failover. I will have to study the code when I get time.

Thanks for the responses.

Chuck.

[joseluu]

I made a change for this purpose (send more than one byte to the master), as I needed to emulate such a device. Submiting a pull request right now.

[rambo]

Thanks for the PR (#17), I'll review it a bit later. Closing this issue now

[rshartog]

I’m fairly new to GitHub, so I’m not sure how feedback is accomplished. I found this thread on the topic, so I’m replying here. Hopefully this is the correct method and you find this feedback useful.

Summary: To allow the master to request more than one byte from the slave, I think that the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be on line 583.

Explanation: For the current code, the ISR routine correctly sends the execution through the “USI_SLAVE_SEND_DATA” case-branch for each byte sent from the slave to the master. With the USI_REQUEST_CALLBACK() on line 613, the callback routine is called for each byte that the master requests. Assuming the callback should queue the send data for the entire request (my assumption), then the current code generates N bytes of data for each byte sent (N^2 bytes total) and only removes N bytes from the transmit buffer. This works when N==1, but if N>1 this quickly results in a transmit buffer overflow and execution hangs on line 406. (This behavior was observed by having the slave respond with the values of txHead and txTail until the slave hung on line 406.)

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).

I made this change and I am able to read multiple bytes (3 in my application) from the slave reliably and repeatedly. Note that this change also works with a 1 byte request, so it should be backward compatible for applications already using usiTwiSlave for 1 byte requests from the slave.

--Scott

[joseluu]

Hello Scott,

looking at my proposed solution, I moved it a line 608: i.e. in the next case and outside the conditional, it's been a long time I did it, so now I can't judge if its equivalent/better/worse. If you are still at it, you may try it to see if it also solves your problem.

Best regards Jose

excerpt from code:

case USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA: if ( USIDR ) { // if NACK, the master does not want more data SET_USI_TO_TWI_START_CONDITION_MODE( ); txHead = txTail; //cleanup return; } USI_REQUEST_CALLBACK();// new position

On Wed, Feb 3, 2016 at 2:18 PM, rshartog notifications@github.com wrote:

I’m fairly new to GitHub, so I’m not sure how feedback is accomplished. I found this thread on the topic, so I’m replying here. Hopefully this is the correct method and you find this feedback useful.

Summary: To allow the master to request more than one byte from the slave, I think that the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be on line 583.

Explanation: For the current code, the ISR routine correctly sends the execution through the “USI_SLAVE_SEND_DATA” case-branch for each byte sent from the slave to the master. With the USI_REQUEST_CALLBACK() on line 613, the callback routine is called for each byte that the master requests. Assuming the callback should queue the send data for the entire request (my assumption), then the current code generates N bytes of data for each byte sent (N^2 bytes total) and only removes N bytes from the transmit buffer. This works when N==1, but if N>1 this quickly results in a transmit buffer overflow and execution hangs on line 406. (This behavior was observed by having the slave respond with the values of txHead and txTail until the slave hung on line 406.)

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).

I made this change and I am able to read multiple bytes (3 in my application) from the slave reliably and repeatedly. Note that this change also works with a 1 byte request, so it should be backward compatible for applications already using usiTwiSlave for 1 byte requests from the slave.

--Scott

— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/issues/12#issuecomment-179224355.

[rshartog]

Hi Jose,

Just to clarify (for me), are you creating a copy of USI_REQUEST_CALLBACK() at the new location or are you moving it to the new location? I assume the latter for my response below.

I'm not in a position to test your change at the moment, but just thinking through it I don't think it will fix the problem. I think putting the callback call at the end of the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch has two issues:

(1) The USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch is not executed until after the slave attempts to send a byte of data, but the callback itself is where the slave program sends data by calling the TinyWireS.send() routine which then calls the usiTwiTransmitByte() routine. I think this will result in the slave sending 0xff as the first byte. For reference, I think a master "requestFrom()" call for a quantity of three bytes (for example) causes the following sequence in the "overflowState" variable of the slave: USI_SLAVE_CHECK_ADDRESS (verify address and rx/tx direction) USI_SLAVE_SEND_DATA (1st byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (2nd byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (3rd byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (nothing sent, empty tx buffer causes read ack)

(2) As you can see above, I think the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch is called for every byte that the slave sends, so putting the USI_REQUEST_CALLBACK() call in the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch still creates the situation where the slave calls USI_REQUEST_CALLBACK() for each byte that the slave sends back to the master.

I think the correct position of the USI_REQUEST_CALLBACK() call is line 583 (or 584) in the USI_SLAVE_CHECK_ADDRESS case-branch as shown below...

case USI_SLAVE_CHECK_ADDRESS:
  if ( ( USIDR == 0 ) || ( ( USIDR >> 1 ) == slaveAddress) )
  {
     // callback
     if(_onTwiDataRequest) _onTwiDataRequest();
     if ( USIDR & 0x01 )
    {
      USI_REQUEST_CALLBACK(); // <--- here, only occurs once per master request
                                                        // and generates all the TinyWireS.send() calls for slave to send.
      overflowState = USI_SLAVE_SEND_DATA;
    }
    else
    {
      overflowState = USI_SLAVE_REQUEST_DATA;
    } // end if
    SET_USI_TO_SEND_ACK( );
  }
  else
  {
    SET_USI_TO_TWI_START_CONDITION_MODE( );
  }
  break;

One other related note: As written, the useable size of the rxBuf is actually TWI_RX_BUFFER_SIZE - 1. Same for txBuf/TWI_TX_BUFFER_SIZE. This is because the Head pointer cannot fully wrap back to the value of the Tail pointer. (Doing so would appear to be an empty buffer.) This detail might be important to someone who thinks they can use the entire buffer. When I code ring-buffers, I generally use a separate variable to track the count of information in the buffer. Then the head/tail pointers are not used to determine if the buffer is empty or full.

Hope this helps.

--Scott

[joseluu]

OK, thanks, this is good information, I will refer to it next time I have to deal with ATTiny I2C.

Cheers Jose

On Wed, Feb 3, 2016 at 8:32 PM, rshartog notifications@github.com wrote:

Hi Jose,

Just to clarify (for me), are you creating a copy of USI_REQUEST_CALLBACK() at the new location or are you moving it to the new location? I assume the latter for my response below.

I'm not in a position to test your change at the moment, but just thinking through it I don't think it will fix the problem. I think putting the callback call at the end of the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch has two issues:

(1) The USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch is not executed until after the slave attempts to send a byte of data, but the callback itself is where the slave program sends data by calling the TinyWireS.send() routine which then calls the usiTwiTransmitByte() routine. I think this will result in the slave sending 0xff as the first byte. For reference, I think a master "requestFrom()" call for a quantity of three bytes (for example) causes the following sequence in the "overflowState" variable of the slave: USI_SLAVE_CHECK_ADDRESS (verify address and rx/tx direction) USI_SLAVE_SEND_DATA (1st byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (2nd byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (3rd byte sent) USI_SLAVE_REQUEST_REPLY_FROM_SEND_DATA (I2C ack protocol) USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA (I2C ack protocol, always falls thru to next state) USI_SLAVE_SEND_DATA (nothing sent, empty tx buffer causes read ack)

(2) As you can see above, I think the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch is called for every byte that the slave sends, so putting the USI_REQUEST_CALLBACK() call in the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case-branch still creates the situation where the slave calls USI_REQUEST_CALLBACK() for each byte that the slave sends back to the master.

I think the correct position of the USI_REQUEST_CALLBACK() call is line 583 (or 584) in the USI_SLAVE_CHECK_ADDRESS case-branch as shown below...

case USI_SLAVE_CHECK_ADDRESS: if ( ( USIDR == 0 ) || ( ( USIDR >> 1 ) == slaveAddress) ) { // callback if(_onTwiDataRequest) _onTwiDataRequest(); if ( USIDR & 0x01 ) { USI_REQUEST_CALLBACK(); // <--- here, only occurs once per master request // and generates all the TinyWireS.send() calls for slave to send. overflowState = USI_SLAVE_SEND_DATA; } else { overflowState = USI_SLAVE_REQUEST_DATA; } // end if SET_USI_TO_SEND_ACK( ); } else { SET_USI_TO_TWI_START_CONDITION_MODE( ); } break;

One other related note: As written, the useable size of the rxBuf is actually TWI_RX_BUFFER_SIZE - 1. Same for txBuf/TWI_TX_BUFFER_SIZE. This is because the Head pointer cannot fully wrap back to the value of the Tail pointer. (Doing so would appear to be an empty buffer.) This detail might be important to someone who thinks they can use the entire buffer. When I code ring-buffers, I generally use a separate variable to track the count of information in the buffer. Then the head/tail pointers are not used to determine if the buffer is empty or full.

Hope this helps.

--Scott

— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/issues/12#issuecomment-179417939.

[rshartog]

Just to add some credibility...

I now have a master program using the regular Wire library running on regular Arduino (not a Attiny) that picks a random number of bytes between 1 and 12. It then sends that many bytes of random data to the Attiny slave within a single Wire.beginTransmission() / Wire.write() / Wire.endTransmission() set. The master program then reads that same number of bytes back with a single Wire.requestFrom() call and compares the received data to the originally transmitted data. The master program displays the number of requests, number of requests with mismatches, and enough of the data so that I can tell it's working.

The slave program using TinyWireS (with the change to usiTwiSlave.c that I mentioned above) running on a Attiny85 receives N bytes of data in a single receiveEvent() callback and stores that data in a global buffer. It then responds to requestEvent() callbacks with that same data. The requestEvent() callback overwrites the data buffer with zeros after responding so it will only respond correctly to the first requestEvent() callback after each receiveEvent() callback.

The two programs have been talking for about two hours. There have been 2380 requests (should be > 14,000 bytes sent and 14,000 received). There have been no mismatches. I also changed the slave program temporarily to prove to myself that the master program was detecting and recording errors correctly.

If you think you can use the master/slave programs, I'll send them. The master program currently displays to a serial (SPI) 128x64 OLED display, but it can be easily modified to output to the serial monitor.

--Scott

[rambo]

Cool, make a pull-request with the change and the test programs.

[rshartog]

I'll try to do the pull-request this weekend.