MAX32670 I2C Driver causes a Double-Read

[Brandon-Hurst]

Reading from a peripheral, it appears that the I2C driver for MAX32670 causes a double read.

What I Did

Connect an ADXL345 to I2C1 on MAX32670 EVKit. I am running the I2C example, and simply added the below source code before the main test runs...

    // FIXME: For testing
    uint8_t my_adxl_tx[1] = {0x0}; // ADXL ID Register
    uint8_t my_adxl_rx[2] = {0x0};
    mxc_i2c_req_t reqMaster = {
        .i2c = I2C_MASTER,
        .addr = 0x1D, // ADXL345 I2C Addr
        .tx_buf = my_adxl_tx,
        .rx_buf = my_adxl_rx,
        .tx_len = 1,
        .rx_len = 1,
        .callback = NULL,
    };
    I2C_FLAG=1;
    MXC_I2C_MasterTransaction(&reqMaster);

Outcome

The above shows a repeated start and a double read of the register despite only attempting to read once.

I am trying to use MAX32670 with a No-OS Project using ADXL345 that I built. I have actually seen this condition cause infinite loops where the I2C bus sends out clocks forever and waits for data that will not come.

This happens because the i2c_reva.c MasterTransaction function erroneously sets the Restart bit and attempt to read again, when no more data will come back from the peripheral. It looks like an attempt was made to address this in commit #1211 but it may have been put off until further notice.

[Brandon-Hurst]

May be related to #1211. It looks like the behavior I'm seeing is similar.

[dnguye14-adi]

Hi, could you please confirm about the i2c configurations?? With rx_len=1, I could not reproduce your signals, but, It is the same if I change the rx_len to 2.

[dnguye14-adi]

@Brandon-Hurst ,

i2c_reva.zip

Anyway, you can use my i2c_reva file to fix the issue. I added 2 while-loop to wait until the bus is not busy, line 905 and line [968 - 973]. They fixed the double-read issue as well as the issue of hang due to quick transaction.

Please double check from your side,

[Brandon-Hurst]

Hi @dnguye14-adi, thanks, I will re-check this in the next couple of days. Sorry I haven't been able to take another look at it until now.

[Brandon-Hurst]

Hello,

I went and confirmed this fixes the MasterTransaction function. The configurations from before were correct, e.g. I tested the old vs. new files and saw the same signals as before. The old driver always generates an erroneous double read (the rxLen parameter merely controls the # of bytes read in each of these reads).

Old i2c_reva.c; RxLen=1

Old i2c_reva.c; RxLen=2

New i2c_reva.c; RxLen=1

I have one change to make to the new proposed i2c_reva.c; the first while loop is unnecessary and adds additional time debugging when this function probably shouldn't be called when the I2C bus is busy in the first place.

In situations where two parallel tasks might try to access the bus at the same time, those accesses should be controlled with a mutex anyway. I would change this while loop to a simple if statement / return E_BUSY if the busy flag is set. This is also more consistent with how the other PeriphDrivers do it (SPI, UART, etc) and prevents locking up the device under error conditions.

if (i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {
    return E_BUSY;
}

I made this change, recompiled/flashed, and tested this:

For what it's worth, I also went and made sure the MasterTransactionAsync function work okay. Below is RxLen=1, MasterTransactionAsync:

[dnguye14-adi]

Hi,
I'm still wondering about replacing the while loop with checking the busy flag as this is in driver source. If use busy flag checking, then, developer still has to place a while loop in application source. In my opinion, we still keep the while loop in driver section.

Below is my testing with using the while loop: write 1 byte > read 1 byte > write 1 byte > read 1 byte > write 1 byte > read 1 byte > write 1 byte > read 1 byte:

[Brandon-Hurst]

Hi @dnguye14-adi, the point is not necessarily to not have a while loop. The point is that the possibility of an infinite while loop can crash the system. If you want to use a while loop in the critical path of a HAL function, you should add a timeout or some other exit condition.

It is a lot more painful for the user to have a broken system and spend hours debugging our code than it is for them to check the error flags we provide and/or provide their own while loop.

[dnguye14-adi]

Hi @Brandon-Hurst ,
I updated the PR with your solution, check and return BUSY if the Bus is not ready in the Transaction function.
Passed from my side.

[yoshihiko-niino]

@dnguye14-adi -san
Please tell me the details.
Please tell me the cause of the double read and the detailed countermeasures.
The same issue is occurring here, so I would like to know the root cause.
Thank you.