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.

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

[ESP32] I2C Interrupt problem since uprade to Espressif 1.1.0 #646

Closed
cyberman54 opened this issue Jul 3, 2018 · 29 comments
Closed

[ESP32] I2C Interrupt problem since uprade to Espressif 1.1.0 #646

cyberman54 opened this issue Jul 3, 2018 · 29 comments

Comments

@cyberman54
Copy link

I'm using u8x8 on ESP32 boards, programmed with Arduino Espressif32 core.
I2C display handling using driver U8X8_SSD1306_128X64_NONAME_HW_I2C

This worked perfectly until last days the Espressif32 core was updated from v1.0.3 to v1.1.0.

Since then i get this error during runtime:
[E][esp32-hal-i2c.c:594] i2c_isr_handler_default(): eject int=0x0, ena=0x0

And the display write speed is significantly reduced.

I'm not sure if this is a problem with u8x8 or the Espressif32 core. I'm not using any other I2C communication on the board.

@olikraus
Copy link
Owner

olikraus commented Jul 4, 2018

Sounds strange. U8g2 just uses the standard Arduino layer. I assume something is broken in the Espressif32 core.

@cyberman54
Copy link
Author

I opened issue in arduino-espressif32 on this:

espressif/arduino-esp32#1588

@cyberman54
Copy link
Author

This bug is solved in next version of core v1.1.x

espressif/arduino-esp32#1665

@cyberman54
Copy link
Author

cyberman54 commented Jul 25, 2018

@olikraus I reopen this issue since the problem persists, even after the i2c code in Espressif v1.1.2 was reviewed and refactored by the author. The author assumes that the i2c handling of the display drivers causes the perfomance problem:

Have you went through your U8X8_SSD1306_128X64_NONAME_HW_I2C verifying that it handles ReSTARTS correctly?
The esp32 hardware handles ReSTARTS differently than AVR (standard Arduino). Possibly the library is having issues.

espressif/arduino-esp32#1588

@cyberman54 cyberman54 reopened this Jul 25, 2018
@olikraus
Copy link
Owner

U8g2 just uses the standard Arduino IDE. Maybe you could do one thing, locate the following line on your local PC:
https://github.com/olikraus/u8g2/blob/master/cppsrc/U8x8lib.cpp#L1002
Change this to

Wire.endTransmission(false);

Will this help?

@cyberman54
Copy link
Author

Tested the change as suggested.
Result ist that display is no more functional and a huge number or errors is thrown:

[E][esp32-hal-i2c.c:792] i2cAddQueue(): realloc Failure

@stickbreaker

@stickbreaker
Copy link

@cyberman54 can you provide me with a test sketch that compiles u8g2. And works on that heltec? I haven't found the correct combinations.

Just something simple, 'hello World!'

Chuck.

@cyberman54
Copy link
Author

@stickbreaker Here's a simple "hello world" for Heltec Lora-32 board:

#include <Arduino.h>
#include <U8x8lib.h>

// Hardware pin definitions for Heltec LoRa-32 Board with OLED SSD1306 I2C Display
#define OLED_RST GPIO_NUM_16 // ESP32 GPIO16 (Pin16) -- SD1306 RST
#define OLED_SDA GPIO_NUM_4  // ESP32 GPIO4 (Pin4)   -- SD1306 D1+D2
#define OLED_SCL GPIO_NUM_15 // ESP32 GPIO15 (Pin15) -- SD1306 D0

// use i2c software emulation
//U8X8_SSD1306_128X64_NONAME_SW_I2C u8x8(OLED_SCL,OLED_SDA,OLED_RST);

// use i2c hardware pins of esp32
U8X8_SSD1306_128X64_NONAME_HW_I2C u8x8(OLED_RST, OLED_SCL, OLED_SDA);

void setup()
{
    pinMode(OLED_RST, OUTPUT);
    digitalWrite(OLED_RST, LOW); // set GPIO16 low to reset OLED
    delay(50);
    digitalWrite(OLED_RST, HIGH); // while OLED is running, must set GPIO16 in high

    u8x8.begin();
    u8x8.setPowerSave(0);
    u8x8.setFont(u8x8_font_chroma48medium8_r);
    u8x8.setCursor(0, 0);
}

void loop()
{
    // put your main code here, to run repeatedly:
    u8x8.println("Hello World");
}

@stickbreaker
Copy link

Thanks,
I'll play with it today.

Chuck.

@stickbreaker
Copy link

@cyberman54
Realloc failure is an out of memory condition. Heap is exhausted.

Chuck.

@cyberman54
Copy link
Author

@stickbreaker hmm, don't know what's going wrong in u8g2.
Could find anything why the performance is slowed down in u8g2 since the espressif core update?
Maybe i should ponder to change u8g2 with adafruit ssd1306. But this means lots of re-coding and re-testing :-(

@stickbreaker
Copy link

I haven't had time to do any coding since last week. One thing i noticed, is that is changes clock rate in every transaction?

I read through the library. Last week. Still haven't had time to try your example. Soon, hopefully.

@olikraus
Copy link
Owner

Not sure whether this helps, but u8g2 depends a lot on the gcc linker garbage collector. But in history this never had been a problem with ESP32.

@stickbreaker
Copy link

just fixed a deep problem in i2c, pr 1717 espressif/arduino. If you get a chance try it. I'll start testing u8g2 tomorrow.

Chuck.

@stickbreaker
Copy link

@cyberman54 I give up on trying to decode u8g2, I cannot discover the flow.
One problem I found is the every transaction changes the clock rate of i2c?
Looking at my scope it sends a bunch of small transactions with delays between them.

I modified the adafruit library only send the bits of screen buffer that changed, and to send the screen update in one block(1025 bytes max). This dramatically increased performance.

I have not been able to identify where the screen memory buffer exists in u8g2. On the scope I see a regular pattern of four- four byte commands and one - ten byte command, I haven't located a big screen memory update.

If I figure out how this library is designed I might be able to re-order it to take advantage of the esp32 ability to move big memory blocks with

  i2c_err_t writeTransmission(uint16_t address, uint8_t* buff, uint16_t size, bool sendStop=true);

i2c_err_t is just a byte error code, it follows the standard Arduino Wire() error values.

Chuck.

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

If I figure out how this library is designed I might be able to re-order it to take advantage of the esp32 ability to move big memory blocks with

The block transfer for hardware I2C happens here:
https://github.com/olikraus/u8g2/blob/master/cppsrc/U8x8lib.cpp#L970

@stickbreaker
Copy link

@olikraus can you describe the design philosophy? I looked at the i2c bus signals and see a repeating pattern of 4 4byte commands followed by a 10 byte command. Is it sending a update to the screen for every character? There is a delay between each command. Based on the o'scope, it is spending more time in the delays than data transfer.

I rewrote AdaFruits SSD1306 to send a rectangle of changed bytes. Adafruits library modifies a screen buffer then updates the screen when display() is called. Is this library similar?

I understand your hardware abstraction layer, but I can't find how it is used. The ESP32 can move big blocks of i2c data with little overhead, but using Wire.write() incurs a lot of overhead. Bytes are moved, buffer positions validated, Function are repeatedly called. Wire.writeTransaction() never directly touches the data, only the i2c ISR moves it once. Dramatically lower overhead.

If my understanding and guesses of how data is moved through this library is correct less than 15% of the i2c traffic is display data, the rest is control data.

Can you tell me why you change I2C bus speed every transaction? You have a call to Wire.setClock() before every Wire.beginTransmission()?

Chuck

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

The ESP32 can move big blocks of i2c data with little overhead,

Yes but the Arduino wire implementation is limited to 32 bytes.
Actually the above layer for i2c is here:
https://github.com/olikraus/u8g2/blob/master/csrc/u8x8_cad.c#L410

The data stream is cut down into pieces of 24 bytes or smaller. And yes, then I do another wire.beginTransmission()

According to the SSD1306 datasheet the status of the DC line must be sent after the address. So if I change the DC line I have to do another wire.beginTransmission()
hmm... currently I have this:

      p = arg_ptr;
       while( arg_int > 24 )
      {
	u8x8_i2c_data_transfer(u8x8, 24, p);
	arg_int-=24;
	p+=24;
      }
      u8x8_i2c_data_transfer(u8x8, arg_int, p);
break;

Which is:

      p = arg_ptr;
       while( arg_int > 24 )
      {
        u8x8_byte_StartTransfer(u8x8);    
        u8x8_byte_SendByte(u8x8, 0x040);
        u8x8->byte_cb(u8x8, U8X8_MSG_CAD_SEND_DATA, 24, p);
        u8x8_byte_EndTransfer(u8x8);
	arg_int-=24;
	p+=24;
      }
        u8x8_byte_StartTransfer(u8x8);    
        u8x8_byte_SendByte(u8x8, 0x040);
        u8x8->byte_cb(u8x8, U8X8_MSG_CAD_SEND_DATA, arg_int, p);
        u8x8_byte_EndTransfer(u8x8);
break;

So maybe this would be better:

      p = arg_ptr;
        u8x8_byte_StartTransfer(u8x8);    
        u8x8_byte_SendByte(u8x8, 0x040);
       while( arg_int > 24 )
      {
        u8x8->byte_cb(u8x8, U8X8_MSG_CAD_SEND_DATA, 24, p);
	arg_int-=24;
	p+=24;
      }
        u8x8->byte_cb(u8x8, U8X8_MSG_CAD_SEND_DATA, arg_int, p);
        u8x8_byte_EndTransfer(u8x8);
break;

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

ah, no, this does not work. Arduino Wire will queue everything and only transfer the last 32 bytes.

I do not know how this problem is solved by Adafruit, but if I want to be compatible and stick to the Arduino Wire Spec, then I have to do this kind of calling of 'wire.beginTransmission()' .

https://www.arduino.cc/en/Reference/Wire

The Wire library implementation uses a 32 byte buffer, therefore any communication should be within this limit. Exceeding bytes in a single transmission will just be dropped.

The buffer is only flushed via wire.endTransmission()

Again, I think there is a problem in the ESP32 implementation. And yes, I do call setClock and beginTransmission very often. But I must do so because of the Arduino Spec.

of course... i could use 31 instead of 24...

@stickbreaker
Copy link

This is the code I use for AdaFruit, Adafruit uses a 1025byte buffer, 1024bytes of screen bits, with a single 'data' marker byte at the beginning.

void SSD1306::display(void) {
// bounding box 2018/07/12
  if(boundX0<=boundX1){
    if((boundX0>boundX1)||(boundY0>boundY1)||(boundX1>(SSD1306_LCDWIDTH-1))||(boundY1>(SSD1306_LCDHEIGHT-1))){
      Serial.printf("(%d,%d)-(%d,%d)\n",boundX0,boundY0,boundX1, boundY1);
      abort();
    }
  } else {
    boundX0 = 0;
    boundX1 = SSD1306_LCDWIDTH-1;
    boundY0 = 0;
    boundY1 = SSD1306_LCDHEIGHT -1;
  }
  ssd1306_command(SSD1306_COLUMNADDR);
  ssd1306_command(boundX0);   // Column start address (0 = reset)
  ssd1306_command(boundX1); // Column end address (127 = reset)

  ssd1306_command(SSD1306_PAGEADDR);
  ssd1306_command(boundY0/8); // Page start address (0 = reset)
  ssd1306_command(boundY1/8); // Page end address

// end bounding box
    if (((boundX0 != 0)||(boundX1 !=SSD1306_LCDWIDTH-1)||(boundY0 !=0)||(boundY1!=SSD1306_LCDHEIGHT-1))&&
     (boundX0<=boundX1)){ // partial screen update
      uint16_t b = ((boundY0 / 8)*SSD1306_LCDWIDTH)+boundX0; // buffer is offset by 1 to make room for data marker
      uint16_t len = (boundX1 - boundX0) +2; //data marker + 1
      uint16_t lines = ((boundY1/8) - (boundY0/8))+1;
      do{
         //each line
        uint8_t save = buffer[b];
        buffer[b] = 0x40; // inject data marker
        uint8_t err = Wire.writeTransmission(_i2caddr,(uint8_t*)&buffer[b],len);
        buffer[b] = save; // restore screen data
        b += SSD1306_LCDWIDTH;        
        if(err!=0){
          log_e("partial update Failure=%d (%s)",err,Wire.getErrorText(err));
        }
      lines--;  
      }while(lines>0);
    } else{ // whole screen update    
      buffer[0] = 0x40; // Data marker for ssd1306e
      uint8_t err = Wire.writeTransmission(_i2caddr,buffer,(SSD1306_LCDWIDTH*SSD1306_LCDHEIGHT/8)+1);
      if(err!=0){
        log_e("update Failure=%d (%s)",err,Wire.getErrorText(err));
      }
    }
    
  // reset modified box to none modified.  
  boundX0 = SSD1306_LCDWIDTH-1;
  boundX1 = 0;
  boundY0 = SSD1306_LCDHEIGHT-1;
  boundY1 = 0;
  }
}

Chuck.

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

What is "writeTransmission"?

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

This is the code, which I found:
https://github.com/adafruit/Adafruit_SSD1306/blob/master/Adafruit_SSD1306.cpp#L469

Does NOT look like the one you have provided.

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

So here for reference. The official Adafruit code does exactly the same as the U8g2 code:

    for (uint16_t i=0; i<(SSD1306_LCDWIDTH*SSD1306_LCDHEIGHT/8); i++) {
      // send a bunch of data in one xmission
      Wire.beginTransmission(_i2caddr);
      WIRE_WRITE(0x40);
      for (uint8_t x=0; x<16; x++) {
        WIRE_WRITE(buffer[i]);
        i++;
      }
      i--;
      Wire.endTransmission();
}

WIRE_WRITE is defined as Wire.write
So all in all they restrict themselves to 16 bytes (U8g2 uses 24 bytes).

@stickbreaker
Copy link

@olikraus as I said before:

I modified the adafruit library only send the bits of screen buffer that changed, and to send the screen update in one block(1025 bytes max). This dramatically increased performance.

Modified AdaFruit_SSD1306 here

Sorry I confused you, I created a fork of adafruit_ssd1306 and renamed it SSD1306, It is just a fast hack and slash optimization for the ESP32. I plan on just cutting it down to I2C only without the general purpose SPI/I2C support.

This error thread started out with a performance problem in my implementation of I2C on ESP32. My ISR code was overwhelming the OS with interrupts on every byte transfer. I was using these byte counts to increment buffers, buffer positions. I ended up rewriting the ISR to not need these counts. I now calculate absolute positions when needed(on errors).

I am looking through your code to see how much effort it would be to implement optimization for the ESP32. This is self serving on my part, I can't see going back to AVR's under Arduino.

When your code calls Wire.setClock() the hal layer has to go through a MUTEX to acquire the hardware before it can change the bus clock rate.

I don't recommend changing the clock rate continuously. I recommend setting the clock rate in the init code Wire.begin() stage.

From an electronic perspective, the clock rate is governed by the physical characteristics of bus capacitance, pullup values and slave device performance. I2C specs only require 3mA drive current to be compatible, the esp32 can drive 12mA max. With my modified SSD1306 running on a HelTec Lora board, I was able to update the display at 500KHz before over running the display. I had to add 3.3k pullups to achieve these speeds, the built in 10k pullup result is a distorted sawtooth waveform at 400KHz. I was able to drive i2c at 1.2MHz with good electrical signals, but the display lost it.

What is "writeTransmission"?

WriteTransmission is a function added to Arduino-esp32 Wire() to take advantage of direct buffer
transmissions.

Chuck.

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

I don't recommend changing the clock rate continuously. I recommend setting the clock rate in the init code Wire.begin() stage.

Arduino users might use two different devices with different clock speed on the same I2C bus. I have to set the clock speed before each transfer.

When your code calls Wire.setClock() the hal layer has to go through a MUTEX to acquire the hardware before it can change the bus clock rate.

I assume, that many Arduino libraries will call setClock(). HAL should check whether the setClock arguments differs from the current clock rate. If it is equal, nothing needs to be done.

Sorry I confused you, I created a fork of adafruit_ssd1306 and renamed it SSD1306, It is just a fast hack and slash optimization for the ESP32. I plan on just cutting it down to I2C only without the general purpose SPI/I2C support.

Fine then, but you compare apples with beans. It is not fair to say that Adafruit Lib is better, if you actually mean a special optimized version for ESP32.. Goal for U8g2 is to work on all Arduino Boards. And if ESP32 project want to run all kind of Arduino Libs, than the Wire interface should stick to the Arduino Standard.

WriteTransmission is a function added to Arduino-esp32 Wire() to take advantage of direct buffer
transmissions.

Good for ESP32, but bad for being Arduino compatible. ESP32 people should better spent time to optimize the standard Arduino Wire interface so that it works reliable with all libraries instead of inventing new procedures which are incompatible with the Arduino Wire API.

Once success factor had been the fact, that there is a standard API.

U8g2 HW I2C now runs on all Arduino Boards except ESP32. I am definitly willing to support, but at the end, I still need to say, that this is a ESP32 problem if u8g2 doesn't work any more.

@v1993
Copy link

v1993 commented Aug 3, 2018

@olikraus As another user of your great library and also great ESP-32: why don't just write problematic code in two versions: for all arduino boards (already done) and for ESP-32 (using some ideas from discussion)? After all, ESP-32 wasn't planned to be arduino, but external framework tries to emulate API. As you can see, it isn't always does very vell…

@stickbreaker
Copy link

@olikraus It's not that it doesn't work, it is that it could be faster. If you are satisfied with the performance. I'll step back.

I disagree with your assertion of the necessity of changing clock rate on every transaction. I believe that shared resource should be politely shared. Your justification for re configuring a shared resource and then leaving it reconfigured doesn't seem polite to me. If you are not worried about optimum performance there is no need to change the clock rate. If you want to be polite restore, it when you are finished using it. Wire.getClock()

Clock rate on the bus is limited to the slowest slave on the bus, not the fastest, all slaves have to decode all START and STOP events if a slow slave cannot decode transactions then the bus may become unusable.

Thank you for your assistance understanding your library, and your reasoning for your design choices. In the end it is your code and your decisions that matter.

Chuck.

@olikraus
Copy link
Owner

olikraus commented Aug 3, 2018

ESP-32: why don't just write problematic code in two versions:

I would be happy to accept pull requests here. #ifdef should be there to distinguish the versions.

Clock rate on the bus is limited to the slowest slave on the bus, not the fastest, all slaves have to decode all START and STOP events if a slow slave cannot decode transactions then the bus may become unusable.

Agree, but the average Arduino user does not think to this extend. Most questions to me are: Why is SSD1306 so slow and why does the SSD1306 not work together with I2C slave XYZ?
A 100KHz I2C slave will not respond to the 400KHz address, so both users are happy. Display will work with 400KHz and I2C XYZ device can be used.

It would probably the most easiest optimiztion for the ESP32 HAL if you could check the setClock argument to avoid a reconfiguration of the same clock rate.

@olikraus
Copy link
Owner

Any results her? Can I close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants