i2c driver/module implementation

[loboris]

I have analized your implementation of i2c driver & lua module and found some problems/bugs.

First, the whole idea of esp-idf i2c master driver implementation (i2c_cmd_link_create -> commands -> i2c_master_cmd_begin) is to place complete master<->slave communication in one transaction which includes start condition, addres/rd|wr, data r/w, [repeated start, address/rd|wr, data r/w], stop condition.

In your current implementation, the master <->slave transaction is executed after each write or read command (i2c_flush in li2c_read & li2c_write functions) and all commands after the first read|write are executed as separate transactions. In most cases it will work (specially when single slave is present on the bus), but can potentially lead to problems (specially when multiple slaves are connected to the i2c bus).
I understand that you have to issue the i2c_flush command if you want to return the single read byte, but I don't understand why it is necessary after each write

// We need to flush because data buffers are on the stack and if we
// flush on the stop condition its values will be indeterminate

I would also suggest changing:

    i2c_master_read(cmd, (uint8_t *)data, len, 1);

in i2c driver function i2c_read to:

    if (len > 1) {
        i2c_master_read(cmd, (uint8_t *)data, len-1, 0);
    }
    i2c_master_read_byte(cmd, (uint8_t *)(data+len-1), 1);

so that reading multiple bytes from slave can handle ACK/NACK correctly.
With current implementation, reading multiple bytes will return wrong result (tested with BME280 sensor).

I understand your desire to have low level i2c commands (start, address, read, write, stop), it is great for educational purposes, but for all practical purposes I think you should consider implementing higher level functions (send, receive, send_receive), which would handle complete master<->slave transactions with data from/to Lua string, table or numbers.

Best regards.

[jolivepetrus]

Hi loboris,

I have read your comments carefully and I have not found any related issues about our strategy to flush buffer on every read, after solve the issue related to ACK/NACK on read multiple bytes from slave.

We have test our approach (using flush) with a logic analyzer and we haven't found any appreciate differences in i2c signals with your proposed approach (not using flush).

We have implemented our approach after analyzing esp-idf source code about link command, then we come to the conclusion that esp-idf link commands are something like a buffer that flush to the i2c hardware using i2c_master_cmd_begin function.

We would appreciate your help if you can test a multi-slave configuration using the flush approach after each read, because at this time we only have an EEPROM for test i2c.

About flush after each write. This have been solved in i2c_write when data length is 1, using i2c_master_write_byte which takes the data argument by value and not by reference, and now it's not required to flush after every write.

[loboris]

There's actually no problem with flashing after each byte as long as the stop command is sent at the end of start-address-read-write sequence.
At first I was little confused by flush function, then I realized that it is not necessary to use it if I use i2c_start|address|read|write functions from C function (for example components/lua_rtos/Lua/modules/sensors.c#BME280_I2C_bus_read and components/lua_rtos/Lua/modules/i2c.c#li2c_send|li2c_receive|li2c_sendreceive in my GitHub
The only real problem was multibyte reading which is solved now.
Thanks.