Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make (synchronous) I2C work again #2972

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Make (synchronous) I2C work again #2972

merged 1 commit into from
Oct 12, 2016

Conversation

betzw
Copy link
Contributor

@betzw betzw commented Oct 10, 2016

Description

  • Remove waiting for BTF flag in i2c_stop()
  • Delay clock enabling until pins are configured
  • Move initialization of handle->Instance to function i2c_reset()

Status

READY

@sg-
Copy link
Contributor

sg- commented Oct 10, 2016

@betzw Looks like there are 3 changes in one commit. These would be better as 3 separate commits. Can you update? Also if there are test results, can you upload?

@betzw
Copy link
Contributor Author

betzw commented Oct 10, 2016 •
edited
Loading

@sg-
Sorry Sam, I do not think that this is worthwhile as we are dealing with just one file and a change of less than 30 lines. Furthermore, we are dealing with a severe regression here, which requires to be merged as soon as possible!

@bcostm & @LMESTM
Can you pls. perform the neccessary tests and upload the results?

cc @nikapov-ST @siorpaes @screamerbg

@jeromecoutant
Copy link
Collaborator

Hi

+---------------+---------------------+-----------+----------+-------------------------+--------------------+---------------+-------+
| Result | Target | Toolchain | Test ID | Test Description | Elapsed Time (sec) | Timeout (sec) | Loops |
+---------------+---------------------+-----------+----------+-------------------------+--------------------+---------------+-------+
| OK | NUCLEO_F429ZI[F1CD] | uARM | MBED_A20 | I2C master/slave test | 0.26 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | uARM | MBED_A29 | i2c_master_slave_asynch | 0.53 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | ARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | ARM | MBED_A29 | i2c_master_slave_asynch | 0.48 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0.48 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | IAR | MBED_A20 | I2C master/slave test | 0.26 | 20 | 1/1 |
| OK | NUCLEO_F429ZI[F1CD] | IAR | MBED_A29 | i2c_master_slave_asynch | 0.53 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | uARM | MBED_A20 | I2C master/slave test | 0.26 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | uARM | MBED_A29 | i2c_master_slave_asynch | 0.55 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | ARM | MBED_A20 | I2C master/slave test | 0.26 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | ARM | MBED_A29 | i2c_master_slave_asynch | 0.51 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0.5 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | IAR | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F446ZE[F130] | IAR | MBED_A29 | i2c_master_slave_asynch | 0.5 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | uARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | uARM | MBED_A29 | i2c_master_slave_asynch | 0.53 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | ARM | MBED_A29 | i2c_master_slave_asynch | 0.51 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0.48 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | IAR | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F411RE[F26A] | IAR | MBED_A29 | i2c_master_slave_asynch | 0.48 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | uARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | uARM | MBED_A29 | i2c_master_slave_asynch | 0.48 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | ARM | MBED_A20 | I2C master/slave test | 0.26 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | ARM | MBED_A29 | i2c_master_slave_asynch | 0.5 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0.52 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | IAR | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F446RE[F048] | IAR | MBED_A29 | i2c_master_slave_asynch | 0.5 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | uARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | uARM | MBED_A29 | i2c_master_slave_asynch | 0.53 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | ARM | MBED_A29 | i2c_master_slave_asynch | 0.51 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0.51 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | IAR | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| OK | NUCLEO_F401RE[F17C] | IAR | MBED_A29 | i2c_master_slave_asynch | 0.5 | 20 | 1/1 |
| OK | NUCLEO_F410RB[F619] | uARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| NOT_SUPPORTED | NUCLEO_F410RB | uARM | MBED_A29 | i2c_master_slave_asynch | 0 | 0 | - |
| OK | NUCLEO_F410RB[F619] | ARM | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| NOT_SUPPORTED | NUCLEO_F410RB | ARM | MBED_A29 | i2c_master_slave_asynch | 0 | 0 | - |
| OK | NUCLEO_F410RB[F619] | GCC_ARM | MBED_A20 | I2C master/slave test | 0.27 | 20 | 1/1 |
| NOT_SUPPORTED | NUCLEO_F410RB | GCC_ARM | MBED_A29 | i2c_master_slave_asynch | 0 | 0 | - |
| OK | NUCLEO_F410RB[F619] | IAR | MBED_A20 | I2C master/slave test | 0.33 | 20 | 1/1 |
| NOT_SUPPORTED | NUCLEO_F410RB | IAR | MBED_A29 | i2c_master_slave_asynch | 0 | 0 | - |
+---------------+---------------------+-----------+----------+-------------------------+--------------------+---------------+-------+

@betzw
Copy link
Contributor Author

betzw commented Oct 11, 2016

On my side I have setup the Sensors_Reader sample application for the X-NUCLEO-IKS01A1 expansion board together with the latest version of mbed-dev.
With the original version of file targets/hal/TARGET_STM/TARGET_STM32F4/i2c_api.c the sample application reported just:

Failed to init X_NUCLEO_IKS01A1 expansion board!

while with the version of the file as in this PR the application performs as expected:

--- Starting new run ---
Humidity  | Temperature Sensor ID = HTS221  (0xbc | 0xbc)
Gyroscope | Motion Sensor ID      = LSM6DS3 (0x69 | 0x69)
===
I2C [errors]: 0x00    X         Y         Z
MAG [mgauss]:       135      -228       470
ACC [mg]:            17      -128       997
GYR [mdps]:        2870     -4620     -3010
---
TEMP | HUMIDITY: 25.02°C | 56.34%
TEMP | PRESSURE: 72.44°F | 999.03mbar
===
I2C [errors]: 0x00    X         Y         Z
MAG [mgauss]:       141      -224       490
ACC [mg]:            13      -130      1002
GYR [mdps]:        2870     -4620     -3010
---
TEMP | HUMIDITY: 25.04°C | 55.70%
TEMP | PRESSURE: 72.48°F | 999.06mbar

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 11, 2016 •
edited
Loading

Sorry Sam, I do not think that this is worthwhile as we are dealing with just one file and a change of less than 30 lines. Furthermore, we are dealing with a severe regression here, which requires to be merged as soon as possible!

It's not about what file and how many lines but a logical set of changes, that are easy to follow, and in the worst case to revert and should provide a description. These could be split, let's just focus now on the commit message: it would be helpful for anyone who comes back to this commit if the commit message describes what changes are included and how they fix a problem (in this case problems). I would propose:

STM32F4: I2C bugfix clock enable

Remove waiting for BTF flag in i2c_stop()
Delay clock enabling until pins are configured
Move initialization of handle->Instance to function i2c_reset()

Each of the list should contain - why, often also what does it fix, a problem of clocks, misconfiguration, wrong data, or anything else? For instance, for the first in the list above - remove waiting for BTF flag - It is removed because .... .

If you can amend you commit, that would be nice

Thanks for fixing i2c and providing test results

@betzw
[STM32F4]: I2C bugfix 'i2c_stop()' timeout
d258c14 
- Remove waiting for 'BTF' flag in 'i2c_stop()':
  When 'i2c_stop()' is called from 'i2c_read()' or 'i2c_write()' flag 'BTF'
  has already been cleared (indirectly) by the calling functions and therefore
  'i2c_stop()' would mistakenly always run into a timeout.
- Delay clock enabling until pins are configured:
  Enabling the I2C bus clock before configuring its pins might in rare
  cases lead to HW faults on the bus.
- Move initialization of 'handle->Instance' to function 'i2c_reset()':
  As 'i2c_reset()' uses '__HAL_I2C_GET_FLAG(handle, I2C_FLAG_BUSY)' field
  'handle->Instance' must have been initialized before doing so. Therefore,
  this operation has been anticipated by moving it from function
  'i2c_frequency()' to function 'i2c_reset()'.
@betzw
Copy link
Contributor Author

betzw commented Oct 12, 2016

Commit updated!

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 12, 2016

Commit updated!

+1, thanks!

LGTM

@sg- sg- merged commit 217a8fb into ARMmbed:master Oct 12, 2016
@betzw betzw deleted the betzw_i2c_wb branch October 13, 2016 06:54
@betzw betzw mentioned this pull request Oct 14, 2016
Closed
This was referenced Oct 14, 2016
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
release-version: 5.2.0-rc3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@betzw @sg- @jeromecoutant @0xc0170