Commit b05430cf authored by chuck todd's avatar chuck todd Committed by Me No Dev

Wire ReSTART fix, with others (#1717)

* ReSTART fix, Sequencing fix

pr #1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill.  during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence.  The problem manifested as an ACK error. 
In addition to this required bug fix I propose:
* `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported.  Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure.
* `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus.  With the functional change to `Wire.begin()` this allows app level recover of a hung bus.
* `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()`  had no room to record the directed address bytes.  It generated just a NAK check without setting the EEPROMs internal register address.  The succeeding ReSTART read succeeded at incorrect address.
* Changes to the HAL layer:
** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently
** added `i2cDebug()` programmatic control of debug buffer output
** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers.
** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points 
 
*

* Update esp32-hal-i2c.c

* Update Wire.cpp

* ReSTART, Sequencing

pr #1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill.  during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence.  The problem manifested as an ACK error. 
In addition to this required bug fix I propose:
* `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported.  Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure.
* `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus.  With the functional change to `Wire.begin()` this allows app level recover of a hung bus.
* `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()`  had no room to record the directed address bytes.  It generated just a NAK check without setting the EEPROMs internal register address.  The succeeding ReSTART read succeeded at incorrect address.
* Changes to the HAL layer:
** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently
** added `i2cDebug()` programmatic control of debug buffer output
** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers.
** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points 
 
*

* Forgot DebugFlags Return

@andriyadi found this, total brain fade on my part.
parent e346f20a
This diff is collapsed.
...@@ -48,6 +48,7 @@ i2c_err_t i2cRead(i2c_t * i2c, uint16_t address, uint8_t* buff, uint16_t size, b ...@@ -48,6 +48,7 @@ i2c_err_t i2cRead(i2c_t * i2c, uint16_t address, uint8_t* buff, uint16_t size, b
i2c_err_t i2cFlush(i2c_t *i2c); i2c_err_t i2cFlush(i2c_t *i2c);
i2c_err_t i2cSetFrequency(i2c_t * i2c, uint32_t clk_speed); i2c_err_t i2cSetFrequency(i2c_t * i2c, uint32_t clk_speed);
uint32_t i2cGetFrequency(i2c_t * i2c); uint32_t i2cGetFrequency(i2c_t * i2c);
uint32_t i2cGetStatus(i2c_t * i2c); // Status register of peripheral
//Functions below should be used only if well understood //Functions below should be used only if well understood
//Might be deprecated and removed in future //Might be deprecated and removed in future
...@@ -62,8 +63,17 @@ i2c_err_t i2cAddQueueWrite(i2c_t *i2c, uint16_t i2cDeviceAddr, uint8_t *dataPtr, ...@@ -62,8 +63,17 @@ i2c_err_t i2cAddQueueWrite(i2c_t *i2c, uint16_t i2cDeviceAddr, uint8_t *dataPtr,
i2c_err_t i2cAddQueueRead(i2c_t *i2c, uint16_t i2cDeviceAddr, uint8_t *dataPtr, uint16_t dataLen, bool SendStop, EventGroupHandle_t event); i2c_err_t i2cAddQueueRead(i2c_t *i2c, uint16_t i2cDeviceAddr, uint8_t *dataPtr, uint16_t dataLen, bool SendStop, EventGroupHandle_t event);
//stickbreaker debug support //stickbreaker debug support
void i2cDumpInts(uint8_t num); uint32_t i2cDebug(i2c_t *, uint32_t setBits, uint32_t resetBits);
void i2cDumpI2c(i2c_t *i2c); // Debug actions have 3 currently defined locus
// 0xXX------ : at entry of ProcQueue
// 0x--XX---- : at exit of ProcQueue
// 0x------XX : at entry of Flush
//
// bit 0 causes DumpI2c to execute
// bit 1 causes DumpInts to execute
// bit 2 causes DumpCmdqueue to execute
// bit 3 causes DumpStatus to execute
// bit 4 causes DumpFifo to execute
#ifdef __cplusplus #ifdef __cplusplus
} }
......
...@@ -58,7 +58,7 @@ TwoWire::~TwoWire() ...@@ -58,7 +58,7 @@ TwoWire::~TwoWire()
} }
} }
void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency) bool TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency)
{ {
if(sdaPin < 0) { // default param passed if(sdaPin < 0) { // default param passed
if(num == 0) { if(num == 0) {
...@@ -70,7 +70,7 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency) ...@@ -70,7 +70,7 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency)
} else { } else {
if(sda==-1) { if(sda==-1) {
log_e("no Default SDA Pin for Second Peripheral"); log_e("no Default SDA Pin for Second Peripheral");
return; //no Default pin for Second Peripheral return false; //no Default pin for Second Peripheral
} else { } else {
sdaPin = sda; // reuse prior pin sdaPin = sda; // reuse prior pin
} }
...@@ -87,7 +87,7 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency) ...@@ -87,7 +87,7 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency)
} else { } else {
if(scl == -1) { if(scl == -1) {
log_e("no Default SCL Pin for Second Peripheral"); log_e("no Default SCL Pin for Second Peripheral");
return; //no Default pin for Second Peripheral return false; //no Default pin for Second Peripheral
} else { } else {
sclPin = scl; // reuse prior pin sclPin = scl; // reuse prior pin
} }
...@@ -98,10 +98,11 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency) ...@@ -98,10 +98,11 @@ void TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency)
scl = sclPin; scl = sclPin;
i2c = i2cInit(num, sdaPin, sclPin, frequency); i2c = i2cInit(num, sdaPin, sclPin, frequency);
if(!i2c) { if(!i2c) {
return; return false;
} }
flush(); flush();
return true;
} }
...@@ -145,6 +146,7 @@ void TwoWire::beginTransmission(uint16_t address) ...@@ -145,6 +146,7 @@ void TwoWire::beginTransmission(uint16_t address)
txAddress = address; txAddress = address;
txIndex = txQueued; // allow multiple beginTransmission(),write(),endTransmission(false) until endTransmission(true) txIndex = txQueued; // allow multiple beginTransmission(),write(),endTransmission(false) until endTransmission(true)
txLength = txQueued; txLength = txQueued;
last_error = I2C_ERROR_OK;
} }
/*stickbreaker isr /*stickbreaker isr
...@@ -202,6 +204,7 @@ size_t TwoWire::write(uint8_t data) ...@@ -202,6 +204,7 @@ size_t TwoWire::write(uint8_t data)
{ {
if(transmitting) { if(transmitting) {
if(txLength >= I2C_BUFFER_LENGTH) { if(txLength >= I2C_BUFFER_LENGTH) {
last_error = I2C_ERROR_MEMORY;
return 0; return 0;
} }
txBuffer[txIndex] = data; txBuffer[txIndex] = data;
...@@ -209,20 +212,19 @@ size_t TwoWire::write(uint8_t data) ...@@ -209,20 +212,19 @@ size_t TwoWire::write(uint8_t data)
txLength = txIndex; txLength = txIndex;
return 1; return 1;
} }
last_error = I2C_ERROR_NO_BEGIN; // no begin, not transmitting
return 0; return 0;
} }
size_t TwoWire::write(const uint8_t *data, size_t quantity) size_t TwoWire::write(const uint8_t *data, size_t quantity)
{ {
if(transmitting) { for(size_t i = 0; i < quantity; ++i) {
for(size_t i = 0; i < quantity; ++i) { if(!write(data[i])) {
if(!write(data[i])) { return i;
return i;
}
} }
return quantity;
} }
return 0; return quantity;
} }
int TwoWire::available(void) int TwoWire::available(void)
...@@ -353,14 +355,13 @@ char * TwoWire::getErrorText(uint8_t err) ...@@ -353,14 +355,13 @@ char * TwoWire::getErrorText(uint8_t err)
/*stickbreaker Dump i2c Interrupt buffer, i2c isr Debugging /*stickbreaker Dump i2c Interrupt buffer, i2c isr Debugging
*/ */
void TwoWire::dumpInts()
{ uint32_t TwoWire::setDebugFlags( uint32_t setBits, uint32_t resetBits){
i2cDumpInts(num); return i2cDebug(i2c,setBits,resetBits);
} }
void TwoWire::dumpI2C() bool TwoWire::busy(void){
{ return ((i2cGetStatus(i2c) & 16 )==16);
i2cDumpI2c(i2c);
} }
TwoWire Wire = TwoWire(0); TwoWire Wire = TwoWire(0);
......
...@@ -30,7 +30,7 @@ ...@@ -30,7 +30,7 @@
#include "freertos/queue.h" #include "freertos/queue.h"
#include "Stream.h" #include "Stream.h"
#define STICKBREAKER V0.2.2 #define STICKBREAKER V1.0.1
#define I2C_BUFFER_LENGTH 128 #define I2C_BUFFER_LENGTH 128
typedef void(*user_onRequest)(void); typedef void(*user_onRequest)(void);
typedef void(*user_onReceive)(uint8_t*, int); typedef void(*user_onReceive)(uint8_t*, int);
...@@ -67,7 +67,7 @@ protected: ...@@ -67,7 +67,7 @@ protected:
public: public:
TwoWire(uint8_t bus_num); TwoWire(uint8_t bus_num);
~TwoWire(); ~TwoWire();
void begin(int sda=-1, int scl=-1, uint32_t frequency=0); bool begin(int sda=-1, int scl=-1, uint32_t frequency=0);
void setClock(uint32_t frequency); // change bus clock without initing hardware void setClock(uint32_t frequency); // change bus clock without initing hardware
size_t getClock(); // current bus clock rate in hz size_t getClock(); // current bus clock rate in hz
...@@ -129,8 +129,8 @@ public: ...@@ -129,8 +129,8 @@ public:
void onReceive( void (*)(int) ); void onReceive( void (*)(int) );
void onRequest( void (*)(void) ); void onRequest( void (*)(void) );
void dumpInts(); uint32_t setDebugFlags( uint32_t setBits, uint32_t resetBits);
void dumpI2C(); bool busy();
}; };
extern TwoWire Wire; extern TwoWire Wire;
...@@ -138,6 +138,9 @@ extern TwoWire Wire1; ...@@ -138,6 +138,9 @@ extern TwoWire Wire1;
/* /*
V1.0.1 02AUG2018 First Fix after release, Correct ReSTART handling, change Debug control, change begin()
to a function, this allow reporting if bus cannot be initialized, Wire.begin() can be used to recover
a hung bus busy condition.
V0.2.2 13APR2018 preserve custom SCL,SDA,Frequency when no parameters passed to begin() V0.2.2 13APR2018 preserve custom SCL,SDA,Frequency when no parameters passed to begin()
V0.2.1 15MAR2018 Hardware reset, Glitch prevention, adding destructor for second i2c testing V0.2.1 15MAR2018 Hardware reset, Glitch prevention, adding destructor for second i2c testing
*/ */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment