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

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
77 changes: 37 additions & 40 deletions cores/arduino/stm32/twi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,9 +731,10 @@ 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) {
Expand All @@ -744,30 +745,28 @@ i2c_status_e i2c_master_write(i2c_t *obj, uint8_t dev_address,
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

do {
#if defined(I2C_OTHER_FRAME)
if (HAL_I2C_Master_Seq_Transmit_IT(&(obj->handle), dev_address, data, size, XferOptions) == HAL_OK) {
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) {
if (HAL_I2C_Master_Transmit_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
#endif
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;
}
// 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 @@ -807,38 +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;

#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

do {
#if defined(I2C_OTHER_FRAME)
if (HAL_I2C_Master_Seq_Receive_IT(&(obj->handle), dev_address, data, size, XferOptions) == HAL_OK) {
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) {
if (HAL_I2C_Master_Receive_IT(&(obj->handle), dev_address, data, size) == HAL_OK) {
#endif
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;
}
// 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