potential issues in i2c_read_blocking_internal #336

New issue

Open

opened 2021-04-12 15:51:35 -05:00 by fivdi · 4 comments

fivdi commented

2021-04-12 15:51:35 -05:00

(Migrated from github.com)

There may be two issues in i2c_read_blocking_internal.

Issue 1
The first potential issue is these two lines of code:

            abort_reason = i2c->hw->tx_abrt_source;
            abort = (bool) i2c->hw->clr_tx_abrt;

The first line of code copies the value of the IC_TX_ABRT_SOURCE register to abort_reason. The second line clears the same register, that is, the IC_TX_ABRT_SOURCE register. If the hardware decides to set a bit in the IC_TX_ABRT_SOURCE register after the first line has been executed, but before the second line is executed, the reason for the aborted transfer will be lost. I don't have an example program that can be used to demonstrate this, it's an assumption.

Issue 2
The second potential issue is these two lines of code:

        while (!i2c_get_write_available(i2c))
            tight_loop_contents();

I'm not 100% sure but I don't think this loop serves any purpose. Can it be removed?

There may be two issues in `i2c_read_blocking_internal`. **Issue 1** The first potential issue is [these two lines of code](https://github.com/raspberrypi/pico-sdk/blob/1.1.2/src/rp2_common/hardware_i2c/i2c.c#L231-L232): ```c abort_reason = i2c->hw->tx_abrt_source; abort = (bool) i2c->hw->clr_tx_abrt; ``` The first line of code copies the value of the IC_TX_ABRT_SOURCE register to abort_reason. The second line clears the same register, that is, the IC_TX_ABRT_SOURCE register. If the hardware decides to set a bit in the IC_TX_ABRT_SOURCE register after the first line has been executed, but before the second line is executed, the reason for the aborted transfer will be lost. I don't have an example program that can be used to demonstrate this, it's an assumption. **Issue 2** The second potential issue is [these two lines of code](https://github.com/raspberrypi/pico-sdk/blob/1.1.2/src/rp2_common/hardware_i2c/i2c.c#L222-L223): ```c while (!i2c_get_write_available(i2c)) tight_loop_contents(); ``` I'm not 100% sure but I don't think this loop serves any purpose. Can it be removed?

lurch commented

2021-04-12 17:45:20 -05:00

(Migrated from github.com)

I'm not 100% sure but I don't think this loop serves any purpose.

Maybe it was supposed to be i2c_get_read_available? That's just a (very uninformed) guess though 😉 🤷

> I'm not 100% sure but I don't think this loop serves any purpose. _Maybe_ it was supposed to be `i2c_get_read_available`? That's just a (**very** uninformed) guess though :wink: :shrug:

fivdi commented

2021-04-13 02:12:12 -05:00

(Migrated from github.com)

I'm not 100% sure but I don't think this loop serves any purpose.

Maybe it was supposed to be i2c_get_read_available? That's just a (very uninformed) guess though 😉 🤷

I don't think so, i2c_get_read_available is called here.

> > I'm not 100% sure but I don't think this loop serves any purpose. > > _Maybe_ it was supposed to be `i2c_get_read_available`? That's just a (**very** uninformed) guess though 😉 🤷 I don't think so, `i2c_get_read_available` is called [here](https://github.com/raspberrypi/pico-sdk/blob/1.1.2/src/rp2_common/hardware_i2c/i2c.c#L237).

lurch commented

2021-04-13 02:19:39 -05:00

(Migrated from github.com)

I don't think so, i2c_get_read_available is called here.

On a side-note, I wonder if tight_loop_contents() ought to be being used inside that do ... while loop, if timeout_check is NULL? 🤷

> I don't think so, i2c_get_read_available is called here. On a side-note, I wonder if `tight_loop_contents()` ought to be being used inside that `do ... while` loop, if `timeout_check` is NULL? :shrug:

fivdi commented

2021-04-13 02:38:20 -05:00

(Migrated from github.com)

On a side-note, I wonder if tight_loop_contents() ought to be being used inside that do ... while loop, if timeout_check is NULL? 🤷

This is likely to be true. I think it should be used irrespective of of whether timeout_check is NULL, like here in i2c_write_blocking_internal.

> On a side-note, I wonder if `tight_loop_contents()` ought to be being used inside that `do ... while` loop, if `timeout_check` is NULL? 🤷 This is likely to be true. I think it should be used irrespective of of whether `timeout_check` is NULL, like [here in `i2c_write_blocking_internal`](https://github.com/raspberrypi/pico-sdk/blob/1.1.2/src/rp2_common/hardware_i2c/i2c.c#L140-L150).