common-io: Support I2C transactions

[osloapps-max]

Is your feature request related to a problem? Please describe.
Current I2C API support only read/write APIs and depend on the driver supporting the "eI2CSendNoStopFlag" mode to implement back-to-back write-reads inside the same I2C transaction (start to stop).

As "eI2CSendNoStopFlag" might not always be possible to use based on the underlying device drivers, this could prevent someone from using the API for devices that expects no STOP between write and read of multiple bytes.

Describe the solution you would like.
Adding a "transaction" API in addition to the existing read/write APIs. A transaction would imply a combined write/read action where first a number (N) of bytes is written and then followed by a number (Z) of bytes to read within the same START -> STOP window.

The API suggestion is the following:
iot_i2c_transaction(handle, writeBuf, N, readBuf, Z);

Expectation on bus:
START -> ADDR -> WB[0] -> ... -> WB[N] -> RB[0] -> ... -> RB[Z] -> STOP

Additional context
This is a common feature of device specific drivers as controlling the stop bit is typically abstracted away quickly from HW to SW. Supporting this could simply port implementation while making sure that the common-io API layer is not lacking features that the HW supports simply because of limitations in device specific hardware.

Thank you!

[dcgaws]

@qiutongs what are your thoughts?

[qiutongs]

I generally agree with this idea. It is a cleaner API to use in typical case of I2C: write-then-read.

Just call out my two thoughts:

1. Even device driver doesn't support "no stop", "eI2CSendNoStopFlag" can still be implemented. For example, the first write call can save the parameters in a temporary buffer but not start the transaction; the second read call will actually start the transaction and prepend the saved information from write call. (This is what I did for TI here)

All I want to say is that we thought about this problem and there was possible workaround. But I agree it is NOT elegant.

2. I like the idea of creating a new transaction function. What do you think if we generalize this idea and make iot_i2c_transaction combine a list of read/write operation requests. What you described can be thought as a special case: one write and one read.

I got this idea from Linux's I2C API.

ioctl(file, I2C_RDWR, struct i2c_rdwr_ioctl_data *msgset)

Ref: https://landlock.io/linux-doc/landlock-v11/i2c/dev-interface.html

@cobusve @vjjunnut Please share your thoughts.

[vjjunnut]

Qiutong, We have transfer API in SPI as its bi-directional. But for I2C this is a sequence of operations in 1 API. So basically instead of user doing this - you are exposing yet another API which would essentially do the same thing (configure controller for sending no stop after write, do the write and then read). I do not think, we want to add support in HAL that would combine some functionality in to APIs.

[osloapps-max]

@vjjunnut @qiutongs
While I agree that you could possibly implement this with the current set of features, I think making the assumption that if "send no stop" would simply buffer the first call and then start the transaction on the second call would be confusing at best.

My personal feelings is that it makes sense to treat also the I2C API as a bi-directional API instead of controlling the stop bit on a per operation status. It makes it clearer for the user that you can perform a sequence of writes followed by a sequence of reads inside the same "frame/transaction". If you then need to interface with single operations, you can simply do a uni-directional transaction.

Another "pro" of making a very clear and generic "transaction" API with defined behavior is that the behavior should be very similar between ports which should make it easy to re-use/move code between platforms. I would guess that the "workaround" suggested by @qiutongs would result in port specific behaviors which would most likely end up costing the user in terms of portability between platforms.

[qiutongs]

@osloapps-max
Thank you for the valuable feedback and we really understand your points. But let me share a different idea from my previous comment, after talking to the team. There a reason that we designed the API that way.

To make the library truly "common", our design philosophy is to support every use case of I2C, as long as the I2C specification allows. That being said, we tried to make the APIs aligned with I2C primitives in the I2C specification and "no stop" is one of them. Here are some special use cases that a transaction API cannot solve:

1. Master needs slave to tell it how many bytes to read. Here master performs an initial read and get value from slave. Based on that value, master performs multiple reads subsequently. And all of these happen in one transaction. (Microchip Gest-IC)

2. Master needs to determine whether responding ACK or NAK based on the value read from slave.

We are aware that these might not be typical I2C use cases, while transaction API is great pattern for most cases. But as I mentioned, we want to be common.

There will be further discussion on this topic internally. I will keep you posed.

[osloapps-max]

@qiutongs I do understand your points, they make sense (especially in cases like the Gest-IC series as the "message" seems to be deleted after a stop is generated).

I still think adding "transaction" in is not a bad idea as the I2C spec more or less defines a transaction to be what takes part in between a start and stop condition. Assuming the current API caters one need, having a transfer API would cater the general use-case where the data sizes is known. This would also assure that devices where you can't easily suppress the stop bit on a byte basis is instead starved of the functionality to perform multiple writes/reads in a convenient way.

Just to clarify, I never pushed for the current API to be changed in terms of the functionality it already have, I simply push for adding a transaction API in addition do it.

[qiutongs]

Sorry for late reply.

We are glad to know that you understand our rationale of using the “no stop” flag. Regarding adding an extra transaction API, we concern about the portability of Common IO library. We don’t want to make any read/write APIs as optional; otherwise the application code will not be portable.

[osloapps-max]

Assuming any device that can do read/write can also do the transfer, I don't see how it would affect portability? For common-io ports, if you can implement read/write then you can implement transfer and it should be safe from a portability perspective.

If there is any portability issues It should be similar to those that arise for ports not supporting the "Don't Send Stop" flag.

A transfer API defined as N x writes followed by Y x reads inside the same START/STOP window might be impossible to implement for some ports that could also not implement the stop flag suppression, that is true. But from experience, I would guess that implementing such an API would actually not be an issue for most platforms which means it could be more versatile than just having read/write and depending on individual ports to support the "Don't Send Stop" flag.

In the case where the "transfer" API could not be implemented as above due to the "stop bit" limitation, it could be handled by defining that in such a case, the transfer API should be equivalent to that of performing individual write/read operations in a grouped way.

This would mean that there should be absolutely no reason for a port not to be able to implement it, and portability should not be harder then than it would be now.

Another gain from this is that you can (possibly) write a lot cleaner code for large chunks of operations.