RFC: asynchronous i2c and spi API

[nordic-krch]

Introduction

Currently, i2c/spi API is synchronous. As a consequence it can only be called from thread context. It also applies to sensors which are built on top of those APIs. In many scenarios that leads to increased RAM usage since additional thread is needed or can be even error prone. It is related to #21515.

Problem description

Lets consider following cases:

• reacting to gpio interrupt from sensor (e.g. pin data ready or limit reach) by reading data from sensor
• periodically reading sensor data, triggering more complex processing when certain condition is met, e.g. limit reached, number of measurements reached.

In both cases we would like to read sensor data while being in the interrupt context. Currently, there are 2 options:

• dedicated thread which is woken up from interrupt context
• deferring to work queue and executing from work queue context

While the former option increases RAM usage as additional thread must be created (most sensors has option to create thread with 1024 bytes as default), the latter is even worse because it is easier and tempting but it can block work queue for unknown period of time (e.g. currently not implemented adt7420 single shot measurement takes 240 ms). For memory constraint devices (which usually applies to all high volume products) 1k of RAM needed to handle simple sensor is unrealistic.

Proposed change

Adding asynchronous i2c/spi APIs. This is not straightforward as in case of other drivers since there can be multiple bus users and returning error on busy does not help. Bus (i2c or spi) control would be divided into 3 layers:

• low level driver - simple, single user, asynchronous API which can perform single operation at a time and returns busy error while in progress.
• transaction manager - asynchronous, thread safe API, API accepts a transaction. Transaction contains n transfers, bus configuration, callback and user data. Transaction can be put into a list when bus is busy. It's using low level driver to perform all operations. Callback is called when all transfers in the transaction are completed.
• synchronous API (current one) - creates semaphore and transaction on the stack, Behavior is unchanged.

With those 3 layers current API can coexist with new transaction manager. Thanks to that support to transaction manager can be gradually added to sensors. Vendor drivers are simpler since no access control is needed or handling multiple messages, only low level transfers are handled.

Note, transaction manager is inspired by nRF5 SDK where it has been successfully used for SPI and I2C.

Proposed change (Detailed)

Low level driver API draft

int i2c_ll_set_callback(i2c_ll_callback_t callback, void *user_data);
int i2c_ll_configure(struct device *dev, u32_t dev_config);
int i2c_ll_transfer(struct device *dev, struct i2c_msg *msgs, u16_t addr);

Transaction manager API draft

struct i2c_mngr_transaction {
   sys_snode_t node;
    i2c_mngr_callback_t callback;
    void *user_data;
    u16_t address;
    u8_t num_msgs;
    struct i2c_msg *msgs;
}
int i2c_mngr_schedule(struct device *dev, struct i2c_mngr_transaction *transaction);
/* configure needed to satisfy sync API */
int i2c_mngr_configure(struct device *dev, u32_t dev_config);

I2C synchronous API as a wrapper:

static inline z_i2c_transaction_callback(..., void *user_data)
{
    k_sem_give((struct k_sem *sem)user_data);
}
static inline int i2c_transfer(struct device *dev, struct i2c_msg *msgs, u8_t num_msgs,
			   u16_t addr)
{
    struct k_sem sem;
    struct i2c_mngr_transaction transaction = {
        .callback = z_i2c_transaction_callback,
        .user_data = &sem,
        .address = addr,
        .num_msgs = num_msgs,
        .msgs = msgs
    }
    int err = i2c_mngr_schedule(dev->i2c_mngr_dev, &transaction);
    if (err >= 0) {
        err = k_sem_take(&sem, K_FOREVER);
    }
    return err;
}

Concerns and Unresolved Questions

Main concern is of course execution but since it won't change i2c synchronous API, it can be added gradually without braking current state:

• adding first implementation of low level driver
• adding transaction manager,
• adding configuration to i2c which allows usage of transaction manager
• extending sensor API to support asynchronous operations
• first sensor which support transaction manager

[nordic-krch]

@pabigot @MaureenHelm @anangl can you take a look?

[nordic-krch]

I started to experiment (#21641) and added following drafts:

• i2c_mngr api and implementation
• i2c_ll api and implementation for nordic platform.
• extended i2c api to be able to use i2c_mngr if present and provide sync api on top of that
• extended sensor API with async call
• modified hts221 sensor to support reading temperature and humidity asynchronously
• modified samples/sensor/hts221 to work in 2 modes: periodic sensor read from thread or asynchronously from k_timer (interrupt context). I've removed all printing to only get sensor related processing

It has proven that synchronous and asynchronous API's can coexist (synchronous sensor init, asynchronous sensor read).

Having all that i was able to measure CPU load when performing this standard scenario (reading sensor periodically). For CPU load measurements I've used hardware events available on nRF devices so there is no measurement overhead.

Results:

• asynchronous - CPU active for 189 us for each measurement
• synchronous (legacy) - CPU active for 398 us for each measurement

I did not dig deeper into those results so i don't know if they can be optimized (only disabled logs and prints) but if number stays then it's a serious argument for asynchronous API's.

[sslupsky]

I have encountered significant issues with the i2c implementation for sam0 (Cortex M0). Interrupt latency, issues with the rtc and how/when zephyr sets a timeout compound so that the time taken to read an SHT31 temp/humidity sensor can be on the order of milliseconds. IMO, this is an extraordinarily long period of time.

One thing for sure, i suggest the new API should treat an i2c message atomically. That is, the kernel cannot "break it up" as can occur using the existing api. This results in excessive power consumption (pulls ups are drawing current the entire time the transaction holds the bus) and excessive time.

[nordic-krch]

@sslupsky with this approach things would happen atomically since next i2c transfer would be issued from the completion interrupt of the previous one so transaction (chain of transfers) would happen without kernel control.

[pabigot]

I'm not sure exactly what @sslupsky was proposing, but a problem with the existing API is that it uses a scatter/gather protocol where the message can be fragmented. This is not supported on all hardware (Nordic TWIM specifically); where it has been supported the effect of specific flags is not consistent between implementations.

There's some convenience in allowing data for a single write or read transaction to be sourced from or written to non-contiguous blocks of memory, but the vast majority of I2C needs can be accomplished with something that has a single function like i2c_write_read extended with the capability that the write or read phases may be optimized out if empty.

However, I have also never used I2C serial memories, such as the FRAM ones that were the blocker for eliminating the various I2C API functions that are documented as not universally supportable. For those, a scatter/gather API may be very important.

Regarding the delays in reading SHT21: high-level I2C transactions often can't be atomic (in a way that eliminates the impact of other processor activity) because the write (of the register address) and read (of the data) transactions require reconfiguration of the peripheral for all hardware I've used. sam0 looks like it's probably the same.

[nordic-krch]

what if i2c low level messages would be defined as something like:

/* flags */
#define I2C_LL_TX
#define I2C_LL_TX_R_TX /* tx repeated start tx */
#define I2C_LL_TX_TX /* scatter gatter tx */
#define I2C_LL_RX
#define I2C_LL_TX_RX;

struct i2c_ll_msg {
  i2c_buf_t primary_buffer; /* buf + len */
  i2c_buf_t secondary_buffer;  /* buf + len */
  u8_t flags;
}

[nordic-krch]

I was also thinking that i2c_mngr could be turned into bus_mngr which would support spi or i2c (it's just a matter of different function which schedules the transfer and understands message). There could be two sets of macros for creating messages so it would be possible to compile given sensor for spi or i2c bus.

[sslupsky]

@nordic-krch Please see issue #21549 . The sam0 driver sends the next byte at the completion interrupt. However, the kernel appears to gets in the way between interrupts and it can insert significant delays between bytes. The problem is exacerbated when you sleep.

[anangl]

Overall, I like this idea.
After taking a look at #21641, I think we don't need to introduce the LL layer to achieve the effect. Instead, we could add to i2c_driver_api a "simple transfer" function that would transfer a single i2c_msg only, and add a way of configuring the callback for it (with additional parameters to this function or via another function added to i2c_driver_api). This new function would be actually (at least on Nordic platform) an extract from the inner loop of the existing i2c_api_full_io_t transfer function, so some code could be made common for both of them.

[tbursztyka]

spi API is synchronous

There is an async SPI API call, but still using caller's thread context. There is no transaction manager in between obviously. All other calls are synchronously pending until they get the lock as for the sync calls.

About transaction manager, how are you going to deal with memory management?
I guess the mgr's thread is going to be a kernel thread. So will you count on current assumption that caller's thread is not supposed to mangle with the data it provided to the transaction, or will you copy the transaction info for the mgr thread?

[sslupsky]

@nordic-krch The problem with the existing driver is it relies on interrupts to fire between sequential parts of an i2c transaction. So, for instance, if I am reading several bytes, each byte transfer generates an interrupt. The Zephyr kernel intercepts these interrupts and if you are using PM, the latency can be enormous. I've seen 100's of ms. So, a simple transfer that should take 50 or 100ms can take 1000ms or more. So, what I intended with the "atomic" term is that an i2c transaction should be considered critical. So, if I read 6 bytes, then that entire transaction is handled atomically and not broken up in such a way as the kernel can preempt the transaction.

[sslupsky]

@nordic-krch There is another sort of related issue. I have experienced several complete lockups of the kernel on boot due to an i2c sensor going rogue and getting into a state where a sensor hangs the bus which then hangs the kernel.

It got me thinking that there should be an API to cause the sensors to reset at boot (init) time, maybe something that uses the i2c general call software reset? I need to think about this some more. If a slave sensor goes rogue, will it still respond to the general call? I do not really know the answer to that at the moment.

Note, this comment may apply to sensors with an SPI interface as well but I haven't given those as much thought yet.

[tbursztyka]

The problem with the existing driver is it relies on interrupts to fire between sequential parts of an i2c transaction. So, for instance, if I am reading several bytes, each byte transfer generates an interrupt.

If the driver is not using DMA, yes that will be the nominal behavior. Some device - not using DMA - have an internal buffer to be filled up, so it limits such interrupts but it is very often the device relies on the host cpu to feed it byte by byte due to hw limitations. I don't see this as an actual issue unless the device can support DMA and the feature is just not enabled in the driver. Either way this is not an API issue.

There is another sort of related issue. I have experienced several complete lockups of the kernel on boot due to an i2c sensor going rogue and getting into a state where a sensor hangs the bus which then hangs the kernel.

We indeed lack of a generic timeout on bus API calls. SPI API has the same issue as many other APIs in fact (it could avoid API updates: timeout does not have to be specified by the caller, it could be asserted from the actual transaction length and some other parameters). That won't be possible to get it fixed in the proposed async API.

[sslupsky]

Either way this is not an API issue.

I suggest there could be a switch for "atomic" transfers that could be passed along to the i2c driver. Such a switch would be useful in the case where there is a very long transfer where you do want to yield to the kernel between parts of the transaction.

[pabigot]

API 2020-07-07: Moved from triage to in-progress. Return to triage if further discussion is warranted, or close if this is duplicative of in-progress device rework viz. #22941

[nordic-krch]

#21578 shown another use case where calling i2c from interrupt would be useful: gpio blinking. SoC gpios can be controlled from any context (simple write to registers) but gpio expanders using i2c which can only be controlled from a thread context. Using k_timer to drive blinking gives less latency but will not work with gpio expanders. Something like delay work is more generic but will probably add jitter and latency and it might be especially problematic if brightness would be supported (pwm).

[erikarn]

For hardware that has Interrupts + FIFOs for TX/RX phase, it'd be nice to have asynchronous i2c stuff to manage multiple busses.

Right now everything seems to stop in the thread whilst the i2c work is going on, where I'd instead like to be doing other async work like USB and servicing/initiating other i2c transactions.

[teburd]

I think the general idea here is good, I would add that rather than a transaction manager against a single device, there be sort of two layers to this to facilitate more complex DMA controllers.

A per device API that accepts a request and a response object associated with it. Each device may choose to use dma or not, with a priority and dma arb setup with it.