Skip to content

fix(PeriphDrivers): Issue #1293: MAX32670 I2C Driver causes a Double-Read #1359

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnguye14-adi
Copy link

@dnguye14-adi dnguye14-adi commented Mar 5, 2025

Description

In the Master Transaction function, add a checking to return BUSY if the I2C bus is not ready.

@Brandon-Hurst
Copy link
Contributor

Hello, as I mentioned in the issue #1293, I recommend changing the first while loop to an if statement. This will prevent the possibility of infinite loops if the I2C peripheral is busy / hanging. Returning an error indicating the I2C controller is busy is sufficient in this case, and a while loop should not be needed. I tested this as well and left screenshots in the issue linked.

I would propose changing this:

while(i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {}
// Wait until last transaction finished and bus ready

to this:

// If there is an active transaction, return a BUSY error
if (i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {
    return E_BUSY;
}

@Brandon-Hurst
Copy link
Contributor

Hello, is there any response to my request above or info on when this change could be merged?

@sihyung-maxim
Copy link
Contributor

Hello, as I mentioned in the issue #1293, I recommend changing the first while loop to an if statement. This will prevent the possibility of infinite loops if the I2C peripheral is busy / hanging. Returning an error indicating the I2C controller is busy is sufficient in this case, and a while loop should not be needed. I tested this as well and left screenshots in the issue linked.

I would propose changing this:

while(i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {}
// Wait until last transaction finished and bus ready

to this:

// If there is an active transaction, return a BUSY error
if (i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {
    return E_BUSY;
}

Sorry, I've been away from MSDK support for a while because of other pending tasks. I agree with this too; we should avoid an infinite loop without a timeout or any way to exit.

@dnguye14-adi dnguye14-adi force-pushed the fix/1293-max32670_i2c_driver-causes-double-read branch from e6554a7 to 15a1fbd Compare April 11, 2025 02:47
@AlbertFrancis07
Copy link

AlbertFrancis07 commented Apr 11, 2025

Hello, as I mentioned in the issue #1293, I recommend changing the first while loop to an if statement. This will prevent the possibility of infinite loops if the I2C peripheral is busy / hanging. Returning an error indicating the I2C controller is busy is sufficient in this case, and a while loop should not be needed. I tested this as well and left screenshots in the issue linked.

I would propose changing this:

while(i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {}
// Wait until last transaction finished and bus ready

to this:

// If there is an active transaction, return a BUSY error
if (i2c->status & MXC_F_I2C_REVA_STATUS_BUSY) {
    return E_BUSY;
}

I too faced the same Problem of I2C hanging in infinite loop due to sudden transaction , while interfacing MAX78000 with ADXL382 accelerometer and the above proposed solution of making I2C bus wait till end of transaction has helped me fix it.

@dnguye14-adi dnguye14-adi force-pushed the fix/1293-max32670_i2c_driver-causes-double-read branch from 15a1fbd to 3b22558 Compare April 12, 2025 02:33
@dnguye14-adi
Copy link
Author

@AlbertFrancis07, you could also try with the solution not using the while-loop.

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

Successfully merging this pull request may close these issues.

4 participants