rp2040: Transaction with a NDIR CO2 sensor completely disables I2C functionality [TinyGo SDK]

raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License

3.74k stars 927 forks source link

rp2040: Transaction with a NDIR CO2 sensor completely disables I2C functionality [TinyGo SDK] #1349

Open soypat opened 1 year ago

soypat commented 1 year ago

https://github.com/tinygo-org/tinygo/issues/3671

This issue was created in the TinyGo repository but it may apply to the pico-sdk or the RP2040's hardware so I'm creating a linked issue here. The reason I'm placing a linked issue here is for the following two reasons:

The I2C implementation in TinyGo is a line-by line port of the pico-sdk repository which means the pico-sdk may present the same issue.
This seems like a important problem: a crafted packet has the ability to interfere with the RP2040's communication denying transactions with other devices on the bus.

I'm not sure if this is the correct place to make note of this, let me know if this goes against the rules of this repo and I'll gladly submit the issue where appropiate.

soypat commented 1 year ago

@kilograham I've fixed the issue in the TinyGo SDK. The relevant lines are here.

Some context: The tx function in TinyGo SDK first writes the write buffer and after that reads into the read buffer, so it incorporates both read/write functionality into one function. The fix is an abort check after the write buffer has been written and the stop condition has been waited for. If the abort is not cleared then future calls to i2c disable will fail, rendering the I2C peripheral useless until it is reset or the abort condition is cleared

fchiesad commented 9 months ago

Hi @kilograham, I think I am having the same I2C issue.

I can see in the Saleae the signal matched to the byte address of the I2C data write transfer (0x10). The problem is that I can not later see the signal corresponding to the actual data I am trying to send out from the rp2040. Could you help me with this please?

RP2040_I2C

peterharperuk commented 9 months ago

Have you tried clearing the abort reason as suggested, after the write. A bit like this?

        abort_reason = i2c->hw->tx_abrt_source;
        if (abort_reason) {
            // Note clearing the abort flag also clears the reason, and
            // this instance of flag is clear-on-read! Note also the
            // IC_CLR_TX_ABRT register always reads as 0.
            i2c->hw->clr_tx_abrt;
        }

peterharperuk commented 5 months ago

Does this need this particular sensor to reproduce this problem?

soypat commented 5 months ago

I've seen this error happen with the NDIR and some other sensor, which I can't recall currently.