espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.72k stars 7.3k forks source link

[TW#26747] I2C clock stretching duration too short (IDFGH-831) #2551

Open tuangu opened 6 years ago

tuangu commented 6 years ago

Environment

Problem Description

I am having a problem with the i2c driver. I am using a pressure sensor which stretches SCL line low for about 25 ms (p. 13 of the documentation). I checked the reference, p.290 and found that the maximum timeout allowed on ESP32 is just about 12.5 ms (0xFFFFF with unit APB 80Mhz clock cycle). Also, the i2c protocol does not specify the maximum stretching period. Any suggestion to work around this timeout limit to get the sensor working correctly?

negativekelvin commented 6 years ago

It says that 25ms delay is only for warm up and start conversion but after that you can read data faster so you should be able to work around it

tuangu commented 6 years ago

@negativekelvin Yes, I noticed that. The thing is I haven't found any workaround yet. Do you have any suggestion?

negativekelvin commented 6 years ago

Send the start conversion, delay at least 25ms, send the read command?

stickbreaker commented 6 years ago

@negativekelvin The timeout interrupt does not have to be treated as a fatal error. This is the code I use for Arduino-ESP32


        if (activeInt & I2C_TIME_OUT_INT_ST_M) {
            // let Gross timeout occur, Slave may release SCL before the configured timeout expires
            // the Statemachine only has a 13.1ms max timout, some Devices >500ms
            p_i2c->dev->int_clr.time_out =1;
            activeInt &=~I2C_TIME_OUT_INT_ST;
          // since a timeout occurred, capture the rxFifo data 
            emptyRxFifo(p_i2c);
            p_i2c->dev->int_clr.rx_fifo_full=1;
            p_i2c->dev->int_ena.rx_fifo_full=1; 
}

I just accept the interrupt, empty the RX fifo to move the stale data. My driver has a user configurable timeout. If a slave is stretching SCL, every 13.1ms a I2C_TIME_OUT_INT_ST_M will occur. But, the peripheral does not fail, it is just in a waiting status. I have manually held SCL LOW with a 1 second configured timeout and successfully completed the transactions.

I think the IDF i2c driver needs to be changed, A user selected timeout should be added. Either triggered by the I2C_TIME_OUT_INT_ST_M interrupt or something in this keep_alive queue call? i2c timeout code

       } else if (status & I2C_TIME_OUT_INT_ST_M) {
            I2C[i2c_num]->int_ena.time_out = 0;
            I2C[i2c_num]->int_clr.time_out = 1;
            p_i2c_obj[i2c_num]->status = I2C_STATUS_TIMEOUT;
            i2c_master_cmd_begin_static(i2c_num);

I don't think the timeout should cause this abort.

I don't directly use IDF, and haven't setup a compiling environment, so I can't offer any tested and usable code, sorry.

Chuck.

negativekelvin commented 6 years ago

@stickbreaker yes that makes sense if the i2c state machine is happy to continue then multiple timeout ints should be allowed. With this particular slave device it shouldn't be necessary to get it working.

koobest commented 5 years ago

Hi @tuangu Is this possible to read the data correctly if the master don't check the ACK when write start conversion command ? like this:

#define ACK_CHECK_EN   (0x1)
#define ACK_CHECK_DIS  (0x0)
#define WRITE_BIT      (0x0)
#define READ_BIT       (0x1) 

uint8_t sensorAddress = 0x5c; 
uint8_t startConversionCommand = 0x20;

esp_err_t i2c_master_sensor_test(i2c_port_t i2c_num, uint8_t *data_h, uint8_t *data_l)
{
    const uint8_t 
    int ret;
    i2c_cmd_handle_t cmd = i2c_cmd_link_create();
    i2c_master_start(cmd);
    i2c_master_write_byte(cmd, sensorAddress << 1 | WRITE_BIT, ACK_CHECK_EN);
    i2c_master_write_byte(cmd, startConversionCommand, ACK_CHECK_DIS); //dont't check the ack
    i2c_master_stop(cmd);
    ret = i2c_master_cmd_begin(i2c_num, cmd, 1000 / portTICK_RATE_MS);
    i2c_cmd_link_delete(cmd);
    if (ret != ESP_OK) {
        return ret;
    }
    vTaskDelay(30 / portTICK_RATE_MS);
    cmd = i2c_cmd_link_create();
    i2c_master_start(cmd);
    i2c_master_write_byte(cmd, sensorAddress << 1 | READ_BIT, ACK_CHECK_EN);
    i2c_master_read_byte(cmd, data_h, ACK_VAL);
    i2c_master_read_byte(cmd, data_l, NACK_VAL);
    i2c_master_stop(cmd);
    ret = i2c_master_cmd_begin(i2c_num, cmd, 1000 / portTICK_RATE_MS);
    i2c_cmd_link_delete(cmd);
    return ret;
}
Alvin1Zhang commented 5 years ago

@tuangu Hi, would you help share if any updates for this issue? Thanks.

tuangu commented 5 years ago

@Alvin1Zhang We don't need this kind of sensor any longer, so I haven't tried out any solution suggested above. In my opinion, one possible option is to bit-bang i2c to start the sensor, after that you can use the regular i2c driver. Please keep me updated if you find out something; I can try out myself if I have free time.

negativekelvin commented 5 years ago

IMO this should be kept open until i2c driver is updated to support longer timeout values by allowing multiple timout interrupts and counting them as suggest by chuck

Alvin1Zhang commented 5 years ago

@tuangu @negativekelvin Thanks for your feedbacks.

jdrewelow303 commented 5 years ago

Any updates on this issue? I would really like to use the ESP32 with a BNO055 sensor for a work project but I believe this is the exact issue that is keeping me from getting good results from the sensor.

yeti7 commented 5 years ago

Same issue here reading Si7021 sensor.

stickbreaker commented 5 years ago

I receive an update from a espressif hardware designer: when the timeout interrupt occurs, the hardware is reset. So, the transaction is aborted.

I previously though the timeout was just a warning, not a fatal error.

The only possibility for extending the timeout period greater than 13.1ms (80mhz/2**20) is to reduce the CPU clock.

The max timeout is a 20 bit value that counts APB clock cycles, the APB clock is either the CPU frequency or 80mhz which ever is lower. The i2c hardware will function correctly down to 3mhz. The Arduino-Esp32 code limits minimum Cpu clock to 10mhz, with a 10mhz clock the 20bit timeout is 104.8ms. If your app can support a reduced CPU clock then just reduce CPU clock, do the i2c transaction then restore the CPU clock. WiFi needs to be shutdown before you reduce the CPU clock and restarted after you speed it up.

Chuck.

negativekelvin commented 5 years ago

@stickbreaker that's unfortunate, I thought you had tested it with clearing the timeout interrupt and the transaction completed?

ajita02 commented 5 years ago

I have made few changes in i2c driver w.r.t. timeout interrupt. The user has to pass additional parameter of slave_response_time in i2c_config_t while calling i2c_param_config API. The slave_response_time has to be in milliseconds. @tuangu can you please try the attached patch against current esp-idf master and let me know the results. Thanks. 0001-feature-of-addtional-clock-stretching-period-for-i2c.zip

stickbreaker commented 5 years ago

@ajita02 I tried using your changes with arduino-esp32, the i2c peripheral never recovered from the clock stretching. I use a SHT25 temp/humidity sensor for testing. this capture shows the temperature sampling command, the Sensor starts stretching SCL in the READ. At about the 200.3ms time mark. SHT25 Read SCL Stretch This capture show the Sensor releasing SCL at 265ms time mark. SHT25 Read SCL end My slave timeout is about 200ms. at 440ms is when the ISR gives up. the i2c->dev->status_reg.val is 0x43000010 after the timeout interrupt occurs. This is a list of all interrupt triggered:

[I][esp32-hal-i2c.c:372] i2cDumpInts(): 0 row   count   INTR    TX  RX  Tick 
[I][esp32-hal-i2c.c:376] i2cDumpInts(): [01]    0x0001  0x0002  0x0001  0x0000  0x001dfed6  0x00000000
[I][esp32-hal-i2c.c:376] i2cDumpInts(): [02]    0x0001  0x0200  0x0000  0x0000  0x001dfed6  0x10040010
[I][esp32-hal-i2c.c:376] i2cDumpInts(): [03]    0x000e  0x0100  0x0000  0x0000  0x001dff9a  0x43000010

count : number of contiguous interrupts INTR : p_i2c->dev->int_status.val&0x7FF; // on entry into ISR TX : number of bytes added to txFifo during this ISR cycle RX: number or bytes read from rxFifo during this ISR cycle TICK: xTaskGetTickCountFromISR(); // when IRQ fired [unnamed] : p_i2c->dev->status_reg.val

this is the code I am trying in arduino-esp32,

       if (activeInt & I2C_TIME_OUT_INT_ST_M) {
            // let Gross timeout occur, Slave may release SCL before the configured timeout expires
            // the Statemachine only has a 13.1ms max timeout, some Devices >500ms
    //        p_i2c->dev->int_clr.time_out =1;
            activeInt &=~I2C_TIME_OUT_INT_ST;
            // since a timeout occurred, capture the rxFifo data 
            if((p_i2c->dev->status_reg.scl_main_state_last == 3) && (p_i2c->dev->status_reg.scl_state_last == 4)){
                p_i2c->dev->int_ena.time_out = 0;
                p_i2c->dev->int_clr.time_out = 1;
                p_i2c->dev->int_ena.time_out = 1;
                p_i2c->dev->timeout.tout--; // use tout to indicate number of times time_out occurred
                // timeout during SCL stretching and Read mode, exit, can't recover
//               i2cIsrExit(p_i2c,EVENT_ERROR_TIMEOUT,true);
            }
            else {
                p_i2c->dev->int_clr.time_out =1;
                emptyRxFifo(p_i2c);
                p_i2c->dev->int_clr.rx_fifo_full=1;
                p_i2c->dev->int_ena.rx_fifo_full=1; //why?
            }
        }

I am not using your slave_response_time, the timeout is handled outside of the ISR by the i2cdriver code with a xEventGroupWaitBits(), the ISR set the eventbits when it encounters error, or completes operations:

    //hang until it completes.

    // how many ticks should it take to transfer totalBytes through the I2C hardware,
    // add user supplied timeOutMillis to Calculated Value

    portTickType ticksTimeOut = ((totalBytes*10*1000)/(i2c->qb->actualClockFreq)+timeOutMillis)/portTICK_PERIOD_MS;

    i2c->dev->ctr.trans_start=1; // go for it

#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_ERROR
    portTickType tBefore=xTaskGetTickCount();
#endif

    // wait for ISR to complete the transfer, or until timeOut in case of bus fault, hardware problem

    uint32_t eBits = xEventGroupWaitBits(i2c->i2c_event,EVENT_DONE,pdFALSE,pdTRUE,ticksTimeOut);

#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_ERROR
    portTickType tAfter=xTaskGetTickCount();
#endif

Did is miss apply something from your patch? I did not see any code to release the peripheral from the timeout condition, other than disabling the time interrupt and re-enabling it. Chuck.

stickbreaker commented 5 years ago

@ajita02 Additional Info, this is a dump of the i2c command list after the timeout occurs:

 [ 0]   Y   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [ 1]   Y   WRITE   val[0]  exp[0]  en[1]   bytes[1]
 [ 2]   Y   WRITE   val[0]  exp[0]  en[1]   bytes[1]
 [ 3]   Y   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [ 4]   Y   WRITE   val[0]  exp[0]  en[1]   bytes[1]
 [ 5]   N   READ    val[0]  exp[0]  en[0]   bytes[2]
 [ 6]   N   READ    val[1]  exp[0]  en[0]   bytes[1]
 [ 7]   N   STOP    val[0]  exp[0]  en[0]   bytes[0]
 [ 8]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [ 9]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [10]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [11]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [12]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [13]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [14]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]
 [15]   N   RSTART  val[0]  exp[0]  en[0]   bytes[0]

Chuck.

bill1davis commented 5 years ago

Thanks for all the good info in this thread. The only thing that made me dig into the idf source tree was that it is not entirely obvious that you must call i2c_settimeout(i2c num, 0xFFFFF) to realize the 12ms stretch. The driver default timeout is much lower.

VishalDhayalan commented 4 years ago

Any updates on this issue? I would really like to use the ESP32 with a BNO055 sensor for a work project but I believe this is the exact issue that is keeping me from getting good results from the sensor.

Same here! Is there no way of stretching the clock for i2c yet?