From bed90a351bab5e7e08acb1e7bb2a170e2bb7c9fb Mon Sep 17 00:00:00 2001 From: MX682X Date: Sat, 25 May 2024 11:42:46 +0200 Subject: [PATCH] Fixing #1090 Summed up, made the Wire library more resistant to erroneous beahiour of the physical bus in Client/Slave mode --- megaavr/libraries/Wire/src/Wire.cpp | 86 ++++++++++++++++------------- megaavr/libraries/Wire/src/Wire.h | 5 +- 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/megaavr/libraries/Wire/src/Wire.cpp b/megaavr/libraries/Wire/src/Wire.cpp index 295bced2..5fa0bd57 100644 --- a/megaavr/libraries/Wire/src/Wire.cpp +++ b/megaavr/libraries/Wire/src/Wire.cpp @@ -40,10 +40,10 @@ extern "C" { // compiler was complaining when I put twi.h into the upper C in #include "twi_pins.h" } -static uint8_t sleepStack = 0; +volatile uint8_t sleepStack = 0; -void pushSleep(void); -void popSleep(void); +void pauseDeepSleep(uint8_t moduelAddr); +void restoreSleep(uint8_t moduelAddr); TwoWire* twi0_wire; TwoWire* twi1_wire; @@ -1288,7 +1288,6 @@ void TwoWire::HandleSlaveIRQ(TwoWire *wire_s) { return; } - uint8_t *address, *buffer; twi_buf_index_t *head, *tail; #if defined(TWI_MANDS) @@ -1312,22 +1311,21 @@ void TwoWire::HandleSlaveIRQ(TwoWire *wire_s) { if (clientStatus & TWI_APIF_bm) { // Address/Stop Bit set - - if ((*head) > 0) { // At this point, we have either a START, REPSTART or a STOP - if (wire_s->user_onReceive != NULL) { // at START, head should be 0, as it was set so on the last STOP - wire_s->user_onReceive((*head)); // otherwise, we notify the use sketch, as we have an REPSTART/STOP + 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(); // push the sleep + 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 - (*head) = 0; // reset buffer positions for user sketch - (*tail) = 0; - if (wire_s->user_onRequest != NULL) { wire_s->user_onRequest(); } @@ -1337,31 +1335,30 @@ void TwoWire::HandleSlaveIRQ(TwoWire *wire_s) { action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by reception of next byte" } } else { // Master is writing - (*head) = 0; // reset buffer positions - (*tail) = 0; - action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by reception of next byte" + wire_s->_bools._hostDataSent = 0x01; + action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by slave data interrupt" } } else { // Stop bit set - popSleep(); - - (*head) = 0; // clear whatever might be left due errors + 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) - (*head) = 0; // Abort further data writes 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->_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" + 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" } } @@ -1433,50 +1430,61 @@ ISR(TWI0_TWIS_vect) { /** - *@brief pushSleep and popSleep handle the sleep guard + *@brief pauseDeepSleep and restoreSleep 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. + * sleep settings only at the end by using a bitmask on the upper nibble. * e.g. when TWI0 - START, TWI1 - START, TWI0 - STOP, TWI1 - STOP - * so, there is a counter that counts up to 15 when pushing and down to 0 when - * popping. Only at 0, the actual push and pop happen. An overflow will lead to - * unpredictable results. * - *@param none + *@param uint8_t module_lower_Addr - lower byte of the TWI Module address + (used only with two Wire objects) * *@return void */ -void pushSleep() { +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) { // Increment only if sleep was enabled - sleepStackLoc = (sleepStackLoc + 0x10); // use upper nibble to count - max 15 pushes - } else { + if (sleepStackLoc == 0) { // Save sleep state only if stack empty sleepStackLoc = SLPCTRL.CTRLA; // save sleep settings to sleepStack - SLPCTRL.CTRLA = sleepStackLoc & 0x01; // Set to IDLE if sleep was enabled + SLPCTRL.CTRLA = sleepStackLoc & 0x01; // only leave the SEN bit, if it was set } + sleepStackLoc |= bit_mask; // Remember which module is busy sleepStack = sleepStackLoc; #else - sleepStack = SLPCTRL.CTRLA; // save old sleep State - SLPCTRL.CTRLA = sleepStack & 0x01; // only leave the SEN bit, if it was set + (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 popSleep() { +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) { // only decrement if pushed once before - sleepStackLoc = (sleepStackLoc - 0x10); // upper nibble - } else { // at 0 we are about to put sleep back + if (sleepStackLoc < 0x10) { // if upper nibble is clear SLPCTRL.CTRLA = sleepStackLoc; // restore sleep sleepStackLoc = 0; // reset everything } - sleepStack = sleepStackLoc; } + sleepStack = sleepStackLoc; #else + (void) module_lower_Addr; SLPCTRL.CTRLA = sleepStack; + sleepStack = 0; #endif } diff --git a/megaavr/libraries/Wire/src/Wire.h b/megaavr/libraries/Wire/src/Wire.h index 1e9e199d..13311183 100644 --- a/megaavr/libraries/Wire/src/Wire.h +++ b/megaavr/libraries/Wire/src/Wire.h @@ -197,13 +197,12 @@ struct twiDataBools { // using a struct so the compiler can use skip if bool _toggleStreamFn: 1; // used to toggle between Slave and Master elements when TWI_MANDS defined bool _hostEnabled: 1; bool _clientEnabled: 1; - uint8_t _reserved: 5; + bool _hostDataSent: 1; + uint8_t _reserved: 4; }; - - class TwoWire: public Stream { private: TWI_t *_module;