I2C: write, write_read and read are unnecessarily limiting

[bjoernQ]

When trying to port the VL53L5CX-ULD driver to Rust I discovered that this device doesn't like e.g. the firmware writes (which are up to 32k) to be chunked. The device doesn't complain but it's also not working afterwards.

When using our very nice implementation of transaction things work fine - so we should consider re-using the internals of transaction for the other implementations as well. Additionally, this would reduce code duplication

Example of a successful write using transaction

You can see the short pause between the write operations but no STOP/START/RESTART condition - this is that the sensor likes to see

[bugadani]

Not sure why you linked to embedded-hal's SpiDevice::transaction instead of our actual implementation that you were referring to 🫠 I'd expect reworking this won't be a big task (note to the fixer: please fix the deterimed typo in L914 and 2133).

However, transaction uses the same internal master_read and master_write, so why does transaction behave better?

[bjoernQ]

... using our very nice implementation transaction ...
So, our implementation of that function ...

transaction uses write_operation / read_operation which call setup_write/setup_read - I don't see master_write/master_read in the call-tree

With transaction we can queue up any number of operations without START/RSTART/STOP (if not needed)

[bjoernQ]

While master_write / master_read use write_operation/read_operation they can only do one operation up to max 254 bytes

[bugadani]

transaction uses write_operation / read_operation which call setup_write/setup_read - I don't see master_write/master_read in the call-tree

Heh, must be the lack of coffee. I promise I looked before commenting 🙈 But you're right of course.

[bugadani]

Oh I know what confused me: transaction, when you pass a single write operation to it, calls write_operation the same way as master_write does. Doesn't this mean that their limitations are identical if you only issue a single write op? Isn't transaction also limited to 254 byte writes?

This leads to: should we start chunking the i2c::Operation buffers in transaction so that we can work with longer buffers?

[bjoernQ]

Oh I know what confused me: transaction, when you pass a single write operation to it, calls write_operation the same way as master_write does. Doesn't this mean that their limitations are identical if you only issue a single write op? Isn't transaction also limited to 254 byte writes?

Yes - maybe I was bad at explaining that. A single operation is also limited to 254 bytes

I don't think we should do chunking in transaction but we should in write,read and read_write IMHO (and probably explain that somewhere) - transaction already gives the user freedom to do it and maybe some weird devices out there have very special needs regarding chunking which we could satisfy with transaction

Would probably also be good if we had an inherent version of transaction?

[bugadani]

I don't think we should do chunking in transaction but we should in write,read and read_write

That's not so simple. We can't create a slice of operations, so we couldn't reuse transaction in its current form. We can probably reformulate transaction to take an impl Iterator, but that will probably not be ideal for code size.

Instead, if we could chunk in transaction, we could keep a single implementation. I'd expect that if a device has special needs, its driver would take care of those needs. Otherwise if a driver ends up generating a >254 byte write, we'd just fail anyway.

[bjoernQ]

We can't create a slice of operations

Yes - we would need to create an array and that would have a hard bound so we would still limit things and temporarily allocate memory - anyways we should probably provide an inherent transaction API and document things a bit better