RP2040 I2C Refactor

[haydenridd]

I2C Refactor

There's enough changes here that I felt it deserved a mini writeup. There was a couple of opinionated decisions I made as well to the API that I wanted to call out and get others opinions on.

High Level

• Various performance improvements to I2C reads/writes related to how/when polling occurs
• Simplified API so that all reading/writing happens in 3 functions
• Tweaked how I2C is "initialized" and "deinitialized"
• Corrected how baud rate is translated to register settings for I2C timing
• Moved most of the error handling responsibility onto the caller of the I2C API, for example code no longer calls panic(), but rather bubbles up an error to the caller for them to make a decision on
• Added "const correctness" to API
• Added comments being explicit about what features of the hardware this API doesn't support

Verification Method

My "self test" firmware for the RP2040 is still very much a WIP, but I used it to verify all the changes I made to the I2C driver. Check that code out here to see what exactly I check, and a good example of using the new API:
https://github.com/haydenridd/microzig/tree/rp2040-selftest/test_apps/rp2040

Performance Improvements

Two main points here:

• The current code followed a pattern from the Pico SDK of polling on IC_RAW_INTR_STAT.TX_EMPTY to check when TX was finished. This is fine, the issue was it did it after every write to the TX FIFO, leading to occasional small gaps between write transactions at higher I2C frequencies. There's no need to poll on this after every byte desired to be transmitted is written to IC_DATA_CMD. Polling on IC_TXFLR during the write loop prevents any accidentally overflowing the TX FIFO, and is faster than waiting for the actual transaction to finish for the previous byte written to the FIFO.
• The function write_then_read_blocking() was introduced to give a performant method for handling the common I2C use case of writing one or more bytes to a device representing an internal register address, then immediately reading one or more bytes representing the data held at that internal address. This allows that to be accomplished in a single transaction (a repeated start occurs between the write bytes and read bytes) where previously this behavior required two transactions with a full stop/start + time gap in between.

The following oscilloscope shots show the performance gains for each:

Old successive writes at 400kHz:

New successive writes at 400kHz:

Old write + read at 100kHz:

New write + read at 100kHz:

Initialization

I modified the structure of I2C slightly from an enum to a struct. This is so it can hold SDA + SCL gpio.Pin instances, as well as a pointer to the raw registers it controls. This enables there to be an init() function bound as a method to this struct. Given that this is indeed initializing resources, it seemed appropriate to follow the Zig convention of init()/deinit() for structs that manage resources. Note I've also added a deinit() function that does something equally un-surprising.

Something I don't love, but don't see another way around, is the addition of an initialized: bool field in the I2C struct. There are no constructors in Zig, which is a good thing. But, this means it's on the user to handle initializing and deinitializing resources explicitly. This field blocks the user from calling the methods that depend on I2C being initialized before use. For instance, if not for this, when you call write_blocking without a timeout (null), you end up in an infinite loop on the polling call to while (i2c.tx_fifo_available_spaces() == 0). This is completely reasonable, as when the peripheral isn't enabled/configured, that register will constantly read 0. Personal opinion is I would rather have this small amount of boilerplate and shield a HAL user from footguns that accompany using a peripheral API without configuring the peripheral appropriately. I've seen very similar boilerplate in vendor HALs.

Finally, I felt from_instance_number() conforms more to a slightly lesser used but still present Zig convention of naming a constructor that creates a struct from some other Something, fromSomething(something: Something) StructType. The original constructor function num(), I felt, was a little bit ambiguous what exactly it did.

Baud Rate Translation

This was by far the most confusing section of the RP2040 datasheet I've had to figure out thus far. So confusing, in fact, it appears from comments the Rpi Pico's own SDK gave up on actually configuring this 100% correctly. My changes are subtle but:

• I now take into account the extra clocks added by the I2C peripheral to IC_FS_SCL_LCNT/IC_FS_SCL_HCNT (See section 4.3.14.1.)
• I give more informative errors if baud rate translation fails
• Some rudimentary tests that show this is in line with the datasheet

Const Correctness

This is up for debate, but I'm of the opinion any HAL API method that changes (writes/reads with side effects) a device register should have the following "non-const" method signature:

pub fn nonConstMethod(self: *HalApi, ...) ...

Of course, this requires HAL designers to pay careful attention to what does and does not mutate device state. There are often registers that have "clear on read" behavior, that despite being a "read" should indeed be considered mutating behavior. However, register reads that don't mutate state (for instance some device ID, or current peripheral configuration), would be allowed under a const method.

I found the way the existing API was set up confusing because you could create a const instance of I2C, and yet have full access to all sorts of functions that configure the peripheral, write bytes, generally mutate device state. Sure, const was valid because the struct itself wasn't mutating, but wouldn't it be nice if a user could write a function like this:

fn someNonMutatingFunction(hal_api: HalApi) void {
    hal_api.nonConstmethod();
}

And have it fail to compile because they accidentally call a function that mutates device state when their input is const?

Misc Changes

• Changed some confusing configuration behavior in gpio.zig + added support for slew-rate/schmitt trigger to configure I2C pins per datasheet recommendations
• Added ability to disable repeated start behavior for legacy devices that may not support it

[haydenridd]

After some feedback:

• Removed "const correctness", as after further inspection I agree it would lead to more confusion than it would help
• Tweaked namespace of from_instance_number()

[haydenridd]

@ikskuh good to merge?