Skip to content

I2C Fix #748

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

Merged
merged 4 commits into from
Nov 5, 2019
Merged

I2C Fix #748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f56b7a6
Added the repeated-start feature in I2C driver (#590)
jmchiappa Sep 13, 2019
c46e7e3
[I2C] Fix error management in Read/Write
ABOSTM Oct 9, 2019
dd3050a
[I2C] Only one return per function/method if possible
fpistm Oct 15, 2019
431ba4b
[I2C] Review errors management
fpistm Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
[I2C] Review errors management 
Signed-off-by: Frederic Pillon <frederic.pillon@st.com>
  • Loading branch information
@fpistm
fpistm committed Oct 31, 2019
commit 431ba4bfdece4160738e2037ae758fdf60709c42
24 changes: 16 additions & 8 deletions cores/arduino/stm32/twi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,12 @@ i2c_status_e i2c_master_write(i2c_t *obj, uint8_t dev_address,
if ((delta > I2C_TIMEOUT_TICK)
|| ((err & HAL_I2C_ERROR_TIMEOUT) == HAL_I2C_ERROR_TIMEOUT)) {
ret = I2C_TIMEOUT;
} else if (err != HAL_I2C_ERROR_NONE) {
ret = I2C_ERROR;
} else {
if ((err & HAL_I2C_ERROR_AF) == HAL_I2C_ERROR_AF) {
ret = I2C_NACK_DATA;
} else if (err != HAL_I2C_ERROR_NONE) {
ret = I2C_ERROR;
}
}
}
}
Expand All @@ -784,7 +788,7 @@ i2c_status_e i2c_slave_write_IT(i2c_t *obj, uint8_t *data, uint16_t size)

// Protection to not override the TxBuffer
if (size > I2C_TXRX_BUFFER_SIZE) {
ret = I2C_ERROR;
ret = I2C_DATA_TOO_LONG;
} else {
// Check the communication status
for (i = 0; i < size; i++) {
Expand Down Expand Up @@ -832,8 +836,12 @@ i2c_status_e i2c_master_read(i2c_t *obj, uint8_t dev_address, uint8_t *data, uin
if ((delta > I2C_TIMEOUT_TICK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a new problem: When this timeout is triggered, I2C_OK is triggered. This happens because in the loop above, it checks for (delta < I2C_TIMEOUT_TICK), so it breaks out of the loop as soon as delta == I2C_TIMEOUT_TICK. Then, this if checks for (delta > I2C_TIMEOUT_TICK), which is not yet false. Since the hardware reports no errors either, I2C_OK is returned.

We noticed this when something confused the I2C hardware and prevented it from continuing somehow. We haven't figured out what the underlying cause is, but we did notice that we did not get an I2C error (just invalid data), which I think can be traced back to this problem.

The same applies to i2c_write_master above, I think.

Fixing this is, I think, a matter of replacing > with >= here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing: This > and < combination was already present in the old code, but due to the old structure there it would, I think, only be problematic when an ACK failure occured exactly a the timeout, which is of course extremely likely. These refactorings have caused this to happen on every timeout instead.

Fixing this is, I think, a matter of replacing > with >= here.

We just tried this, and it seems to work. @HendryKaak is considering to prepare a PR for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

|| ((err & HAL_I2C_ERROR_TIMEOUT) == HAL_I2C_ERROR_TIMEOUT)) {
ret = I2C_TIMEOUT;
} else if (err != HAL_I2C_ERROR_NONE) {
ret = I2C_ERROR;
} else {
if ((err & HAL_I2C_ERROR_AF) == HAL_I2C_ERROR_AF) {
ret = I2C_NACK_DATA;
} else if (err != HAL_I2C_ERROR_NONE) {
ret = I2C_ERROR;
}
}
}
return ret;
Expand All @@ -855,13 +863,13 @@ i2c_status_e i2c_IsDeviceReady(i2c_t *obj, uint8_t devAddr, uint32_t trials)
ret = I2C_OK;
break;
case HAL_TIMEOUT:
ret = I2C_TIMEOUT;
ret = (obj->handle.State != HAL_I2C_STATE_READY) ? I2C_TIMEOUT : I2C_NACK_ADDR;
break;
case HAL_BUSY:
ret = I2C_BUSY;
ret = (obj->handle.State != HAL_I2C_STATE_READY) ? I2C_BUSY : I2C_NACK_ADDR;
break;
default:
ret = I2C_TIMEOUT;
ret = (obj->handle.State != HAL_I2C_STATE_READY) ? I2C_ERROR : I2C_NACK_ADDR;
break;
}
return ret;
Expand Down
9 changes: 6 additions & 3 deletions cores/arduino/stm32/twi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,12 @@ struct i2c_s {
///@brief I2C state
typedef enum {
I2C_OK = 0,
I2C_TIMEOUT = 1,
I2C_ERROR = 2,
I2C_BUSY = 3
I2C_DATA_TOO_LONG = 1,
I2C_NACK_ADDR = 2,
I2C_NACK_DATA = 3,
I2C_ERROR = 4,
I2C_TIMEOUT = 5,
I2C_BUSY = 6
} i2c_status_e;

/* Exported functions ------------------------------------------------------- */
Expand Down
15 changes: 12 additions & 3 deletions libraries/Wire/src/Wire.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ void TwoWire::begin(int address, bool generalCall)

void TwoWire::end(void)
{
i2c_deinit(&_i2c);
free(txBuffer);
txBuffer = nullptr;
txBufferAllocated = 0;
free(rxBuffer);
rxBuffer = nullptr;
rxBufferAllocated = 0;
i2c_deinit(&_i2c);
}

void TwoWire::setClock(uint32_t frequency)
Expand Down Expand Up @@ -237,11 +237,20 @@ uint8_t TwoWire::endTransmission(uint8_t sendStop)
// transmit buffer (blocking)
switch (i2c_master_write(&_i2c, txAddress, txBuffer, txBufferLength)) {
case I2C_OK :
ret = 0;
ret = 0; // Success
break;
case I2C_TIMEOUT :
case I2C_DATA_TOO_LONG :
ret = 1;
break;
case I2C_NACK_ADDR:
ret = 2;
break;
case I2C_NACK_DATA:
ret = 3;
break;
case I2C_TIMEOUT:
case I2C_BUSY:
case I2C_ERROR:
default:
ret = 4;
break;
Expand Down