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

STM32: Correct I2C master error handling #4012

Merged
merged 1 commit into from Mar 29, 2017

Conversation

Projects
None yet
6 participants
@monkiineko
Contributor

monkiineko commented Mar 23, 2017

Description

If I2C slave support is included, then the I2C error handler would always reset the I2C address, resulting in incorrectly changing the I2C state to listen for a controller configured as I2C master. This change conditionalizes the address setting to only occur if the controller was in slave mode when the error occurred.

Status

READY

Migrations

NO

Related PRs

None

Todos

None

Deploy notes

None

Steps to test or reproduce

Create a build for an STM32 based board with DEVICE_I2CSLAVE and DEVICE_I2C_ASYNCH enabled (current default). Create an I2C master and issue a .transfer() call to a non-existent I2C device address, so that the transfer will be NACKed. HAL_I2C_ErrorCallback() will be called on the NACK and call i2c_slave_address(), resulting in the state being set to LISTEN. Subsequent I2C transfer attempts to any address will then immediately fail because i2c_active() returns 1 (true) for any state other than READY.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 23, 2017

@LMESTM

LMESTM approved these changes Mar 24, 2017

thank you for the fix !!

@LMESTM

LMESTM approved these changes Mar 24, 2017

ok with the change, just a formatting update needed

targets/TARGET_STM/i2c_api.c Outdated
@@ -904,7 +904,8 @@ void HAL_I2C_ErrorCallback(I2C_HandleTypeDef *hi2c){
#if DEVICE_I2CSLAVE
/* restore slave address */
i2c_slave_address(obj, 0, address, 0);
if(address != 0)
i2c_slave_address(obj, 0, address, 0);

This comment has been minimized.

@LMESTM

LMESTM Mar 24, 2017

Contributor

actually, one small remark, code may need small formatting update : space after if and use of braces
if (address != 0) {
i2c_slave_address(obj, 0, address, 0);
}
Note that the same applies to other places of the code that I wrote before but our maintainer Martin usually asks for such changes :-)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 24, 2017

STM32: Correct I2C master error handling
If I2C slave support is included, then the I2C error handler would
always reset the I2C address, resulting in incorrectly changing the
I2C state to listen for a controller configured as I2C master.  This
change conditionalizes the address setting to only occur if the
controller was in slave mode when the error occurred.

@monkiineko monkiineko force-pushed the monkiineko:master branch to 2eb4048 Mar 24, 2017

@monkiineko

This comment has been minimized.

Contributor

monkiineko commented Mar 24, 2017

@LMESTM Thanks for the review. I have updated the formatting as you request (my preferred formatting, actually ; I had copied the format from the slave "if" a few lines above, but I see other "if"s in the file using the improved formatting). I also noticed that i2c_init() would reset the slave field back to 0, so I added a line just before the slave address restore to set slave field to 1, as it should be.

@monkiineko

This comment has been minimized.

Contributor

monkiineko commented Mar 24, 2017

@0xc0170 Yes, I have previously signed the CLA using my mbed.org account, under the name bscott:
https://developer.mbed.org/users/bscott/

@LMESTM

LMESTM approved these changes Mar 24, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Mar 24, 2017

@monkiineko good catch - thanks for joining your efforts to ours !

@0xc0170 0xc0170 removed the needs: review label Mar 24, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 24, 2017

/morph test

@0xc0170 0xc0170 added the needs: CI label Mar 24, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 24, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1737

Test failed!

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 28, 2017

Timing test failure due to CI load, restarting...

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1763

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 29, 2017

@sg- sg- merged commit d467f7d into ARMmbed:master Mar 29, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment