I2C Master no longer supports repeated start condition across multiple command links as of ESP-IDF v4.4 (IDFGH-6719)

andycarle commented 2 years ago

Environment

Development Kit: Moddable Two
Module or chip used: ESP32-WROOM-32D
IDF version: v4.4
Build System: idf.py
Compiler version: xtensa-esp32-elf-gcc (crosstool-NG esp-2021r2-patch2) 8.4.0
Operating System: Windows
(Windows only) environment type: ESP Command Prompt
Using an IDE?: No
Power Supply: USB

Problem Description

In ESP-IDF v4.4, I2C command links that do not terminate with a stop bit (i2c_master_stop) cause a crash.

For instance, this code:

void write(uint8_t address, const uint8_t *buffer, uint16_t length, uint8_t sendStop)
{
    i2c_cmd_handle_t cmd;

    cmd = i2c_cmd_link_create();
    i2c_master_start(cmd);
    i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_WRITE, 1);
    i2c_master_write(cmd, (uint8_t *)buffer, length, 1);
    if (sendStop)
        i2c_master_stop(cmd);
    ret = i2c_master_cmd_begin(I2C_NUM_1, cmd, 1000 / portTICK_RATE_MS);
    i2c_cmd_link_delete(cmd);
}

crashes when sendStop is false. The crash is (from idf.py monitor):

Remote debugging using \\.\COM5
i2c_master_cmd_begin_static (i2c_num=1) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:1345
1345                    p_i2c->cmd_link.head->cmd.bytes_used = 0;
(gdb) bt
#0  i2c_master_cmd_begin_static (i2c_num=1) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:1345
#1  0x4008302e in i2c_isr_handler_default (arg=0x3ffb60b8) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:500
#2  0x40082330 in _xt_lowint1 () at C:/Users/andyc/esp32/esp-idf/components/freertos/port/xtensa\xtensa_vectors.S:1111
#3  0x40116434 in ?? () at C:/Users/andyc/esp32/esp-idf/components/esp_system/freertos_hooks.c:39

The same code does not crash when sendStop is true.

This crash makes it impossible to create sequences of I2C writes and reads with repeated start conditions across multiple I2C command links. This ability is a requirement for implementing the ECMA-419 ECMAScript embedded systems API specification on top of the ESP-IDF. More generally, this ability is needed for any application framework that allows construction of raw I2C and SMBus read/write sequences by stringing together atomic I2C read and write commands, such as is supported in the Moddable SDK's I2C implementation.

I2C command links without stop bits at the end worked as expected in ESP-IDF v4.3.2. This issue is new to ESP-IDF v4.4.

Expected Behavior

I2C command links that do not end with a stop bit do not crash.

Actual Behavior

I2C command links that do not end with a stop bit crash.

Debug Logs

i2c_master_cmd_begin_static (i2c_num=1) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:1345
1345                    p_i2c->cmd_link.head->cmd.bytes_used = 0;
(gdb) bt
#0  i2c_master_cmd_begin_static (i2c_num=1) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:1345
#1  0x4008302e in i2c_isr_handler_default (arg=0x3ffb60b8) at C:/Users/andyc/esp32/esp-idf/components/driver/i2c.c:500
#2  0x40082330 in _xt_lowint1 () at C:/Users/andyc/esp32/esp-idf/components/freertos/port/xtensa\xtensa_vectors.S:1111
#3  0x40116434 in ?? () at C:/Users/andyc/esp32/esp-idf/components/esp_system/freertos_hooks.c:39

suslic2012 commented 2 years ago

I have the same problem. The following quick fix seems to rectify the problem:

The problematic line 1345 should have a null-pointer guard, like so:

if(p_i2c->cmd_link.head) {
    p_i2c->cmd_link.head->cmd.bytes_used = 0;
}

andycarle commented 2 years ago

@suslic2012 Agreed that that is probably the right fix. Perhaps one of us should just put in a PR to that effect referencing this issue and see if anyone disagrees.

o-marshmallow commented 2 years ago

Hi @andycarle and @suslic2012 , Thanks for reporting this issue (and for the PR), I am going to handle this.

Small remark regarding the snippet of code you showed. In 4.4, you can now provide a buffer to i2c_cmd_link_create_static in order to create a static command link (macro I2C_LINK_RECOMMENDED_SIZE can help for determining the minimum size required for a particular transfer). This is handy if you want to avoid dynamic allocations, increasing program's speed.

Check this function here: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/i2c.html#_CPPv426i2c_cmd_link_create_staticP7uint8_t8uint32_t

Another new feature is the possibility to reuse already-sent command links. Let's say that you need to send a buffer multiple times on the bus and only few bytes change across the calls, you can use this feature to only modify the bytes that need to within your buffer and then re-use the same I2C command link to start a new transfer. This is useful for framebuffers, for example.

andycarle commented 2 years ago

@o-marshmallow Nice; thank you for the heads-up for and taking a look at this issue!

AxelLin commented 2 years ago

Fix for v4.4: f60a914

o-marshmallow commented 2 years ago

Thanks @AxelLin for the commit number! Closing this issue

not-my-real-name commented 2 years ago

Can you also look at adding the same thing on line 1289?

EDIT: Opened a new issue #8548

espressif / esp-idf