raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.7k stars 918 forks source link

i2c_write_raw_blocking does not work #708

Open davidedelvento opened 2 years ago

davidedelvento commented 2 years ago

As extensively discussed at https://forums.raspberrypi.com/viewtopic.php?t=304074 and https://github.com/raspberrypi/pico-examples/issues/192 i2c_write_raw_blocking does not work.

Using just the SDK, the pico cannot operate as a slave (worker) sending bytes to the I2C bus: it can only read. In fact, the i2c_write_raw_blocking function does not appear to block as expected when there is no request from the master (controller). Moreover, even if there is the necessary request from a controller, which is blocking in a corresponding i2c_read_blocking (assuming it's another pico), no data is transmitted on the bus, the controller continues to stay blocked and the worker continues the operations after the i2c_write_raw_blocking.

Accessing the raw registers directly appears to work, as demonstrated by the examples mentioned in the forum post linked above, namely https://github.com/vmilea/pico_i2c_slave and particularly

https://github.com/vmilea/pico_i2c_slave/blob/master/i2c_slave/include/i2c_fifo.h#L29-L33 and https://github.com/vmilea/pico_i2c_slave/blob/master/i2c_slave/include/i2c_fifo.h#L43-L47

At the very least a comment should be added in the documentation about i2c_write_raw_blocking not working, to keep until it's fixed.

davidedelvento commented 2 years ago

After a few careful tests, I think I understand the root cause, which I suspect is in the hardware, rather than the SDK. If that the case, only the following is needed in the SDK:

What I found is that if the worker extremely carefully always avoids calling i2c_write_raw_blocking before the corresponding read is posted by the controller, everything works just fine. The obvious way to do so is with interrupts, which are triggered in the worker only after the controller has posted the read, but even just sleeping in the worker solves the problem (clearly just as a demonstration that it works, in most real-life application that's not possible or acceptable)

I am working on the simplest possible code needing the least possible pieces of hardware to demonstrate the problem and the solution, and I will post it here and in the pico-example repo.

fivdi commented 2 years ago

the _blocking suffix must be removed from the function name, because the call is non-blocking (yes, I've seen the source code where it appears to block, but that is never triggered, and looking at what that does, it goes straight into the raw register 0x74 hence my suspicion that the hardware is the problem)

I haven't actually used i2c_write_raw_blocking which is implemented here, but my understanding of the implementation is that it will block and wait if the FIFO is full. If the data that is being written fits in the FIFO, it will not block. If i2c_write_raw_blocking is called to write small blocks of data, it may never need to block as the FIFO may never be full. If I remember correctly, the FIFO can hold up to 16 bytes of data.

If my assumptions about the implementation here are correct and i2c_write_raw_blocking is being called to write small blocks of data, it will likely return before even a single bit of data has been sent over the wire.

Perhaps you are misunderstanding how i2c_write_raw_blocking functions? Or are expecting it to do something that it doesn't do?

davidedelvento commented 2 years ago

@fivdi thanks for your message. Yes, I had seen that source code, and that is why I am speculating it's a hardware problem. I even checked deep into the code to see if it uses the correct hardware register and it does. Well, I have no access to the hardware design to know if it's the correct address, but it is the one described in the datasheet, namely the IC_RXFLR register.

Perhaps I am misunderstanding how things are supposed to work, but I have (tried to) use i2c_write_raw_blocking quite a lot and its behavior is quirky and confusing to say the least. In fact, as the forum post conversation I linked above, many other people are equally unable to make it work.

Instead of just words, I wrote the simplest code that clearly demonstrate the issue. Please see it at https://github.com/davidedelvento/Rpi-pico-i2c-example

Now, it might be that everything is behaving as it is supposed to work, but if that's the case the _blocking suffix is incorrect, and instead should be replaced by something like _never_ever_call_me_before_the_read_has_been_posted (if you allow me the joke). And a sentence in the documentation stating that basically it must be used only in IRQ, unlike the corresponding i2c_read_raw_blocking which works just fine in regular code.

davidedelvento commented 2 years ago

Sadly, i2c_write_raw_blocking does not work with interrupts either. I put a stud example of such an approach commented out in the code, which you can try to uncomment and see the code hanging. Like the non-interrupt based approach, it's hanging in the controller/receiver side. The sender/worker continues happily to run, as one can see e.g. by putting a blinking LED or a printf in the interrupt call (never do that in production, but for a test should be ok)

The only feasible approach is to go really low level, as demonstrated here

Unless somebody can prove it otherwise with a working example, i2c_write_raw_blocking is utterly broken for slave/worker operations, and should be removed from the SDK.

JamesH65 commented 2 years ago

@kilograham In case you hadn't seen this.