espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.63k stars 7.41k forks source link

Is there any way to ensure 100% accurate i2c interfacing ? #811

Closed euquiq closed 6 years ago

euquiq commented 7 years ago

I came to realize that i2c timing is critical and since Arduino layer is on top os RTOS, interfacing with espressif IDF, depending on my code, it may be very difficult to maintain such i2c strict timing requirements.

I cannot get 100% clean data from my Melexis MLX90621 termal array sensor. It communicates through i2c with my ESP32 and sooner or later, data gets garbled. I already posted this issue before. https://github.com/espressif/arduino-esp32/issues/659

On the esp8266 everything works smoothly, no errors after days of running my code. On the esp32, sadly, the i2c communication is at most, unstable.

I get:

[E][esp32-hal-i2c.c:313] i2cRead(): Timeout! Addr: 60, index 32 [E][esp32-hal-i2c.c:161] i2cWrite(): Busy Timeout! Addr: 60 [W][esp32-hal-i2c.c:280] i2cRead(): Busy Timeout! Addr: 60

By the dozen each minute.

I did follow user stickbreaker recommendation, stating "The I2C subsystem is timing dependent. If any of the core error messages are generated it causes a chain reaction of additional timeout/ack failures. Disable the core logging and test the return codes in your application. uint8_t err=Wire.endTransmission() uint8_t err=Wire.requestFrom(slaveId,count);"

I can see the correct number of bytes being returned by my sensor, and no error after wire.endTransmission() ... until the errors start.

I can catch this and retry the reading / writing to i2c, or even first RESETTING the wire interface and then retry.

Right now I am doing just that. But it does not feel right (unless it is the only way to cope with it) ... hence the question:

Is there any way to tell Arduino "suspend all RTOS activities while I execute this code" ?

Regards, Enrique

stickbreaker commented 7 years ago

@euquiq a couple of us, @lonerzzz and I are working to improve the I2C subsystem. @lonerzzz has implement some fixes for bigger block handling. I am working on changing to an interrupt driven model from the current polling version. As you intimate, there is an interaction with the RTOS background process that is interfering with the polling model. I hope to achieve my goal. I an currently having success with big block reads, up to 32768 byte continuous transfers. Alas, I still have to implement Writes, onRequest() and onReceive().

Chuck.

stickbreaker commented 7 years ago

@euquiq I have a question: Does your device use ReStart? I think I found an issue with any Wire.endTransmission(false) or Wire.requestFrom(ID,size,false);

Chuck.

euquiq commented 7 years ago

Thanks @stickbreaker for your fast eyes-up response! I am glad you and @lonerzzz are working on it. Good luck on it, and I am eagerly following your posts on i2c topic.

For now, I catch the return value of Wire.endTransmission() and Wire.requestFrom() and if I get an error or the wrong number of bytes, I retry, previously doing a Wire.reset() ... it seems to work ok.

BUT ... Wire.endTransmission() sometimes returns error code 5 ... and I cannot find a definition for such error. Probably I am looking into the wrong places. Do you or anyone can enlighten me what error 5 would mean ?

Regards,

Enrique

euquiq commented 7 years ago

@stickbreaker About ReStart ... it uses Wire.endTransmission(false) in several places. I.E. a bit of original Code (as proposed by several older posts over the web for MLX90621):

//Pixel Corrections void readCPIX() { Wire.beginTransmission(0x60); Wire.write(0x02); Wire.write(0x41); Wire.write(0x00); Wire.write(0x01); Wire.endTransmission(false); Wire.requestFrom(0x60, 2); byte cpixLow = Wire.read(); byte cpixHigh = Wire.read(); }

Also, just in case anyone wants to comment (or if this is useful to anyone), this is what I am doing now for catching errors:

//Pixel Corrections void readCPIX() { byte errend = 0; byte errreq = 0; do { Wire.beginTransmission(0x60); Wire.write(0x02); Wire.write(0x41); Wire.write(0x00); Wire.write(0x01); errend = Wire.endTransmission(false); if (errend != 0) { Serial.println("CPIX TRA " + (String)errend); Wire.endTransmission(true); Wire.reset(); } else { errreq = Wire.requestFrom(MLX90621, 2); if (errreq != 2) Serial.println("CPIX REQ " + (String)errreq); } } while (errend > 0 || errreq != 2); byte cpixLow = Wire.read(); byte cpixHigh = Wire.read(); }

stickbreaker commented 7 years ago

@euquiq check out my last response in #741 , Are you interested in testing my function?

Chuck.

euquiq commented 7 years ago

@stickbreaker thx for the offering. This code is part of a POC device ("proof of concept") which I am presenting to a third party over the following hours / days, so right now I am not inclined to tinker with it much more.

Over the next hours / days, I got yet to build like 6 more devices, but there is a ton of soldering and 3D printing ahead, so I know I will not be able to deliver the attention to testing your function deserves ... not at least until / after I get the next "batch" ready for tinkering and got time avail to resume coding.

As I understand, you propose a Wire.transact() function which will "take over" all the Wire.write() and Wire.endTransmission() work, fixing the i2c timeout due to issuing a Wire.endTransmission(false).

At this time, with the error catching you told me a couple of days ago on another post, placed as in my above code, all over my i2c data fetching / writing routines I am not getting the timeout errors "all the time".

Right now my sensor has been polled once per second, for the last 3 hours () and the i2c errors detection / correction triggered only seven times over that time ... Which, given my timeline - sleep deprived situation, is bearable.

Yes, I want to eliminate every i2c error altogether. I want the same i2c behavior I am currently experiencing on my ESP8266 based devices, which has been working with no error catching and no false readings for weeks by now.

So I think I will need to test your proposed solution, and dive deeper into this, but three or four days from now, if that is ok with you ?

stickbreaker commented 7 years ago

That would be fine. If it is enough of a problem that you want to spend the time chasing it drop me a note and I'll provide the code.

Chuck.

euquiq commented 7 years ago

@stickbreaker Update: I keep receiving an unacceptable number of corrupt values from Wire.read() even after catching the Wire.endTransmission() and checking for the correct number of bytes received by Wire.read().

The MLX90621 sensor is being polled once per second. After 10 hours or so, I received at least 3 wrong values from the thermal array (Declaring weird temperatures on one -random- pixel).

I got several sensors, and several ESP32 and ESP8266: As far as I could test, this behavior does NOT occur on the ESP8266.

AFAIK, I do not have a reliable way of detecting such errors from the Arduino framework itself.

Under my (mostly uninformed and rushed) eye, I suspect this may also be a consequence from bad timing / "multitasking" breaks while Arduino ESP32 deals with the Wire interface ?

For now, I am adding (more) extra code order for my program to detect such corrupt values, but I am concerned about the whole thing.

If Arduino had some mechanism / command to freeze the RTOS "multitasking" and dedicated 100% of the core for executing the Wire transaction, and then resuming, maybe it would reach the desired stability ?

Do you think your function / modifications would improve the stability, considering my scenario (where bytes are being corrupted when read) ? If so, please, share it :)

stickbreaker commented 7 years ago

I am in the midst of reverse engineering the I2C subsystem. The ESP-IDF, Espressif documentation are not very conclusive. What I have discovered so far, is that the I2C statemachine is no very forgiving. The I2C ReStart support is very time sensitive. In the current implementation of Wire.h the Wire.endTransmission(false); statement leaves the I2C system waiting for additional data. (the Wire.requestFrom();) If the RTOS does a task switch between the endTransmission() and the requestFrom() a timeout may occur. The I2C hardware timer is running, and if the next Wire() command is not issued before this timer expires, a non recoverable error occurs. The only recourse is to reInit the I2C hardware.

I wrote a Wire.transact() function that would combine these two operations into one continuous I2C command sequence. It should solve this timeout issue.

But, as you can see from this excuse, I do not have a working pull to offer you at this time. I have continued my exploration of the I2C subsystem, currently I am in the middle of a major rewrite to support background interrupt driven data handling. My goal is to develop SLAVE mode support. I have successfully created and tested MASTER mode unlimited block size sending and receiving. I have spend the last week trying to figure out error recovery methods that would allow me to reset the I2C hardware without re initializing it all the time. Sometimes the reinit sequence creates glitches on the bus that cause other devices to become confused and fail to respond to commands.

I had assumed by your prior message that your interests were divergent from mine, so I continued on my path. It will probably take me a week or two to compete my ISR work (if I don't fail :smile:) As soon as I get a working version I will message you.

I have had another thought about the possible coding for this restart support. Arduino support for ReStart sequences was a bolt on addition to the Wire.h library. The way it was implemented allows for illegal I2C operations: START > WRITE device A > (noStop) ReStart -> READ Device B > (noStop) {this sequence will result in a NAK because Device B 'knows' Device A has ownership of the bus, no STOP has been issued to release the bus) Restart -> READ Device A -> STOP { may execute, but since the READ B was issued, it might not}

The ReStart function on the I2C bus allows multiple read/write transactions to be preformed without loosing ownership of the I2C bus. In a Multi-MASTER environment a STOP command allows other MASTERs to start a new transaction. The ReStart holds the ownership, but allows multiple command sequences that are contiguous.

What do you think about a Wire.endTransmission(false) returning ERROR_CONTINUE on the ESP32? To use the existing Wire.h syntax:

Wire.beginTransmission(ID);
Wire.write(addr);
uint8_t err=Wire.endTransmission(false); //initiate ReStart operation, 
// on the ESP32, the Write data is just stored until the requestFrom() command is executed,
// then the combined START->WRITE->ReStart->READ->STOP sequence is actually executed.
// this delayed write may fail during the requestFrom() command.  So error Checking is different.
if(err==ERROR_CONTINUE){ // write command has been queued.
  err=Wire.requestFrom(id,numRead,true); //actually preform the sequence
// possible Error return value from requestFrom are 0 or numRead.  This is not sufficient
// my version returns ActualNumberRead
// to check for errors I created a new function in `Wire.h`
  err = Wire.lastError();
// lastError() returns the standard I2C error Values plus
// ERROR_WRITE_ADDR_NAK // NAK during device address of a stand alone write operation, or as part of transact()
// ERROR_READ_ADDR_NAK// NAK during device address of a stand alone read operation, or as part of transact()
// ERROR_DATA_NAK // only possible during Write Operations
// ERROR_ARBITRATION_LOST // in a Multi-Master environment, the Other master gained ownership of the bus, this transaction failed.  Error recovery is left to the app.
  while(Wire.available()){
    Wire.read();
    }
  }

Another Question: Does your code require this sequence of I2C commands?

Wire.beginTransmission(ID);
Wire.write(value);
Wire.endTransmission(false); // would return ERROR_CONTINUE
Wire.beginTransmission(ID);
Wire.write(value);
Wire.endTransmission(false); // Would Return ERROR_CONTINUE
Wire.beginTransmission(ID);
Wire.write(value);
Wire.endTransmission(); // or Wire.endTransmission(true); would return an Actual Error Value.

The reason I ask, is that all of these three sequences need to be embodied into ONE I2C sequence to operate properly. If I need to support these type of sequences, I'll have to do implement a more complex multi sequence queue.

Chuck.

euquiq commented 7 years ago

Hi Chuck! Thanks for your time. My knowledge about i2c is flimsy, so I am re-reading your last post, while digesting it. I will comment about it a bit later.

About your last question, I cannot see that specific kind of commands sequence on the code. The mlx90621 sensor needs several i2c functions in order to configure / receive data.

Such functions can be seen, i.e. in here: https://github.com/robinvanemden/MLX90621_Arduino_Processing/blob/master/readTemperatures/MLX90621.cpp

I get errors, at random on each of these functions inside ESP32… Most of them include the dreaded Wire.endTransmission(false) !

By now I modified every function with error catching and after reading temperatures, I do also check for inconsistent values and retry until I am sure the values read are ok. Such mechanisms saved the day, final user-wise. But I am not happy with all the extra code and time used in order to police around the Wire problem.

In case you are interested / curious about each function needed, when interfacing with the MLX90621 sensor thru i2c there is interaction with an EEPROM (at address 0x50) and the sensor pixel array computer itself (0x60), thru the following:

readIR() Reads 64 2-byte values from the sensor, fetching each 16bit temperature value from each cell

readPTAT() It reads the ambient temperature

readCPIX() Reads a 2 byte value (uInt16) (value used later for thermal pixel corrections, whatever that should mean).

readConfig() It fetches two bytes with config flag-bits inside

writeTrimmingValue() Writes trimming value for the internal oscillator.

readEEPROM() it reads 64 byte with factory calculated compensations for each pixel, specific for each sensor pixel.

setConfiguration() it writes the user configuration for the sensor (i.e. update speed ).

stickbreaker commented 6 years ago

@euquiq check out my #839 it may solve your problem.

Chuck.

euquiq commented 6 years ago

@stickbreaker Thanks Chuck ! Brilliant ! I will create a copy of my current code and implement all i2c as you deviced, and will come back with my results :) Thanks a lot, your investigation and work in invaluable !

euquiq commented 6 years ago

@stickbreaker Hi there ! I am working on adapting your code. I had to acommodate it into my sensor's functions and add a Wire.endTransaction() at the end of each of each one.

I had other problems, I am slowly fixing them. I will post my overall experience ASAP. Overall, It seems to be working for me, but with some "quirks". I am yet to get it 100% in working condition.

The interesting part is that there seems to be NO timeout errors, as you said :)

Thanks again !

stickbreaker commented 6 years ago

@euquiq What kind of code changes have you had to make? I would be interested in seeing a copy of your before and after conversion.

Chuck.

euquiq commented 6 years ago

@stickbreaker I think I've found a bug: I implemented your i2c version on all i2c related functions and I got weird temperature values.

Checking the data being polled from i2c, with your implementation some bytes get consistently lost and replaced by "255" value.

I.E. Function that reads 256 bytes from the eeprom of my MLX90621 sensor. It reads it in chunks of 32 bytes.

As a reference, this is the code you can find on several repositories, valid for ESP8266 with no error correction whatsoever and it works like a charm in them, taken from: https://github.com/GarageProto/GP-004-01/blob/master/MLX90621.cpp

void MLX90621::readEEPROM() { // Read in blocks of 32 bytes to accomodate Wire library
  for(int j=0;j<256;j+=32) {
    Wire.beginTransmission(0x50);
    Wire.write(j);
    byte rc = Wire.endTransmission(false);
    Wire.requestFrom(0x50, 32);
    for (int i = 0; i < 32; i++) {
      eepromData[j+i] = (uint8_t) Wire.read();
    }
  }
}

The following is my "old" (standard ESP32 i2c) code with the error catching stuff added to it, which "gets the work done" catching tons of errors:

void readEEPROM() { // Read in blocks of 32 bytes to accomodate Wire library
    byte errend = 0;
    byte errreq = 0;
    for (int j = 0; j<256; j += 32) {
        do {
            Wire.beginTransmission(MLX90621_EEPROM);
            Wire.write(j);
            errend = Wire.endTransmission(false); 
            if (errend != 0) { 
                Serial.println("EEPROM ERROR " + (String)errend);
                Wire.endTransmission(true);
                Wire.reset();
            }
            else {
                errreq = Wire.requestFrom(MLX90621_EEPROM, 32); 
                if (errreq != 32) Serial.println("EEPROM REQ ERROR" + (String)errreq); 
            }       
        } while (errend > 0 || errreq != 32);
        for (int i = 0; i < 32; i++) {
            eepromData[j + i] = (uint8_t)Wire.read();
        }
    }
}

The above code produces the following 256 values byte array:

EEPROM:
 255 255 255 255 255 255 255 255 16 26 9 26 17 21 27 34 17 28 45 41 22 42 46 23 18 27 35 24 35 31 41 28 42 42 27 38 24 34 48 46 42 43 39 53 33 37 41 42 40 55 52 51 35 43 44 63 44 40 48 64 38 44 37 57 162 162 175 166 166 166 175 171 166 175 175 179 171 183 188 183 179 188 188 183 179 188 188 179 179 188 188 179 183 183 192 183 188 192 196 192 188 192 201 192 188 196 201 196 183 192 192 201 183 196 205 205 188 201 201 205 188 196 201 213 179 192 201 209 19 33 30 7 55 77 68 46 93 115 111 84 127 150 153 120 161 186 175 147 170 216 206 168 199 230 232 191 198 233 228 200 191 233 233 198 191 224 218 189 162 198 203 169 141 172 174 152 112 135 140 118 75 102 107 84 41 62 64 43 0 25 26 14 153 158 158 158 158 158 158 158 158 158 158 158 158 158 180 255 137 255 139 170 255 166 123 15 24 7 80 101 205 87 157 95 168 90 38 31 0 128 12 2 255 19 1 44 101 190 46 96 120 106 255 255 255 62 70 83 228 48 13 53 104 34 0 134 END

And with your i2c version, this is the code, which I tested both with the Wire.endTransmission(false); Wire.requestForm(MLX90621_EEPROM, 32); and the more compact Wire.transact(MLX90621_EEPROM, 32);

void readEEPROM() { // Read in blocks of 32 bytes to accomodate Wire library
    byte err = 0;
    for (int j = 0; j<256; j += 32) {
        do {
            Wire.beginTransmission(MLX90621_EEPROM);
            Wire.write(j);
            Wire.endTransmission(false); 
            err = Wire.requestFrom(MLX90621_EEPROM, 32);
            if (err != 32) { 
                Serial.println("EEPROM TRA " + (String)err);
                Wire.endTransmission(true); 
                Wire.reset();
            }   
        } while (err != 32);
        for (int i = 0; i < 32; i++) {
            eepromData[j + i] = (uint8_t)Wire.read();
        }
               Wire.endTransmission(true); 
    }
}

The above code with your i2c version produces the following output on the 256 byte array:

CHUCK EEPROM:
 255 255 255 255 255 255 255 255 16 26 9 26 17 21 27 34 17 28 45 41 22 42 46 23 18 27 35 24 35 31 41 28 42 27 38 24 34 48 46 42 43 39 53 33 37 41 42 40 55 52 51 35 43 44 63 44 40 48 64 38 44 37 57 162 175 166 166 166 175 171 166 175 175 179 171 183 188 183 179 188 188 183 179 188 188 179 179 188 188 179 183 183 192 183 188 192 192 188 192 201 192 188 196 201 196 183 192 192 201 183 196 205 205 188 201 201 205 188 196 201 213 179 192 201 209 19 33 30 55 77 68 46 93 115 111 84 127 150 153 120 161 186 175 147 170 216 206 168 199 230 232 191 198 233 228 200 191 233 233 198 224 218 189 162 198 203 169 141 172 174 152 112 135 140 118 75 102 107 84 41 62 64 43 0 25 26 14 153 158 158 158 158 158 158 158 158 158 158 158 158 180 255 137 255 139 170 255 166 123 15 24 7 80 101 205 87 157 95 168 90 38 31 0 128 2 255 19 1 44 101 190 46 96 120 106 255 255 255 62 70 83 228 48 13 53 104 34 0 134 255 255 255 255 255 255 255 END

And subsequently all other i2c reads (and writes ?) seems to be somewhat garbled, since I get really weird temperatures as a result.

It is as if your i2c implementation is missing reading some bytes.

Hopefully this helps.

Enrique.

stickbreaker commented 6 years ago

@euquiq try this code:


void MLX90621::readEEPROM() { // Read in blocks of 32 bytes to accomodate Wire library
Wire.beginTransmission(0x50);
Wire.write((uint8_t)0); // set address pointer to zero,  
int err =Wire.transact(eepromData,256); //grab 256 byte directly into eepromData
if(err != 256){
  Serial.printf("Transact failed =%d, bytes Received =%d",Wire.lastError(),err);
  }
}

Chuck.
euquiq commented 6 years ago

@stickbreaker I am trying the above codes in a second ESP32, (other brand) and another mlx90621 sensor, in order to rule out hardware problems.

Again, weird data read with your i2c implementation. Consider this is a second MLX90621 sensor, and the eeprom data is the precalibration at factory time, so bytes are different from the first sensor:

This result is the correct one, with the standard i2c implementation:

 EEPROM 2:
 0 2 4 9 5 16 10 14 12 21 16 21 15 23 28 33 33 27 27 37 17 25 27 32 22 28 23 32 31 29 45 22 29 32 33 27 35 41 41 30 27 36 39 50 39 30 49 56 41 40 44 47 44 35 41 40 38 40 36 51 36 39 32 50 162 166 171 166 166 175 175 166 171 175 175 171 175 179 183 179 188 183 179 183 179 183 183 179 179 188 188 183 188 188 196 183 183 192 188 183 188 192 192 192 183 192 192 192 183 188 201 196 188 192 196 205 188 188 192 196 179 192 201 192 179 183 201 201 9 24 24 9 45 62 62 47 77 98 101 77 108 134 134 109 137 164 166 135 154 188 185 156 176 206 209 172 183 215 217 181 181 215 213 179 174 205 209 165 155 182 179 152 140 161 152 117 106 121 121 96 78 94 86 65 39 52 54 33 1 15 14 0 153 158 158 158 158 158 158 158 158 158 158 158 158 158 173 255 147 255 139 164 255 166 235 15 24 7 102 100 119 87 192 94 56 87 38 31 0 128 12 2 255 4 1 22 100 190 73 95 137 105 255 255 255 62 70 79 228 48 13 53 104 34 0 157 END

And now with your i2c implementation, as seen on my post above:

CHUCK  EEPROM 2:
 0 2 4 9 5 16 10 14 12 21 16 21 15 23 28 33 33 27 27 37 17 25 27 32 22 28 23 32 31 29 45 22 32 33 27 35 41 41 30 27 36 39 50 39 30 49 56 41 40 44 47 44 35 41 40 38 40 36 51 36 39 32 50 162 171 166 166 175 175 166 171 175 175 171 175 179 183 179 188 183 179 183 179 183 183 179 179 188 188 183 188 188 196 183 183 192 183 188 192 192 192 183 192 192 192 183 188 201 196 188 192 196 205 188 188 192 196 179 192 201 192 179 183 201 201 9 24 24 45 62 62 47 77 98 101 77 108 134 134 109 137 164 166 135 154 188 185 156 176 206 209 172 183 215 217 181 181 215 213 179 205 209 165 155 182 179 152 140 161 152 117 106 121 121 96 78 94 86 65 39 52 54 33 1 15 14 0 153 158 158 158 158 158 158 158 158 158 158 158 158 173 255 147 255 139 164 255 166 235 15 24 7 102 100 119 87 192 94 56 87 38 31 0 128 2 255 4 1 22 100 190 73 95 137 105 255 255 255 62 70 79 228 48 13 53 104 34 0 157 0 2 4 9 5 16 10 END
euquiq commented 6 years ago

@stickbreaker I posted my last comment before reading your suggestion about placing the eeprom data directly inside the byte array. I will try your suggestion tomorrow as now I am so asleep I am afraid I will make a mistake. I will post the result ASAP.

euquiq commented 6 years ago

@stickbreaker sorry, I could not go to sleep without trying: Congrats ! When reading the whole eeprom content directly into the array, it read OK.

Problem may still be present inside all other i2c functions that the MLX90621 uses. I will dive into it tomorrow. In the meantime, hopefully you can use this info.

CHUCK WHOLE ARRAY AT ONCE EEPROM 2:
 0 2 4 9 5 16 10 14 12 21 16 21 15 23 28 33 33 27 27 37 17 25 27 32 22 28 23 32 31 29 45 22 29 32 33 27 35 41 41 30 27 36 39 50 39 30 49 56 41 40 44 47 44 35 41 40 38 40 36 51 36 39 32 50 162 166 171 166 166 175 175 166 171 175 175 171 175 179 183 179 188 183 179 183 179 183 183 179 179 188 188 183 188 188 196 183 183 192 188 183 188 192 192 192 183 192 192 192 183 188 201 196 188 192 196 205 188 188 192 196 179 192 201 192 179 183 201 201 9 24 24 9 45 62 62 47 77 98 101 77 108 134 134 109 137 164 166 135 154 188 185 156 176 206 209 172 183 215 217 181 181 215 213 179 174 205 209 165 155 182 179 152 140 161 152 117 106 121 121 96 78 94 86 65 39 52 54 33 1 15 14 0 153 158 158 158 158 158 158 158 158 158 158 158 158 158 173 255 147 255 139 164 255 166 235 15 24 7 102 100 119 87 192 94 56 87 38 31 0 128 12 2 255 4 1 22 100 190 73 95 137 105 255 255 255 62 70 79 228 48 13 53 104 34 0 157 END
stickbreaker commented 6 years ago

ok, I can see from the two data blocks, the second on is skipping a value every read, The first is 0..31, 32..63 the Second 0..31, 33..64, I am not able to reproduce it with my test bed, I am testing against 24LC32,64,512 and DS1308, MCP23008;

here is the code from my test function:

Wire.beginTransmission(ID);
if((ID >= 0x50)&&(ID <= 0x57)) Wire.write(highByte(addr)); // 16bit device addressing
Wire.write(lowByte(addr));
if((err=Wire.endTransmission())!=0) {
  Serial.printf(" EndTransmission=%d",err);
  if(err!=2) {
    Serial.printf(", resetting\n");
    Wire.reset();
    return;
    }
  else {
    Serial.printf(", No Device presend, aborting\n");
    return;
    }
  }
err = Wire.requestFrom((uint8_t)ID,(size_t)BlockLen,true);
Serial.printf("@0x%02x(0x%04x)=%d ->",ID,addr,err);
addr += BlockLen;
if(!((ID >= 0x50)&&(ID <= 0x57))) addr %= 256; // byte addressed devices

uint8_t b=0;
char buf[100];
uint16_t blen=0;

while(Wire.available()){
  char a=Wire.read();
  if((a<32)||(a>127)) Serial.print('.');
  else Serial.print(a);
  blen+=sprintf(&buf[blen]," %02x",a);
  b++;
  if((b%32)==0){
    Serial.println(buf);
    blen=0;
    }
  }
if(!((b%32)==0)) Serial.println(buf);
}

for your device the two address range if((ID>=0x50)&&(ID<=0x57)) need to be changed, They exist because my devices in that range use 16bit internal address pointers instead of your 8bit. So just comment out both of the if lines, The add `addr=addr%255.

Chuck.

stickbreaker commented 6 years ago

@euquiq Duplicated your problem, Am researching it. Should have solution by morning 😬

Chuck.

stickbreaker commented 6 years ago

@euquiq , found it, I did not set txQueued back to zero after the requestFrom()

at about line 174 in /libraries/Wire/src/Wire.cpp at txQueued =0; right Here! Wire.cpp:174

Sorry! Chuck.

euquiq commented 6 years ago

@stickbreaker thanks a lot, Chuck !

I just edited Wire.cpp and added the code line you posted. My device compiles OK and seems to be working fine !! I will give it a non/stop run over the weekend and log any errors / hiccups, but I am quite confident that errors are tamed / null with your code.

I would encourage you and @me-no-dev to consider your additions / changes for the main arduino ESP32 repository :) since as it is, changes for earlier implementation i2c code are nill / minimal, but it adds on stability and new features. Bugs -if still present- will be ironed out faster if more people try it !

Cheers, Enrique

stickbreaker commented 6 years ago

Sounds great, I found another related issue that would only crop up if multiple consecutive:

Wire.beginTransmission(ID);
Wire.write();
Wire.endTransmission(false);

Blocks were present. My code would keep summing the total bytes written with each newly queued endTransmission().

You did not encounter it because your code only queued ONE command before the queue was processed.

I pushed another fix to my repo. Wire.cpp:334

I hope you have a Happy and Productive Weekend!

Chuck.

euquiq commented 6 years ago

@stickbreaker There might be another bug:

This happened at one "beta testing" location for my hardware: when turned on, it had yet to be configured for working with their WIFI network, so it was set up in AP mode.

Weirdly enough, the ESP32 returned garbage from the i2c sensor. We tried several cycling / AP reconnecting instances, and the behavior was consistent.

When I configured the wifi ssid and password and the ESP32 started working on wifi station mode, the i2c started working normally.

This should be pretty much easily duplicated, I will try it at home in a couple of hours, but wanted to give you a heads up, in the event that you where around at this time and wanted to check it for yourself.

Regards,

Enrique

stickbreaker commented 6 years ago

@euquiq off the top of my head, I have no clues. About interaction between the WiFi code and my i2c, Unless the Wifi configures/ uses the i2c during its init?

My i2c code assumes exclusive access to I2C0

Chuck.

lonerzzz commented 6 years ago

@euquiq If you are using multiple tasks, you should pin the one using @stickbreaker 's code to CORE1 otherwise you will have collisions with the WiFi interrupt. Just figured this out.

stickbreaker commented 6 years ago

@lonerzzz Where would the best place to note this Wifi/i2c interaction. The necessity of pinning? espressif/arduino-esp32 does not have WiKi enabled? enabled it on my fork.

Should we add some file in the /docs directory?

lonerzzz commented 6 years ago

@stickbreaker We should ask @me-no-dev if there are issues with enabling the Wiki because otherwise the question will likely come up often.

stickbreaker commented 6 years ago

@me-no-dev nudge, nudge

stickbreaker commented 6 years ago

@me-no-dev @lonerzzz I have been thinking about creating a branch in my repo to contain a cleanup merge candidate. Should it be Here espressif/arduino-esp32 or stickbreaker/arduino-esp32?

euquiq commented 6 years ago

@stickbreaker @lonerzzz Hi again ... I was not able to replicate that weird i2c behavior in my other ESP32 / MLX90621 sensors back here at home .

I got only one task (send email) which was pinned at core 0 and seldom used, but even triggering an email send in order for it to work, I am not getting any problems (although I just moved it into core 1, just in case ...).

I am trying to think on every combination of action I could have made at the installation time in order to garble i2c in such a way for it to throw me the wrong bytes when read while on AP mode.

Unless I can get to trigger the weird values again, I am inclined of thinking on some other spurious situation (static, bad manipulation from the person that was installing it, etc.)

Again, Kudos for this i2c implementation !

stickbreaker commented 6 years ago

@euquiq Keep testing, It is has only been tested on a narrow range of hardware and there could be other hardware/software interactions.

But, I'm still happy that it works for you!

Chuck.

me-no-dev commented 6 years ago

someone poking me? :D I am all for enabling the Wiki ;)

me-no-dev commented 6 years ago

@stickbreaker @lonerzzz let's figure out a more "real-time" way to discuss this. How about gitter?

stickbreaker commented 6 years ago

@me-no-dev , Now you want me to figure out an totally new website, 😬

Ok, I'll try.

euquiq commented 6 years ago

@stickbreaker Hi there! I am updating the github ESP32 main branch, so then I can get into your repository for the i2c related changes. But correct me if I am wrong, now your repository is a branch of the whole esp32 master repository, instead of just the files you changed ? Or was it always like this ?

I am asking because I think that the master now includes some updates to IDF and other commits that your fork seems to be missing.

Sorry for the questions / Eyes-up but I am still trying to get an understanding at github way of work.

Regards,

Enrique

stickbreaker commented 6 years ago

@euquiq , yea, I just forked the repo and changed files.

I'm going to try to merge from here into my repo, wish me luck!

Grab the latest from here espressif/arduino-esp32 then replace these 5 files.

\libraries\Wire\scr\Wire.h \libraries\Wire\scr\Wire.cpp \cores\esp32\esp32-hal-i2c.h \cores\esp32\esp32-hal-i2c.c \cores\esp32\esp32-hal-log.h

from my repo. -log.h is not necessary right now, but in a couple of hours I am committing a update that will require it. I have been working to clean up and organize my code. Trying to make it more polite.

I have changed a couple more: README.md \libraries\Wire*

In the Wire library I have added some documentation, and examples, updated the libraries.properties and the keywords.txt

Chuck.

stickbreaker commented 6 years ago

@euquiq Ok, I think my repo is current. Now you will need the esp32-hal-log.h I added a log_n() so that critical errors will still pop. Before this get to Release, It will probably need to be excised.

I have been using my I2c without any fails for the last week. I'm starting on something else.

If you have a issues mention me.

I have been adding to a wiki on my repo I2C thoughts Chuck.

stickbreaker commented 6 years ago

@euquiq What is your status? Are you using my I2C? is it working? If you don't have any more issues, you can close this issue.

If something else crops up open a new issue in stickbreaker/arduino-esp32

Chuck.

euquiq commented 6 years ago

Hi @stickbreaker ... yeps no problems whatsoever. I really think your code should be integrated to esp32's main branch ! Congratulations for your fine and exhaustive work.

Khelicon commented 6 years ago

@stickbreaker First of the all thanks for great work!

I am still facing an error. I guess it will help you to debug more. I am using SSD1306 Oled and with three buttons using Onebutton library in platformIO. I update the I2C files as per your suggestions. OneButton --> Button.tick() is called from loop() function (later i attach timer0 and called button.tick() but same results)

Problem seems to be only causing if delay between calling button.tick() is less than 200ms

I get following error:

[E][esp32-hal-i2c.c:609] i2cDumpI2c(): i2c=0x3ffc1624 [E][esp32-hal-i2c.c:610] i2cDumpI2c(): dev=0x60013000 date=0x16042000 [E][esp32-hal-i2c.c:611] i2cDumpI2c(): lock=0x3ffc8b8c [E][esp32-hal-i2c.c:612] i2cDumpI2c(): num=0 [E][esp32-hal-i2c.c:613] i2cDumpI2c(): mode=1 [E][esp32-hal-i2c.c:614] i2cDumpI2c(): stage=3 [E][esp32-hal-i2c.c:615] i2cDumpI2c(): error=5 [E][esp32-hal-i2c.c:616] i2cDumpI2c(): event=0x3ffc8c10 bits=100 [E][esp32-hal-i2c.c:617] i2cDumpI2c(): intr_handle=0x3ffc8c40 [E][esp32-hal-i2c.c:618] i2cDumpI2c(): dq=0x3ffc5cfc [E][esp32-hal-i2c.c:619] i2cDumpI2c(): queueCount=1 [E][esp32-hal-i2c.c:620] i2cDumpI2c(): queuePos=0 [E][esp32-hal-i2c.c:621] i2cDumpI2c(): byteCnt=0 [E][esp32-hal-i2c.c:582] i2cDumpDqData(): [0] 78 W STOP buf@=0x3ffc3226, len=17, pos=17, eventH=0x0 bits=0 [E][esp32-hal-i2c.c:598] i2cDumpDqData(): 0x0000: @................ 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [E][esp32-hal-i2c.c:942] i2cDumpInts(): row count INTR TX RX [E][esp32-hal-i2c.c:945] i2cDumpInts(): [01] 0x0001 0x0002 0x0012 0x0000 0x0001e11a [E][esp32-hal-i2c.c:945] i2cDumpInts(): [02] 0x0003 0x0100 0x0000 0x0000 0x0001e141 [E][esp32-hal-i2c.c:1149] i2cProcQueue(): Busy Timeout start=0x1e663, end=0x1e696, =51, max=51 error=5.


Simran

stickbreaker commented 6 years ago

@harji2130 This debug output is showing the I2C bus is in a Busy state before this Wire.write() sequence was started. I don't have any experience with SSD1306 or OneButton what are they? How are they interfaced?

Something is causing the I2C bus to hang. It is not this call. The Busy state is probably initiated in a prior Wire() call. Try increasing the default Wire timeout from it's 50ms value to say 500ms with: Wire.setTimeout(500); in setup() See if that changes anything.

Also, capture earlier debug output. The debug you posted is after the problem occurred.

Chuck.

carbonadam commented 6 years ago

@stickbreaker thank you for doing the branch. I was having drop outs with the i2c after i started wifi and interfacing with a bnO055. Taking your branch has solved it so now I can control my little robot over wifi and run the BNO YAY!

stickbreaker commented 6 years ago

@carbonadam Always good to hear it works! Thanks.

Chuck.

kristsm commented 6 years ago

Just wanted to inform everyone that yesterday I tried to compile with Platformio's platform = https://github.com/platformio/platform-espressif32.git#feature/stage And it seems that with the latest Platformio's Arduino fork (without stickbreaker's mods) the I2C with my BME280 and MLX90614 parallel readings is working without problems!

kemaddux commented 6 years ago

[W][esp32-hal-i2c.c:334] i2cRead(): Ack Error! Addr: 40 Adafruit ESP32 Huzzah on Arduino 1.8.5 with Adafruit HTU21DF Humidity and Temperature sensor using I2C. Runtime of 44:17:33 on the 8859 I2C read before the error halted the ESP32. Using library WiFi at version 1.0 Using library ArduinoOTA at version 1.0 Using library Update at version 1.0 Using library Wire at version 1.0 Using library Adafruit_HTU21DF_Library at version 1.0.1 Using library Adafruit_MQTT_Library at version 0.20.1 Using library DallasTemperature at version 3.8.0 Using library arduino_834569 at version 2.3.4 Using library ESPmDNS at version 1.0 Any new I2C updates or things to try to prevent this read error?

euquiq commented 6 years ago

@stickbreaker Hi Chuck! I've been trying to contact you. I think I wrote you on your facebook page. Hopefully you can read it :) Thanks in advance!