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: support writes with a list of buffers #4746

Closed

Conversation

dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented May 3, 2019

This implements the ideas for extending i2c.writeto() as previously discussed in #4020 (which is still pending). It allows to pass in a list of buffers to write all at once, eg:

def write_data(i2c, data_buf):
    i2c.writeto(addr, (b'\x01', data_buf))

There are a few commits here:

  • first is to change the C-level I2C API to support partial I2C transactions: sending a start+addr, then successions of buffers in separate C calls
  • second to update stm32 to use this new C-API
  • third to use this new C API to remove the need for temporary memory allocation in the existing i2c.writeto_mem()
  • fourth is to extend i2c.writeto() to accept a tuple/list of buffers

It's not a trivial change. Going down this route puts a burden on ports to implement the new I2C C-level API. Some ports will have trouble doing this because the I2C abstraction they expose is not powerful enough to support such partial transactions, even though I2C as a bus is completely fine doing such things.

esp32 port will be ok, it provides a way to compose multiple writes. nrf port won't be ok, it'll either not support this, need memory allocation to support it, or require low-level changes to its HAL (the hardware can support it, of course). zephyr port will be ok because it provides composition (although will require a small buffer to be set aside for holding the messages).


The alternative is to not go down this path at all. That would then put the burden on the user to find ways around the problem this is trying to solve, namely avoid dynamic memory allocation to combine buffers. That filters all the way up to the top-level user API because they can no longer simply pass buffers down into I2C-based drivers containing data/payload, in case such buffers must be combined with addition data before being sent out (in a single I2C transaction). Eg to send a string to a device might look to the user like display.show('hello world') but within the driver this string needs to be copied to a temporary buffer (of unknown initial size) and extra bytes added.

So the choice is to push complexity all the way down to the I2C peripheral driver, or all the way up to the end user.

For example: i2c.writeto(addr, (buf1, buf2)).  This allows to efficiently
(wrt memory) write data composed of separate buffers, such as a command
followed by a large amount of data.
@jimmo
Copy link
Member

jimmo commented May 3, 2019

👍 For all the reasons you've outlined above and on #4020 and #3482.
#3482 was exactly the sort of experience where I wished for the complexity to be pushed to the I2C driver. And compared to the alternative of providing a transactional API, at least this approach can be implemented by the NRF port (albeit with memory allocation).

Speaking of writeto_mem, would you consider making it also support a tuple/list of buffers too? (I don't imagine this is a common requirement though, mostly just a question of consistency).

For a port (e.g. NRF without making HAL changes) that only supports a single write(address, data, len) API that was being made to work using memory allocation, how did you imagine they would implement the start_addr / read_part / write_part API. Does start_addr need a total_length argument to support these ports?

@dpgeorge
Copy link
Member Author

dpgeorge commented May 3, 2019

at least this approach can be implemented by the NRF port (albeit with memory allocation).

If you go down to the register level it can be implemented without memory allocation, like stm32 does it. The point with some hardware/MCUs (eg SAMD I think, and stm32 to some extent) is that they don't offer arbitrary flexibility, eg they don't allow to send only a start condition. The C API here is supposed to be the fine line between what hardware can support and maximum flexibility for creating I2C transactions.

Speaking of writeto_mem, would you consider making it also support a tuple/list of buffers too?

I don't think it's worth it. The functionality can already be achieved using writeto() (and is easier now because you can specify the register address separately from the data, eg i2c.writeto(addr, (memaddr, data))).

For a port (e.g. NRF without making HAL changes) that only supports a single write(address, data, len) API that was being made to work using memory allocation, how did you imagine they would implement the start_addr / read_part / write_part API. Does start_addr need a total_length argument to support these ports?

The way these C functions work is that you must pass in some "continuation information" to tell what is coming next, so the underling I2C implementation can do its job. The full amount of data to send is not needed, just if it's 0, 1 or >1 bytes. In the simplest case the underling port can just buffer the calls, eg make a linked list / array of pointers to buffers, then allocate a big array at the end to concat all the data if necessary. It's a bit of extra work to compute the total length before the transaction starts because all input buffers must be extracted (from Python objects); the current implementation just extracts them one by one.

@jimmo
Copy link
Member

jimmo commented May 3, 2019

👍

In the simplest case the underling port can just buffer the calls, eg make a linked list / array of pointers to buffers, then allocate a big array at the end to concat all the data if necessary.

I was thinking you'd need to pre-allocate it but yeah of course that works too. (Sorry for the noise)

@dpgeorge
Copy link
Member Author

Superseded by #4763.

@dpgeorge dpgeorge closed this May 20, 2019
@dpgeorge dpgeorge deleted the extmod-i2c-partial-transactions branch May 20, 2019 05:10
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

Successfully merging this pull request may close these issues.

None yet

2 participants