TwoWire calls OnReceive() more then once for the same data

[andrewwatkin]

twi.c slave IRQ handler can call the receive callback more than once for the same data. This confuses the sketch code.

Use case 1
Reading from the slave, preceded by an write (which is the instruction as to what to read). I.e.

• master writes a read request to the slave
• master reads an initial answer (say the amount of data to be sent back to the master)
• master reads that number (i.e. determined by the slave) of data bytes from the slave.

Use case 1A
Reading from the slave, preceded by an write (which is the instruction as to what to read). I.e.

• master writes a read request to the slave
• master reads a predetermined number (i.e. implicit in the request) of data bytes from the slave.

Use case 2
Writing to the slave without reading. I.e.

• master writes to the slave.
• master does not read from the slave.

My code contains use cases 1 and 2. I haven't tried use case 1A but I can't see any reason why it should be materially different from use case 1.

In twi.c slave IRQ handler:

When a REPSTART occurs when reading, user_onReceive() is invoked in order that the slave can process the command so that the slave can generate the reply. Needed for use case 1 (and 1A).

When STOP occurs at the end of the transaction, user_onReceive() is invoked in order that the slave can process the command. Needed for use case 2.

The problem is that in use case 1 (and 1A), both REPSTART and STOP occur in the transaction, so user_onReceive() is invoked again on the same data. But the master doesn't know this, so doesn't do the expected read - which isn't wanted anyway, nor is processing the command a second time on the slave.

My solution is that the receive buffer should be emptied after every user_onReceive() call. I've done this by adding (*rxHead) = 0; (*rxTail) = 0; in twi.c where user_onReceive() is called on REPSTART in the slave IRQ handler around line 566 as follows:

  if (clientStatus & TWI_APIF_bm) {  // Address/Stop Bit set
    if (clientStatus & TWI_AP_bm) {    // Address bit set
      uint8_t payload = _data->_module->SDATA;  // read address from data register
      if (clientStatus & TWI_DIR_bm) {  // Master is reading
        if ((*rxHead) > 0) {                    // There is no way to identify a REPSTART,
          popSleep();                           // (have to treat REPSTART as another pop for sleep)
          if (_data->user_onReceive != NULL) {  // so when a Master Read occurs after a Master write
            _data->user_onReceive((*rxHead));   // issue a call to the user callback first
            (*rxHead) = 0;
            (*rxTail) = 0;
          }
        }
        (*address) = payload;                   // saving address to expose to the user sketch
        (*txHead) = 0;                          // reset buffer positions so the Master can start writing at zero.
        (*txTail) = 0;

The receive buffer is already emptied after the user_onReceive() call on STOP:

    } else {                            // Stop bit set
      popSleep();
      if (_data->user_onReceive != NULL) {
        if ((*rxHead) > 0) {
          _data->user_onReceive((*rxHead));
        }
      }
      action = TWI_SCMD_COMPTRANS_gc;  // "Wait for any Start (S/Sr) condition"
      (*rxHead) = 0;
      (*txHead) = 0;
      (*rxTail) = 0;
      (*txTail) = 0;
    }

[MX682X]

I see where the problem comes from.
However, I've already prepared a new Wire version (PR), that does not seperate Transmit and Receive buffers (as there is just one possibility at a time anyway), so it shouldn't be a problem anymore. I would appreciate if you could check if the bug still persists though.

Apparantly Spence is pretty busy right now, so it might take some time for it to be merged in.
The updated Wire source files can be found here: https://github.com/MX682X/megaTinyCore/tree/master/megaavr/libraries/Wire/src

[andrewwatkin]

Hi MX682X

Issue 1
I think having a single buffer is great and the code is much simplified has a result. However there is still an issue with REPSTART.

Every path through TwoWire::HandleSlaveIRQ() when the address / STOP bit is set, results in the buffer being reset. This will ensure that user_onReceive() is only called once per group of bytes (i.e. all the bytes following a single address frame).

EXCEPT if STOP doesn't happen as in a REPSTART. In this case the buffer is left populated and will trigger a user_onReceive() call on the next address received.

Consider

• master sends START + write. Slave fills the buffer from the wire
• master sends STOP. Slave calls user_onReceive() and then resets the buffer
• master sends REPSTART + read request. Slave buffer head == 0, so, correctly, doesn't call user_onReceive(). Slave calls user_onRequest() which fills the buffer with data from the slave
• slave sends the buffer. When finished tail == head and both are >0
• master sends REPSTART + write. At this point, the slave's buffer still contains the data from the previous request. The first thing that happens is a test for a non-empty buffer and then call user_onReceive(). BAD.

Had the master sent STOP after receiving the requested data from the slave, the slave would have reset the buffer and all would have ben well in the next request (which would have been START rather than REPSTART).

[I know that REPART and START are the same thing on the wire but I've distinguished them in the above description to try and be clear what is happening]

A possible fix might be to add *head = 0; *tail = 0; to the Data bit set / master is reading / RXACK bit not set / no data available code path - i.e. where the action is "Wait for any Start (S/Sr) condition" following the completion of the read by the master around line 1365. See "ADDED" below:

  } else if (clientStatus & TWI_DIF_bm) { // Data bit set
    if (clientStatus & TWI_DIR_bm) {        // Master is reading
      if (clientStatus & wire_s->client_irq_mask) {   // If a collision was detected, or RXACK bit is set (when it matters)
        (*head) = 0;                            // Abort further data writes
        wire_s->client_irq_mask = TWI_COLL_bm;  // stop checking for NACK
        action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
      } else {                                // RXACK bit not set, no COLL
        wire_s->_bytesTransmittedS++;            // increment bytes transmitted counter (for register model)
        wire_s->client_irq_mask = TWI_COLL_bm | TWI_RXACK_bm;  // start checking for NACK
        if ((*tail) < (*head)) {                // Data is available
          wire_s->_module->SDATA = buffer[(*tail)];  // Writing to the register to send data
          (*tail)++;                              // Increment counter for sent bytes
          action = TWI_SCMD_RESPONSE_gc;          // "Execute a byte read operation followed by Acknowledge Action"
        } else {                                  // No more data available
          action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
         *head = 0; // ADDED
         *tail = 0;   // ADDED
        }
      }
    } else {                                  // Master is writing

Issue 2

I also think that pushSleep() / popSleep() are unsafe - this is so for both the old and new code.

If there was an error on the physical wire such that a STOP was missed, the sleepStack top nibble would be >0 forever.
As there are only 2 possible sources of sleep (TWI0 and TWI1) we can do better by simply tracking those transaction sources with a single bit each rather than maintaining a reference count that does not know the sources of the references.

I.e. set (sleepStack & 0x10) for TWI0 in a transaction and set (sleepStack & 0x20) for TWI1 in a transaction.

So push and pop need to know which interface is being handled:
START:

    if (clientStatus & TWI_AP_bm) {     // Address bit set
      if ((*head) == 0) {                 // only if there was no data (START)
#if defined(TWI1) 
        pushSleep(&TWI1 == _data->_module);                      // push the sleep
#else
        pushSleep();                      // push the sleep
#endif
      }

STOP:

    } else {                          // Stop bit set
#if defined(TWI1) 
        popSleep(&TWI1 == _data->_module);
#else
        popSleep();
#endif

      (*head) = 0;                    // clear whatever might be left due errors
      (*tail) = 0;
      action = TWI_SCMD_COMPTRANS_gc;  // "Wait for any Start (S/Sr) condition"
    }

And then we have to handle the combinations of interfaces using bitwise logic rather than arithmetic:

/**
 *@brief      pushSleep and popSleep handle the sleep guard
 *
 *            When used only by one peripheral, just saving the sleep register is plenty,
 *              But when used by more then one, special care must be taken to restore the
 *              sleep settings only at the end.
 *              e.g. when TWI0 - START, TWI1 - START, TWI0 - STOP, TWI1 - STOP
 *              so
 *                bit 4 is set while TWI0 is active
 *                bit 5 is set while TWI1 is active
 *              push limits sleeping if no interfaces were active before the call.
 *              pop restores sleeping if there will be no interfaces active after the call.
 *              This tolerates any errors of missing START or STOP and any number of REPSTART.
 *
 *@param      none
 *
 *@return     void
 */
#if defined(TWI1) 
void pushSleep(bool twi1) {
#else
void pushSleep() {
#endif
  uint8_t sleepStackLoc = sleepStack;
  if (sleepStackLoc == 0) {               // Limit sleep only if neither interface was active before
    sleepStackLoc = SLPCTRL.CTRLA;        // save sleep settings to sleepStack
    SLPCTRL.CTRLA = sleepStackLoc & 0x01; // Set to IDLE if sleep was enabled
  }
  #if defined(TWI_USING_WIRE1)
    if (sleepStackLoc > 0) { // only record the interface if sleep has been limited by (either) TWI
      sleepStackLoc |= twi1 ? 0x20 : 0x10 // Set the active flag for this interface
    }
  #endif
  sleepStack = sleepStackLoc;
}

#if defined(TWI1) 
void popSleep(bool twi1) {
#else
void popSleep() {
#endif
  uint8_t sleepStackLoc = sleepStack;
  if (sleepStackLoc > 0) {      // only do something if sleep has been limited by (either) TWI
    #if defined(TWI_USING_WIRE1)
      sleepStackLoc &= twi1 ? ~0x20 : ~0x10; // Clear the active flag for this interface
      if (sleepStackLoc & 0x30 == 0) { // as neither interface is active we are about to put sleep back
        SLPCTRL.CTRLA = sleepStackLoc;  // restore sleep
        sleepStackLoc = 0;              // reset everything
      }
    #else
      SLPCTRL.CTRLA = sleepStackLoc;
      sleepStackLoc = 0;
    #endif
  }
  sleepStack = sleepStackLoc;
}

Also FWIW, pushSleep is a confusing name; pushLimitSleep() would be a clearer statement of intent. popLimitSleep() similarly.

[MX682X]

A possible fix might be to add *head = 0; *tail = 0; to the Data bit set / master is reading / RXACK bit not set / no data available code path - i.e. where the action is "Wait for any Start (S/Sr) condition" following the completion of the read by the master around line 1365. See "ADDED" below:

This fix would break the START recognition that is used to push the sleep State, so I decided to set a Flag on Master Transmissions only. This way the code should only call onReceive after a preceeding Master Write START Frame.

Also FWIW, pushSleep is a confusing name; pushLimitSleep() would be a clearer statement of intent. popLimitSleep() similarly.

Subjectively, I find the "Limit" to be more confusing, so I won't change that yet

I'm kinda short on time nowadays, so I'm not able to set up a test bed to check the code. Consider the following a preview / suggestion.

Wire.h:

struct twiDataBools {         // using a struct so the compiler can use skip if bit is set/cleared
  bool _toggleStreamFn:   1;  // used to toggle between Slave and Master elements when TWI_MANDS defined
  bool _hostEnabled:      1;
  bool _clientEnabled:    1;
  bool _hostDataSent:     1;
  uint8_t _reserved       4;
};

void TwoWire::HandleSlaveIRQ(TwoWire *wire_s) {
  if (wire_s == NULL) {
    return;
  }

  uint8_t *address,  *buffer;
  twi_buf_index_t *head, *tail;
  #if defined(TWI_MANDS)
    address = &(wire_s->_incomingAddress);
    head    = &(wire_s->_bytesToReadWriteS);
    tail    = &(wire_s->_bytesReadWrittenS);
    buffer  =   wire_s->_clientBuffer;
  #else
    address = &(wire_s->_clientAddress);
    head    = &(wire_s->_bytesToReadWrite);
    tail    = &(wire_s->_bytesReadWritten);
    buffer  =   wire_s->_hostBuffer;
  #endif

  #if defined(TWI_MANDS)
    wire_s->_bools._toggleStreamFn = 0x01;
  #endif

  uint8_t action = 0;
  uint8_t clientStatus = wire_s->_module->SSTATUS;


  if (clientStatus & TWI_APIF_bm) {   // Address/Stop Bit set
    if (wire_s->_bools._hostDataSent != 0) { // At this point, we have either a START, REPSTART or a STOP
      wire_s->_bools._hostDataSent = 0x00;
      if (wire_s->user_onReceive != NULL) {   // only if the last APIF was a Master Write,
        wire_s->user_onReceive((*head));      // we notify the sketch about new Data
      }
    }
    if (clientStatus & TWI_AP_bm) {     // Address bit set
      if ((*head) == 0) {                 // only if there was no data (START)
        pushSleep((uint8_t)((uint16_t)wire_s->_module));  // push the sleep
      }
      (*head) = 0;
      (*tail) = 0;
      (*address) = wire_s->_module->SDATA;  // read address from data register
      if (clientStatus & TWI_DIR_bm) {      // Master is reading
        if (wire_s->user_onRequest != NULL) {
          wire_s->user_onRequest();
        }
        if ((*head) == 0) {                     // If no data to transmit, send NACK
          action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc;  // NACK + "Wait for any Start (S/Sr) condition"
        } else {
          action = TWI_SCMD_RESPONSE_gc;        // "Execute Acknowledge Action succeeded by reception of next byte"
        }
      } else {                          // Master is writing
        wire_s->_bools._hostDataSent = 0x01;
        action = TWI_SCMD_RESPONSE_gc;  // "Execute Acknowledge Action succeeded by reception of next byte"
      }
    } else {                          // Stop bit set
      popSleep((uint8_t)((uint16_t)wire_s->_module));
      (*head) = 0;
      (*tail) = 0;
      action = TWI_SCMD_COMPTRANS_gc;  // "Wait for any Start (S/Sr) condition"
    }
  } else if (clientStatus & TWI_DIF_bm) { // Data bit set
    if (clientStatus & TWI_DIR_bm) {        // Master is reading
      if (clientStatus & wire_s->client_irq_mask) {   // If a collision was detected, or RXACK bit is set (when it matters)
        wire_s->client_irq_mask = TWI_COLL_bm;  // stop checking for NACK
        (*head) = 0;                            // Abort further data writes
        action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
      } else {                                // RXACK bit not set, no COLL
        wire_s->_bytesTransmittedS++;            // increment bytes transmitted counter (for register model)
        wire_s->client_irq_mask = TWI_COLL_bm | TWI_RXACK_bm;  // start checking for NACK
        if ((*tail) < (*head)) {                // Data is available
          wire_s->_module->SDATA = buffer[(*tail)];  // Writing to the register to send data
          (*tail)++;                              // Increment counter for sent bytes
          action = TWI_SCMD_RESPONSE_gc;          // "Execute a byte read operation followed by Acknowledge Action"
        } else {                                  // No more data available
          (*head) = 0;                            // Avoid triggering REPSTART handler
          action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
        }
      }
    } else {                                  // Master is writing
      uint8_t payload = wire_s->_module->SDATA;     // reading SDATA will clear the DATA IRQ flag
      if ((*head) < TWI_BUFFER_LENGTH) {            // make sure that we don't have a buffer overflow in case Master ignores NACK
        buffer[(*head)] = payload;                  // save data
        (*head)++;                                  // Advance Head
        if ((*head) == TWI_BUFFER_LENGTH) {         // if buffer is not yet full
          action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc;  // "Execute ACK Action succeeded by waiting for any Start (S/Sr) condition"
        } else {                                    // else buffer would overflow with next byte
          action = TWI_SCMD_RESPONSE_gc;            // "Execute Acknowledge Action succeeded by reception of next byte"
        }
      }
    }
  }
  wire_s->_module->SCTRLB = action;  // using local variable (register) reduces the amount of loading _module
  #if defined(TWI_MANDS)
    wire_s->_bools._toggleStreamFn = 0x00;
  #endif
}

/**
 *@brief      pushSleep and popSleep handle the sleep guard
 *
 *            When used only by one peripheral, just saving the sleep register is plenty,
 *              But when used by more then one, special care must be taken to restore the
 *              sleep settings only at the end.
 *              e.g. when TWI0 - START, TWI1 - START, TWI0 - STOP, TWI1 - STOP
 *              using a bit mask in the upper nibble to check for the bus activity
 *
 *@param      uint8_t module_lower_Addr - lower byte of the TWI Module address
 *
 *@return     void
 */
void pushSleep(uint8_t module_lower_Addr) {
  #if defined(TWI_USING_WIRE1)
    uint8_t bit_mask = 0x10;
    if (module_lower_Addr == (uint8_t)((uint16_t)&TWI1)){
      bit_mask = 0x20;
    }
    uint8_t sleepStackLoc = sleepStack;
    if (sleepStackLoc == 0) {        // Save sleep state only if stack empty
      sleepStackLoc = SLPCTRL.CTRLA;        // save sleep settings to sleepStack
      SLPCTRL.CTRLA = sleepStackLoc & 0x01; // only leave the SEN bit, if it was set
    }
    sleepStackLoc |= bit_mask;      // Remember which module is busy
    sleepStack = sleepStackLoc;
  #else
    (void) module_lower_Addr;
    sleepStack = SLPCTRL.CTRLA;          // save old sleep State
    SLPCTRL.CTRLA = sleepStack & 0x01;   // only leave the SEN bit, if it was set
  #endif
}

void popSleep(uint8_t module_lower_Addr) {
  #if defined(TWI_USING_WIRE1)
    uint8_t bit_mask = ~0x10;
    if (module_lower_Addr == (uint8_t)((uint16_t)&TWI1)){
      bit_mask = ~0x20;
    }
    uint8_t sleepStackLoc = sleepStack;
    sleepStackLoc &= bit_mask;
    if (sleepStackLoc > 0) {      // only do something if sleep was enabled
      if (sleepStackLoc < 0x10) {   // if upper nibble is clear
        SLPCTRL.CTRLA = sleepStackLoc;  // restore sleep
        sleepStackLoc = 0;              // reset everything
      }
    }
    sleepStack = sleepStackLoc;
  #else
    (void) module_lower_Addr;
    SLPCTRL.CTRLA = sleepStack;
  #endif
}

[andrewwatkin]

Sleep management in the single TWI case still won't work. Consider the message sequence START, START, STOP. This will result in pushSleep(), pushSleep(), popSleep(). Therefore pushSleep() and popSleep() need to handle the case of when pushSleep() is called multiple times without intervening popSleep(). One way is to test sleepStack > 0 as is done for the dual TWI case.

The use of _hostDataSent seems conceptually correct. It does imply a trivial functional change in that a write from master with no data will now cause user_onReceive() to be called. This was not previously the case. Seems benign to me. Possibly it is actually more correct.

Note: This is only from code inspection - your preview / suggestion code is from a significantly different base so I haven't attempted to get it running - I'm sure it is possible but I would have to guess a few things when would largely invalidate the test.

[MX682X]

Alright, tested on 1614 now. Haven't tested the I2C Bus Fail logic though, it would require writing a I2C bit-bang library and that will take more time on that then i want to spend on it. Came up with some better named functions for push/pop Sleep, aswell as adding a guard fo double START.

void TwoWire::HandleSlaveIRQ(TwoWire *wire_s) {
  if (wire_s == NULL) {
    return;
  }

  uint8_t *address,  *buffer;
  twi_buf_index_t *head, *tail;
  #if defined(TWI_MANDS)
    address = &(wire_s->_incomingAddress);
    head    = &(wire_s->_bytesToReadWriteS);
    tail    = &(wire_s->_bytesReadWrittenS);
    buffer  =   wire_s->_clientBuffer;
  #else
    address = &(wire_s->_clientAddress);
    head    = &(wire_s->_bytesToReadWrite);
    tail    = &(wire_s->_bytesReadWritten);
    buffer  =   wire_s->_hostBuffer;
  #endif

  #if defined(TWI_MANDS)
    wire_s->_bools._toggleStreamFn = 0x01;
  #endif

  uint8_t action = 0;
  uint8_t clientStatus = wire_s->_module->SSTATUS;


  if (clientStatus & TWI_APIF_bm) {   // Address/Stop Bit set
    if (wire_s->_bools._hostDataSent != 0) { // At this point, we have either a START, REPSTART or a STOP
      if (wire_s->user_onReceive != NULL) {   // only if the last APIF was a Master Write,
        wire_s->user_onReceive((*head));      // we notify the sketch about new Data
      }
    }
    wire_s->_bools._hostDataSent = 0x00;
    if (clientStatus & TWI_AP_bm) {     // Address bit set
      if ((*head) == 0) {                 // only if there was no data (START)
        pauseDeepSleep((uint8_t)((uint16_t)wire_s->_module));  // Only START can wake from deep sleep, change to IDLE
      }
      (*head) = 0;
      (*tail) = 0;
      (*address) = wire_s->_module->SDATA;  // read address from data register
      if (clientStatus & TWI_DIR_bm) {      // Master is reading
        if (wire_s->user_onRequest != NULL) {
          wire_s->user_onRequest();
        }
        if ((*head) == 0) {                     // If no data to transmit, send NACK
          action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc;  // NACK + "Wait for any Start (S/Sr) condition"
        } else {
          action = TWI_SCMD_RESPONSE_gc;        // "Execute Acknowledge Action succeeded by reception of next byte"
        }
      } else {                          // Master is writing
        wire_s->_bools._hostDataSent = 0x01;
        action = TWI_SCMD_RESPONSE_gc;  // "Execute Acknowledge Action succeeded by slave data interrupt"
      }
    } else {                          // Stop bit set
      restoreSleep((uint8_t)((uint16_t)wire_s->_module));
      (*head) = 0;
      (*tail) = 0;
      action = TWI_SCMD_COMPTRANS_gc;  // "Wait for any Start (S/Sr) condition"
    }
  } else if (clientStatus & TWI_DIF_bm) { // Data bit set
    if (clientStatus & TWI_DIR_bm) {        // Master is reading
      if (clientStatus & wire_s->client_irq_mask) {   // If a collision was detected, or RXACK bit is set (when it matters)
        wire_s->client_irq_mask = TWI_COLL_bm;  // stop checking for NACK
        (*head) = 0;                            // Abort further data writes
        action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
      } else {                                // RXACK bit not set, no COLL
        wire_s->client_irq_mask = TWI_COLL_bm | TWI_RXACK_bm;  // start checking for NACK
        wire_s->_bytesTransmittedS++;           // increment bytes transmitted counter (for register model)
        if ((*tail) < (*head)) {                // Data is available
          wire_s->_module->SDATA = buffer[(*tail)];  // Writing to the register to send data
          (*tail)++;                              // Increment counter for sent bytes
          action = TWI_SCMD_RESPONSE_gc;          // "Execute Acknowledge Action succeeded by reception of next byte"
        } else {                                  // No more data available
          (*head) = 0;                            // Avoid triggering REPSTART handler
          action = TWI_SCMD_COMPTRANS_gc;         // "Wait for any Start (S/Sr) condition"
        }
      }
    } else {                                  // Master is writing
      uint8_t payload = wire_s->_module->SDATA;     // reading SDATA will clear the DATA IRQ flag
      if ((*head) < TWI_BUFFER_LENGTH) {            // make sure that we don't have a buffer overflow in case Master ignores NACK
        buffer[(*head)] = payload;                  // save data
        (*head)++;                                  // Advance Head
        if ((*head) == TWI_BUFFER_LENGTH) {         // if buffer is not yet full
          action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc;  // "Execute ACK Action succeeded by waiting for any Start (S/Sr) condition"
        } else {                                    // else buffer would overflow with next byte
          action = TWI_SCMD_RESPONSE_gc;            // "Execute Acknowledge Action succeeded by reception of next byte"
        }
      }
    }
  }
  wire_s->_module->SCTRLB = action;  // using local variable (register) reduces the amount of loading _module
  #if defined(TWI_MANDS)
    wire_s->_bools._toggleStreamFn = 0x00;
  #endif
}

void pauseDeepSleep(uint8_t module_lower_Addr) {
  #if defined(TWI_USING_WIRE1)
    uint8_t bit_mask = 0x10;
    if (module_lower_Addr == (uint8_t)((uint16_t)&TWI1)){
      bit_mask = 0x20;
    }
    uint8_t sleepStackLoc = sleepStack;
    if (sleepStackLoc == 0) {        // Save sleep state only if stack empty
      sleepStackLoc = SLPCTRL.CTRLA;        // save sleep settings to sleepStack
      SLPCTRL.CTRLA = sleepStackLoc & 0x01; // only leave the SEN bit, if it was set
    }
    sleepStackLoc |= bit_mask;      // Remember which module is busy
    sleepStack = sleepStackLoc;
  #else
    (void) module_lower_Addr;
    
    if (sleepStack == 0x00) {
      uint8_t slp = SLPCTRL.CTRLA;    // save current sleep State
      sleepStack = slp;               // using local variable for less store/load
      SLPCTRL.CTRLA = slp & 0x01;     // only leave the SEN bit, if it was set
    }
  #endif
}

void restoreSleep(uint8_t module_lower_Addr) {
  #if defined(TWI_USING_WIRE1)
    uint8_t bit_mask = ~0x10;
    if (module_lower_Addr == (uint8_t)((uint16_t)&TWI1)){
      bit_mask = ~0x20;
    }
    uint8_t sleepStackLoc = sleepStack;
    sleepStackLoc &= bit_mask;
    if (sleepStackLoc > 0) {      // only do something if sleep was enabled
      if (sleepStackLoc < 0x10) {   // if upper nibble is clear
        SLPCTRL.CTRLA = sleepStackLoc;  // restore sleep
        sleepStackLoc = 0;              // reset everything
      }
    }
    sleepStack = sleepStackLoc;
  #else
    (void) module_lower_Addr;
    SLPCTRL.CTRLA = sleepStack;
    sleepStack = 0;
  #endif
}

[andrewwatkin]

I think the test for *head==0 prior to calling pauseDeepSleep() is unnecessary; pauseDeepSleep() now protects itself from multiple consecutive calls for the same interface anyway.

As a minor speed optimisation the line wire_s->_bools._hostDataSent = 0x00; could be nested inside the preceding if (wire_s->_bools._hostDataSent != 0) block.

Apologies for my ignorance but how do I get the 1614 code?

[MX682X]

I think the test for *head==0 prior to calling pauseDeepSleep() is unnecessary; pauseDeepSleep() now protects itself from multiple consecutive calls for the same interface anyway.

I though about it aswell, but i'll keep it like that for now, I don't want to change too much in one go. Roll out, make sure there are no hidden/weird bugs, continue with optimization.

As a minor speed optimisation the line wire_s->_bools._hostDataSent = 0x00; could be nested inside the preceding if (wire_s->_bools._hostDataSent != 0) block.

Nice catch.

Apologies for my ignorance but how do I get the 1614 code?

If you mean the Code I used for testing, aka the Arduino Sketch, I can post it if you want.
If you need the Wire.cpp - It's just a drop in from the code you see above,, (the new function definitions for the sleep guarding have to be replaced manually... I guess I could add it to to the existing PR anyway.