Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i2c: Merging of consecutive operations in transaction contract #582

Open
sgoll opened this issue Mar 21, 2024 · 3 comments
Open

i2c: Merging of consecutive operations in transaction contract #582

sgoll opened this issue Mar 21, 2024 · 3 comments
Milestone

Comments

@sgoll
Copy link

sgoll commented Mar 21, 2024

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

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.

Footnotes

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

@sgoll
Copy link
Author

sgoll commented Oct 14, 2024

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.

Footnotes

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

@Dirbaio Dirbaio added this to the v2.0 milestone Oct 15, 2024
@jannic
Copy link
Member

jannic commented Oct 18, 2024

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
Copy link
Author

sgoll commented Oct 20, 2024

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.

Footnotes

  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants