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 2 commits
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
89 changes: 51 additions & 38 deletions cores/arduino/stm32/twi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,35 +731,42 @@ i2c_status_e i2c_master_write(i2c_t *obj, uint8_t dev_address,
uint8_t *data, uint16_t size)

{
i2c_status_e ret = I2C_ERROR;
i2c_status_e ret = I2C_OK;
uint32_t tickstart = HAL_GetTick();
uint32_t delta = 0;
uint32_t err = 0;

/* When size is 0, this is usually an I2C scan / ping to check if device is there and ready */
if (size == 0) {
return i2c_IsDeviceReady(obj, dev_address, 1);
}

do {
if (HAL_I2C_Master_Transmit_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
ret = I2C_OK;
// wait for transfer completion
while ((HAL_I2C_GetState(&(obj->handle)) != HAL_I2C_STATE_READY)
&& (ret == I2C_OK)) {
delta = (HAL_GetTick() - tickstart);
uint32_t err = HAL_I2C_GetError(&(obj->handle));
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;
}
#if defined(I2C_OTHER_FRAME)
uint32_t XferOptions = obj->handle.XferOptions; // save XferOptions value, because handle can be modified by HAL, which cause issue in case of NACK from slave
#endif

#if defined(I2C_OTHER_FRAME)
if (HAL_I2C_Master_Seq_Transmit_IT(&(obj->handle), dev_address, data, size, XferOptions) == HAL_OK) {
#else
if (HAL_I2C_Master_Transmit_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
#endif
// wait for transfer completion
while ((HAL_I2C_GetState(&(obj->handle)) != HAL_I2C_STATE_READY) && (delta < I2C_TIMEOUT_TICK)) {
delta = (HAL_GetTick() - tickstart);
if (HAL_I2C_GetError(&(obj->handle)) != HAL_I2C_ERROR_NONE) {
break;
}
}
/* When Acknowledge failure occurs (Slave don't acknowledge it's address)
Master restarts communication */
} while (((HAL_I2C_GetError(&(obj->handle)) & HAL_I2C_ERROR_AF) == HAL_I2C_ERROR_AF)
&& (delta < I2C_TIMEOUT_TICK));

err = HAL_I2C_GetError(&(obj->handle));
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;
}
}

return ret;
}

Expand Down Expand Up @@ -799,30 +806,36 @@ i2c_status_e i2c_slave_write_IT(i2c_t *obj, uint8_t *data, uint16_t size)
*/
i2c_status_e i2c_master_read(i2c_t *obj, uint8_t dev_address, uint8_t *data, uint16_t size)
{
i2c_status_e ret = I2C_ERROR;
i2c_status_e ret = I2C_OK;
uint32_t tickstart = HAL_GetTick();
uint32_t delta = 0;
uint32_t err = 0;

do {
if (HAL_I2C_Master_Receive_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
ret = I2C_OK;
// wait for transfer completion
while ((HAL_I2C_GetState(&(obj->handle)) != HAL_I2C_STATE_READY)
&& (ret == I2C_OK)) {
delta = (HAL_GetTick() - tickstart);
uint32_t err = HAL_I2C_GetError(&(obj->handle));
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;
}
#if defined(I2C_OTHER_FRAME)
uint32_t XferOptions = obj->handle.XferOptions; // save XferOptions value, because handle can be modified by HAL, which cause issue in case of NACK from slave
#endif

#if defined(I2C_OTHER_FRAME)
if (HAL_I2C_Master_Seq_Receive_IT(&(obj->handle), dev_address, data, size, XferOptions) == HAL_OK) {
#else
if (HAL_I2C_Master_Receive_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
#endif
// wait for transfer completion
while ((HAL_I2C_GetState(&(obj->handle)) != HAL_I2C_STATE_READY) && (delta < I2C_TIMEOUT_TICK)) {
delta = (HAL_GetTick() - tickstart);
if (HAL_I2C_GetError(&(obj->handle)) != HAL_I2C_ERROR_NONE) {
break;
}
}
/* When Acknowledge failure occurs (Slave don't acknowledge it's address)
Master restarts communication */
} while (((HAL_I2C_GetError(&(obj->handle)) & HAL_I2C_ERROR_AF) == HAL_I2C_ERROR_AF)
&& (delta < I2C_TIMEOUT_TICK));

err = HAL_I2C_GetError(&(obj->handle));
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;
}
}

return ret;
}
Expand Down
22 changes: 22 additions & 0 deletions libraries/Wire/src/Wire.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ void TwoWire::setClock(uint32_t frequency)

uint8_t TwoWire::requestFrom(uint8_t address, uint8_t quantity, uint32_t iaddress, uint8_t isize, uint8_t sendStop)
{
#if !defined(I2C_OTHER_FRAME)
UNUSED(sendStop);
#endif

if (_i2c.isMaster == 1) {
allocateRxBuffer(quantity);
// error if no memory block available to allocate the buffer
Expand Down Expand Up @@ -146,6 +149,15 @@ uint8_t TwoWire::requestFrom(uint8_t address, uint8_t quantity, uint32_t iaddres

// perform blocking read into buffer
uint8_t read = 0;

#if defined(I2C_OTHER_FRAME)
if (sendStop == 0) {
_i2c.handle.XferOptions = I2C_OTHER_FRAME ;
} else {
_i2c.handle.XferOptions = I2C_OTHER_AND_LAST_FRAME;
}
#endif

if (I2C_OK == i2c_master_read(&_i2c, address << 1, rxBuffer, quantity)) {
read = quantity;
}
Expand Down Expand Up @@ -211,8 +223,18 @@ void TwoWire::beginTransmission(int address)
//
uint8_t TwoWire::endTransmission(uint8_t sendStop)
{
#if !defined(I2C_OTHER_FRAME)
UNUSED(sendStop);
#endif
int8_t ret = 4;
// check transfer options and store it in the I2C handle
#if defined(I2C_OTHER_FRAME)
if (sendStop == 0) {
_i2c.handle.XferOptions = I2C_OTHER_FRAME ;
} else {
_i2c.handle.XferOptions = I2C_OTHER_AND_LAST_FRAME;
}
#endif

if (_i2c.isMaster == 1) {
// transmit buffer (blocking)
Expand Down