Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ReSTART fix, with others #1717

Merged
merged 5 commits into from
Aug 14, 2018
Merged

ReSTART fix, with others #1717

merged 5 commits into from
Aug 14, 2018

Conversation

stickbreaker
Copy link
Contributor

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 buffer pointers.
    • removed direct access to i2cDumpInts(), i2cDumpI2c() from app, instead, use i2cDebug() to set trigger points

@stickbreaker
ReSTART fix, Sequencing fix
9df839c 
pr espressif#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 
 
*
@stickbreaker
Update esp32-hal-i2c.c
9c1c532
@stickbreaker
Update Wire.cpp
22afc3a
@stickbreaker
ReSTART, Sequencing
9da648c 
pr espressif#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 
 
*
This was referenced Aug 2, 2018
Closed
Closed
Closed
@emptynick
Copy link

emptynick commented Aug 4, 2018 •
edited
Loading

Hi @stickbreaker,
I pulled your changes to my local environment, but I'm getting random reboots.
I'm using a display library which throws the following errors:

CORRUPT HEAP: Bad head at 0x3ffda388. Expected 0xabba1234 got 0x3ffda408
assertion "head != NULL" failed: file "/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/heap/multi_heap_poisoning.c", line 205, functi
on: multi_heap_free
abort() was called at PC 0x400e73b7 on core 1

Backtrace: 0x4009034c:0x3fffc740 0x4009054f:0x3fffc760 0x400e73b7:0x3fffc780 0x40090019:0x3fffc7b0 0x4008534e:0x3fffc7d0 0x40087515:0x3fffc7f0 0x
4000bec7:0x3fffc810 0x400e0663:0x3fffc830 0x400e0e52:0x3fffc850 0x400d8f2d:0x3fffc870 0x400d8f61:0x3fffc890 0x400d8fb1:0x3fffc8b0 0x400d222f:0x3f
ffc8d0 0x400d4a5e:0x3fffc8f0 0x400d9c4a:0x3fffc920 0x400d9c9f:0x3fffc950 0x400d2f56:0x3fffc970 0x400d3226:0x3fffc990

Decoded:

0x400e73b7: __assert_func at /Users/ivan/e/newlib_xtensa-2.2.0-bin/newlib_xtensa-2.2.0/xtensa-esp32-elf/newlib/libc/stdlib/../../../.././newlib/libc/stdlib/assert.c line 63 (discriminator 8)
0x4009034c: invoke_abort at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/panic.c line 649
0x4009054f: abort at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/panic.c line 649
0x400e73b7: __assert_func at /Users/ivan/e/newlib_xtensa-2.2.0-bin/newlib_xtensa-2.2.0/xtensa-esp32-elf/newlib/libc/stdlib/../../../.././newlib/libc/stdlib/assert.c line 63 (discriminator 8)
0x40090019: multi_heap_free at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/heap/multi_heap_poisoning.c line 306
0x4008534e: heap_caps_free at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/heap/heap_caps.c line 123
0x40087515: _free_r at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/newlib/syscalls.c line 42
0x400e0663: i2cFlush at ?? line ?
0x400e0e52: i2cWrite at ?? line ?
0x400d8f2d: TwoWire::writeTransmission(unsigned short, unsigned char*, unsigned short, bool) at ?? line ?
0x400d8f61: TwoWire::endTransmission(bool) at ?? line ?
0x400d8fb1: TwoWire::endTransmission() at ?? line ?
0x400d222f: SSD1306Wire::sendCommand(unsigned char) at ?? line ?
0x400d4a5e: SSD1306Wire::display() at ?? line ?
0x400d9c4a: Menu::redraw() at ?? line ?
0x400d9c9f: Menu::up() at ?? line ?
0x400d2f56: displayLoop() at ?? line ?
0x400d3226: uiLoop(void*) at ?? line ?

Honestly I'm not sure if your changes cause it, but I never had this before using your code from this PR.

Christoph

Edit: I reverted the changes and don't see the error coming up. Another point is that I dont have the display connected to the board (which might be a bad idea)

@stickbreaker
Copy link
Contributor Author

@emptynick The stack dump is showing a buffer overwrite inside the ISR, i2cFlush() releases an array used for transaction elements. Will you make two debug changes and rerun this?

inside esp32-hal-esp32.c uncomment line 45: to enable debug

#define ENABLE_I2C_DEBUG_BUFFER

in SSD1306Wire::display() before sendCommand() add

Wire.setDebugFlags(1,0);

after sendCommand()

Wire.setDebugFlags(0,1);

Then capture the output.

I've got a SSD1306 (thanks @cyberman54) so I might be able to use your failing code. If you want to send it.

Chuck.

@andriyadi
Copy link

One line 360 of Wire.cpp you forgot to add return.

I've pulled this PR and integrate to my environment, so far not found any issues. I'll keep testing.

@andriyadi
Copy link

I've tried to read accelerometer data from LSM6DSL sensor, once in a while I got this:

[E][esp32-hal-i2c.c:314] i2cDumpI2c(): i2c=0x3ffc12e0
[I][esp32-hal-i2c.c:315] i2cDumpI2c(): dev=0x60027000 date=0x16042000
[I][esp32-hal-i2c.c:319] i2cDumpI2c(): num=1
[I][esp32-hal-i2c.c:320] i2cDumpI2c(): mode=1
[I][esp32-hal-i2c.c:321] i2cDumpI2c(): stage=3
[I][esp32-hal-i2c.c:322] i2cDumpI2c(): error=5
[I][esp32-hal-i2c.c:323] i2cDumpI2c(): event=0x3ffc6cb0 bits=112
[I][esp32-hal-i2c.c:324] i2cDumpI2c(): intr_handle=0x3ffcadac
[I][esp32-hal-i2c.c:325] i2cDumpI2c(): dq=0x3ffc6d98
[I][esp32-hal-i2c.c:326] i2cDumpI2c(): queueCount=2
[I][esp32-hal-i2c.c:327] i2cDumpI2c(): queuePos=1
[I][esp32-hal-i2c.c:328] i2cDumpI2c(): errorByteCnt=0
[I][esp32-hal-i2c.c:329] i2cDumpI2c(): errorQueue=1
[I][esp32-hal-i2c.c:330] i2cDumpI2c(): debugFlags=0x00000000
[I][esp32-hal-i2c.c:287] i2cDumpDqData(): [0] 7bit 6b W  buf@=0x3ffc38aa, len=1, pos=1, ctrl=11101
[I][esp32-hal-i2c.c:305] i2cDumpDqData(): 0x0000: -                                2d 
[I][esp32-hal-i2c.c:287] i2cDumpDqData(): [1] 7bit 6b R STOP buf@=0x3ffc3824, len=1, pos=0, ctrl=11111
[I][esp32-hal-i2c.c:305] i2cDumpDqData(): 0x0000: .                                06 
[I][esp32-hal-i2c.c:341] i2cDumpInts(): 1 row	count	INTR	TX	RX	Tick 
[I][esp32-hal-i2c.c:345] i2cDumpInts(): [01]	0x0001	0x0002	0x0003	0x0000	0x00005d91
[I][esp32-hal-i2c.c:345] i2cDumpInts(): [02]	0x0001	0x0200	0x0000	0x0000	0x00005d91
[I][esp32-hal-i2c.c:345] i2cDumpInts(): [03]	0x0001	0x0020	0x0000	0x0000	0x00005d91

Please take a look.

Also in line 409 of esp32-hal-i2c.c it should be:

} else k += sprintf(&buf[k],"%04X ",fifoBuffer[i]);

You have a space between % and 4.

@stickbreaker
Forgot DebugFlags Return
e5851bf 
@andriyadi found this, total brain fade on my part.
@stickbreaker
Copy link
Contributor Author

@andriyadi does that LSM6DSL sensor, SDA low busy signaling? Error 5 is an Arbitration error:

// internal Error condition
typedef enum {
    //  I2C_NONE=0,
    I2C_OK=1,
    I2C_ERROR,
    I2C_ADDR_NAK,
    I2C_DATA_NAK,
    I2C_ARBITRATION,
    I2C_TIMEOUT
} I2C_ERROR_t;

What that is saying is the ESP32 released SDA from a LOW, waited a specified number cpu clockcycles, then re-read SDA. SDA was still LOW when it want it to be high, so this generated an arbitration error. The last interrupt in the dumpInts is 0x0020 with is the arbitration failure interrupt.

This error could also be created if the pullups on the i2c bus are weak. if SDA does not rise fast enough if condition will be be generated.

The errorByte and errorQueue values tells that the First byte of the requestFrom() is where it failed. It successfully send the Read request, the Slave ACK'd its address. I think the first place in the i/o stream where this arbitration failure could have been detected is at the ACK bit of first byte read from the slave, it the slave is holding SDA low as a busy, at the 9th bit clock (SCL) the slave is suppose to release SDA and allow the master(ESP32) do drive it low as an ACK. If the Slave was still driving SDA as a busy signal this would cause the Arbitration fault.

I see you are using the second I2C bus. Do you use both at the same time?

Chuck.

p.s that "% 4X" is correct I want the sprintf to generate a 4 character wide HEX field with space (' ') as the fill character instead of the standard '0' fill.

This was referenced Aug 6, 2018
Closed
Closed
@ifrew
Copy link

ifrew commented Aug 9, 2018

Hi @stickbreaker ,

Loaded up the latest esp32 ardunio core yesterday and my MPU9250 invensense library is failing to read the device. I found this thread and I can see the changes you have done. Many thanks! Looking through the driver code for wire.endtransmission(false) and wire.requestfrom(addr,size,false) I see two methods that need changed. What's the best way with the proposed changes here Chuck?

cheers

iain

uint8_t MPU9250::readByte(uint8_t address, uint8_t subAddress)
{
  uint8_t data; // `data` will store the register data   
  Wire.beginTransmission(address);         // Initialize the Tx buffer
  Wire.write(subAddress);                  // Put slave register address in Tx buffer
  Wire.endTransmission(false);             // Send the Tx buffer, but send a restart to keep connection alive
  Wire.requestFrom(address, (uint8_t) 1);  // Read one byte from slave register address 
  data = Wire.read();                      // Fill Rx buffer with result
  return data;                             // Return data read from slave register
}

void MPU9250::readBytes(uint8_t address, uint8_t subAddress, uint8_t count,
                        uint8_t * dest)
{  
  Wire.beginTransmission(address);   // Initialize the Tx buffer
  Wire.write(subAddress);            // Put slave register address in Tx buffer
  Wire.endTransmission(false);       // Send the Tx buffer, but send a restart to keep connection alive
  uint8_t i = 0;
  Wire.requestFrom(address, count);  // Read bytes from slave register address 
  while (Wire.available()) {
    dest[i++] = Wire.read(); }         // Put read results in the Rx buffer
}

@stickbreaker
Copy link
Contributor Author

@ifrew If your sensor has to have ReSTART for its data sampling protocol, you have two choices;

  • install this PR
  • roll back to rc3

You can try changing the sampling code to not have ReSTART, It may not actually be necessary for the proper operation of the sensor. The comment is kind of questionable, I don't know about the use of 'connection' in i2c parlance. Try this simple change, It's quick, and may work.

  Wire.endTransmission();             // Send the Tx buffer, but send a restart to keep connection alive

I introduced this ReSTART failure with PR#1665 when I changed the ISR to no longer use the I2C_MASTER_TRAN_COMP interrupt. PR 1665 solved a interrupt saturation problem. But caused this ReSTART problem. This new PR seem to function correctly, I have ran it through more testing.

Chuck.

@ifrew
Copy link

ifrew commented Aug 9, 2018 via email

@stickbreaker
Copy link
Contributor Author

@ifrew ReSTART doesn't work in RC4 or Release 1.0.0, There is an internal position index that decides where to store incoming data, pr1665 did not increment it correctly between the Write(ReSTART) element and the Read element. The ISR rxFifoEmpty() detects this error and fails. So, no data is stored into the Read buffer.

The code you published does not check the return status of endTransmission() so it never sees the different return code. The endTransmission(false) will always return 7 (I2C_ERROR_CONTINUE) to indicate the Write transaction is pending. If your code was:

if( !Wire.endTransmission(false)){ // do read
}else { // die horribly 
}

Then, you would have to change the if to:

if(  Wire.endTransmission(false) == 7 ){ // do read
}else { // die horribly 
}

Chuck.

@ifrew
Copy link

ifrew commented Aug 9, 2018 via email

@stickbreaker stickbreaker mentioned this pull request Aug 12, 2018
Closed
@me-no-dev me-no-dev merged commit b05430c into espressif:master Aug 14, 2018
@me-no-dev me-no-dev deleted the patch-2 branch August 14, 2018 09:51
tijnkooijmans pushed a commit to StudioSophisti/arduino-esp32 that referenced this pull request Aug 16, 2018
@stickbreaker @tijnkooijmans
Wire ReSTART fix, with others (espressif#1717)
dc04e2a
Curclamas pushed a commit to Curclamas/arduino-esp32 that referenced this pull request Aug 21, 2018
@stickbreaker @Curclamas
Wire ReSTART fix, with others (espressif#1717)
e36cacc 
* ReSTART fix, Sequencing fix

pr espressif#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 espressif#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.
@CaptIgmu
Copy link
Contributor

@ifrew Those MPU9250 routines are from @kriswiner and based on using a Teensy board. Similar routines are used for the EM7180 Sensor Hub based Ultimate Sensor Fusion Solution - MPU9250 and the newer more accurate Ultimate Sensor Fusion Solution - LSM6DSM + LIS2MD. I struggled for a long time to get reliable ESP32 i2c reads of my original USFS board until @stickbreaker merged his improved I2C routines up to v1.0.0-rc3. After that, I2C reads broke again! It was the comment from @stickbreaker: You can try changing the sampling code to not have ReSTART, It may not actually be necessary for the proper operation of the sensor. Correct! The ESP32 is not like the Teensy ARM Cortex boards, so we do not need this for i2c reads:

Wire.endTransmission(false); // Send the Tx buffer, but send a restart to keep connection alive

My code originally looked like this:

Wire.endTransmission(I2C_NOSTOP); // Send the Tx buffer, but send a restart to keep connection alive

So I changed my define at program start:

// #define I2C_NOSTOP false  // for compatibility with Teensy i2c to keep connection Alive for reads but BREAKS ESP32 i2c system! Not needed.
#define I2C_NOSTOP true    // This works for improved i2c routines from @StickBreaker    RMB ***

So now I can again communicate with my USFS board! All is not perfect though, my @kriswiner based code is interrupt driven and over time, some interrupts are missed. There may need to be more ongoing tweaks to the @stickbreaker i2c routines . . .

@kriswiner
Copy link

kriswiner commented Sep 15, 2018 via email

@stickbreaker
Copy link
Contributor Author

@CaptIgmu Release 1.0.0 does not work with ReSTART operations (Wire.endTransmission(false);)
This problem has been fixed in the current GitHub code.

So you can recode your I2C operations not to use ReSTART operations, or update to the current GitHub Code. You can just grab the 4 GitHub i2c files and overwrite the ones in Release 1.0.0

  • cores\esp32\esp32-hal-i2c.h
  • cores\esp32\esp32-hal-i2c.c
  • libraries\Wire\src\Wire.h
  • libraries\Wire\src\Wire.cpp

Chuck.

@stickbreaker stickbreaker mentioned this pull request Sep 20, 2018
Closed
stickbreaker added a commit to stickbreaker/arduino-esp32 that referenced this pull request Sep 20, 2018
@stickbreaker
Reduce resource requirements, Share Interrupt
fc728e0 
espressif#1869 exposed a resource exhaustion issue. The current HAL layer for I2C support is designed to use a shared interrupt, But, during debugging to solve the interrupt overloading condition identified in espressif#1588, and the generation of pr espressif#1717, the interrupt allocation parameters were changed.  This change was unnecessary, the code will work successfully with shared interrupts.  So, there is no need to assign a private interrupt for each I2C peripheral.
@stickbreaker stickbreaker mentioned this pull request Sep 20, 2018
Merged
me-no-dev pushed a commit that referenced this pull request Sep 21, 2018
@stickbreaker @me-no-dev
Reduce resource requirements, Share Interrupt (#1877)
e5ea089 
#1869 exposed a resource exhaustion issue. The current HAL layer for I2C support is designed to use a shared interrupt, But, during debugging to solve the interrupt overloading condition identified in #1588, and the generation of pr #1717, the interrupt allocation parameters were changed.  This change was unnecessary, the code will work successfully with shared interrupts.  So, there is no need to assign a private interrupt for each I2C peripheral.
@CaptIgmu
Copy link
Contributor

@stickbreaker: Even with latest code, cannot use ReStart in Wire.endTransmission(false) in I2C read routines from @kriswiner EM7180 based Ultimate Sensor Fusion Solution - MPU9250; won't run. Works fine with Wire.endTransmission(true). With his example code, I also had to use polling in my main loop instead of interrupts, to get the I2C reads to be reliable at 400 KHz clock with no hangs. This might be improved with #1877, I will try interrupts again! Seems the ESP32 I2C and USFS board have some relationship troubles . . .

@CaptIgmu CaptIgmu mentioned this pull request Sep 29, 2018
Open
@edlman
Copy link

edlman commented Aug 8, 2019

Hello, I have the same problem as described here. I use platformio framework with core/esp32-hal-i2c.{c,h} and library/Wire/Wire.{cpp,h} which match with stickbreaker/arduino-esp32 (branch patch-3). So I suppose it should support I2C restart.
I have Wemos Lolin 32 (ESP32) controller and two I2C devices - INA219 for current/voltage measurement and MPU6050 accelerometer.
Sometime (after few minutes, hours or even days) I2C reports failure but it doesn't restart.

[E][esp32-hal-i2c.c:318] i2cDumpI2c(): i2c=0x3ffbec40
[I][esp32-hal-i2c.c:319] i2cDumpI2c(): dev=0x60013000 date=0x16042000
[I][esp32-hal-i2c.c:321] i2cDumpI2c(): lock=0x3ffbb32c
[I][esp32-hal-i2c.c:323] i2cDumpI2c(): num=0
[I][esp32-hal-i2c.c:324] i2cDumpI2c(): mode=1
[I][esp32-hal-i2c.c:325] i2cDumpI2c(): stage=3
[I][esp32-hal-i2c.c:326] i2cDumpI2c(): error=5
[I][esp32-hal-i2c.c:327] i2cDumpI2c(): event=0x3ffbb3b0 bits=112
[I][esp32-hal-i2c.c:328] i2cDumpI2c(): intr_handle=0x3ffbb3e0
[I][esp32-hal-i2c.c:329] i2cDumpI2c(): dq=0x3ffbbd70
[I][esp32-hal-i2c.c:330] i2cDumpI2c(): queueCount=1
[I][esp32-hal-i2c.c:331] i2cDumpI2c(): queuePos=0
[I][esp32-hal-i2c.c:332] i2cDumpI2c(): errorByteCnt=0
[I][esp32-hal-i2c.c:333] i2cDumpI2c(): errorQueue=1
[I][esp32-hal-i2c.c:334] i2cDumpI2c(): debugFlags=0x00000000
[I][esp32-hal-i2c.c:311] i2cDumpDqData(): Debug Buffer not Enabled
[I][esp32-hal-i2c.c:354] i2cDumpInts(): Debug Buffer not Enabled
[I][esp32-hal-i2c.c:1130] i2cProcQueue(): Bus busy, reinit

I tried to set up SCL and SDA pins to INPUT_PULLUP

  pinMode(SDA,INPUT_PULLUP);
  pinMode(SCL,INPUT_PULLUP);

I didn't try to add extra pullup resistors as suggested as both devices (MPU6050 and INA219) have these already on boards (2k2 on MPU6050, see https://protosupplies.com/wp-content/uploads/2019/03/MPU-6050-GY-521-Accelerometer-Schematic.jpg, and 10k on INA219, see https://e2e.ti.com/resized-image/__size/1230x0/__key/communityserver-discussions-components-files/14/4314.7585.ina219.png)

When I detect I have no data from MPU6050 or INA219 I try to reset I2C. I tried many call, none helped

Wire.endTransmission();
mpu6050->resetSensors(); // these calls reset some I2C bits
mpu6050->resetI2CMaster();
mpu6050->reset();

The only way to make it working again is to power cycle (off & on) the device. Can someone help? What should I do or try? The only thing I can think of is to add some relay controlled by the ESP32 to cut the power off and thus power cycle the device. But it's not what I want.

@atanisoft
Copy link
Collaborator

@edlman please open an issue with a reproducible case and all debug logs that you can provide. Posting on an old merged PR is not the right approach to seek support.

@stickbreaker
Copy link
Contributor Author

@edlman This patch is ancient. It is for an old version 1.0.0. Please open an issue, fill out the issue template. The current release 1.0.2 has been stable. My personal branch is out of date. I would recommend you use this repo's version. either 1.0.2 or the pre-release 1.0.3rc1. Both of them use the same i2c code.

Chuck.

@atanisoft
Copy link
Collaborator

@stickbreaker I'd suggest cleanup/archive your fork to prevent people from intentionally using it going forward.

@edlman
Copy link

edlman commented Aug 8, 2019

Ok, where shall I open new bug report? At the original espressif:master? Or at stickbreaker:patch-3?

@atanisoft
Copy link
Collaborator

@edlman open on https://github.com/espressif/arduino-esp32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@stickbreaker @emptynick @andriyadi @ifrew @CaptIgmu @kriswiner @edlman @atanisoft @me-no-dev