search

rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 201 forks source link

i2c: Merging of consecutive operations in transaction contract #582

Open sgoll opened 7 months ago

sgoll commented 7 months ago

The following question concerns the transaction contract in I2c, i.e. when using the transaction() method.

Due to the specified merging of consecutive operations of the same type (e.g. two writes one after the other), it does not seem possible to issue separate operations (of the same type) right after each other, using a repeated start condition, without adding another "dummy" operation of the other type in-between. Is this correct?

Was this behavior intended when embedded-hal reached stable version 1, or was it an oversight, or kept deliberately out of scope?[^1]

[^1]: Unfortunately, I am not able to find any relevant discussions for this here on GitHub.

The main reason for having support for this kind of explicit repeated start condition is write operations (not so much read operations): many I2C devices expect a control register or address as the first byte (or first two bytes) of each write. But when consecutive write operations are always merged, it is not possible to write to two or more separate such locations in the target device.

Example: TLC59116 LED driver

I want to be able to set several PWM values at once. Using the driver's auto-increment feature, I can easily set a range by writing the address of the first register, then writing the target values of this register and the following registers.

But in order to set only two specific PWM values (say LED 5 and LED 11), I would want to make two writes, i.e. with a repeated start condition in-between: write first value's address followed by its value, generate a repeated start condition, then write second value's address followed by its value.

Right now, this can only be achieved by separate write() calls but with the side effect of generating both a stop and start condition (instead of repeated start without stop) which releases the bus in-between the two writes.

Unfortunately, I don't have a good suggestion on how to best model an API that allows both: the merging of consecutive operations (which is still very useful in and of itself) and the explicit repeated start between such operations.

The only thing I can think of is the addition of an explicit Operation::Restart (or similar) which would serve as sentinel or marker to keep two operations from being merged.

The question then is if this could even be introduced without breaking compatibility concerning the Operation enum and downstream implementations of the I2c trait. Probably not.

sgoll commented 4 weeks ago

Is this something that should be tracked in the v2.0 milestone?[^1] My suggested solution would be to introduce a third enum variant Operation::Restart.

[^1]: With an uncertain future, for the reasons lined out in https://github.com/rust-embedded/embedded-hal/issues/633#issuecomment-2373736663.

jannic commented 3 weeks ago

Is restart supported by all I2C implementations? If it's not, should there be an implicit fallback to STOP/START, or should the function return an error?

sgoll commented 3 weeks ago

Is restart supported by all I2C implementations? If it's not, should there be an implicit fallback to STOP/START, or should the function return an error?

All I2C implementations that I am aware of (mostly STM32) have first-class support for repeated start conditions—in fact, repeated start conditions are essentially what allows us to implement transactions in the first place—and using the same type of transfer twice in a row (read versus write) is just a coincidence, from the point of view of the protocol.

Using a combined transaction differs from individual transfers in separate transactions in a significant way (releasing the bus between such transfers) and I2C devices may make use of this fact in arbitrary ways. That said, I think we should not fall back to stop/start. Using transactions requires support for repeated start conditions anyway.

The necessity for a separate enum variant in Operation is only because of the current implied merge behavior:

  • Data from adjacent operations of the same type are sent after each other without an SP or SR.

To be clear, this behavior is useful and it should not be removed.[^1] But for opting-out of it (to send consecutive but non-merged transfers of the same kind, as laid out in the original post), some kind of marker is necessary.

[^1]: With merging, we can set up a list of transfers without temporary memory allocations that would otherwise be necessary to merge data into a single packet beforehand.