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

docs/library/machine.I2C.rst: Extend writeto to support a list of bufs. #4020

Closed

Conversation

@dpgeorge
Copy link
Member

commented Aug 4, 2018

As it currently stands, the machine.I2C API cannot efficiently write out to a device data that is made up of multiple buffers. For example if you need to send out cmd and data in one transaction then it must be done via i2c.writeto(addr, cmd + data). This is not ideal because it needs to create a temporary buffer. If there are lots of bytes to send then that means allocating a big buffer, which should be avoided.

For further discussion about this problem see #3482, which is in the context of the ssd1306 display driver.

The proposal in this PR (which is just an update of the docs, implementation would follow later) is to extend i2c.writeto() to allow to pass in a list of buffers to write. For example it would allow:

i2c.writeto(addr, [cmd, data])

which would send the address, then cmd, then data (ie address is only sent at the start of the entire transaction).

The reasons for going for this approach are:

  1. It's a backwards compatible extension.
  2. It's convenient for the user.
  3. It's efficient because there is only one Python call, then the C code can do everything in one go.
  4. It's efficient on the I2C bus because the implementation can do everything in one go without pauses between blocks of bytes.
  5. It should be possible to implement this extension in all ports, for hardware and software I2C.

Alternative approaches, like using start/write/stop, or splitting it into multiple writeto calls, don't satisfy points 2-5 above.

There could be an equivalent extension to i2c.readfrom_into() but I don't think it's as necessary as for writeto, so I didn't add it but it could be added in the future if needed.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

As it currently stands, the machine.I2C API cannot efficiently write out to a device data that is made up of multiple buffers.

This isn't specific to I2C. And #2180 proposed a generalized solution for this - stream .writev() method long ago. (Except that if .writev() was implemented on streams, it wouldn't help I2C, because it doesn't embrace stream protocol itself. So each of possible hardware (and non-hardware) interfaces would need to invent a solution again and again - in the end they all will be alike, but with discrepancies and inconsistencies due to inconsistent design).

@DoubleVee73

This comment has been minimized.

Copy link

commented Aug 11, 2018

Whilst I can't properly comment on points 1 and 5 due to a lack of knowledge, points 2,3 and 4 make complete sense.

Thanks for looking at this - this really improves the deployment of I2C.

As an aside, would you happen to know yet when a feature improvement such as this will make it into the main release of Micropython?

@peterhinch

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2018

A key advantage over multiple writeto calls is the ability to send multiple data blocks not punctuated by I2C stop states. This matters for hardware such as SSD1306. The current driver achieves this with primitive I2C operations only available on software I2C instances. This PR would enable the driver to be changed such that it was hardware/software I2C agnostic.

Currently (as far as I can see) an agnostic driver can only be written by copying the command and data into a single buffer for transmission; this has obvious drawbacks.

This came up on the forum recently.

@DoubleVee73

This comment has been minimized.

Copy link

commented Aug 12, 2018

What are the obvious drawbacks with this approach, Peter? My understanding is the framebuffer object (in the instance of using SSD1306 for example) is transmitted in one bulk transfer anyway?

@peterhinch

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2018

The SSD1306 hardware expects a command followed by the framebuffer data without an I2C stop condition occurring between the two. With the current API this can be done in two ways:

  1. Use primitives: this is how it is currently achieved, but it forces software I2C.
  2. Create a new buffer containing the command byte followed by the framebuffer data. This is slow, clunky and causes RAM allocation but works with hard or soft I2C.

The proposed API would enable the SSD1306 driver to be hard/soft agnostic.

@DoubleVee73

This comment has been minimized.

Copy link

commented Aug 12, 2018

Ah ok. This is where my lack of understanding of the lower mechanics are very much exposed:) Thanks for explaining - I now understand your reticence.

So a buffer is created in the framebuffer object, but in order to insert an additional byte at the beginning (the 0x40 data command for example), a new buffer needs to be created and the original 'copied' - which consumes processing and RAM?

@peterhinch

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

Yes: it requires a RAM allocation. While the RAM may be reclaimed in a garbage collect, allocations are best avoided in device drivers. Further, copying all the data is wasteful of CPU cycles.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2018

And #2180 proposed a generalized solution for this - stream .writev() method long ago. (Except that if .writev() was implemented on streams, it wouldn't help I2C, because it doesn't embrace stream protocol itself. ...)

I agree that consistency is nice to aim for, but, as pointed out, I2C is fundamentally not a stream and I don't see any other efficient way to support sending consecutive buffers than what is proposed in this PR.

Considering writev: if it was adapted to take an address it could be i2c.writev_to(addr, data1, data2, ...). But then to support the existing writeto() behaviour it should also take a (optional) stop parameter. If it were optional at the end of the arg list it would need to be a keyword argument, which adds extra code. Otherwise it could be placed before the data args, but then be required.

But the main issue I see with this form of writev_to is that a variable number of arguments usually leads to heap allocation, which should be avoided. For example, if one wanted to define a custom I2C class in Python then the writev_to method would need to have a *args argument which definitely allocates heap memory when called. Another example would be wrapping a C-level I2C class in a Python logger that logs read/write calls. Such a case would require allocating memory passing the variable arguments through.

The proposal in this PR has a fixed number of arguments so there is no allocation when calling the function (you just need to preallocate the list/tuple for the sequence of buffers, but that only needs to be done once).

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2018

@dpgeorge: Well, thanks for the discussion.

Considering writev: if it was adapted to take an address it could be i2c.writev_to(addr, data1, data2, ...).

So, why, if it was adopted, it would be like that, given all the concerns you raise with vararg funcs? Perhaps, it would be adopted as .writev([data1, data2, ...]), just like you propose. Which brings us back to the consistency of API. We definitely don't want to extend generic stream .write() to support both buffer or list of buffers, because this adds hard to avoid performance hit, I mean hard to avoid even with advanced compile-time optimization. So, if that support would be added to generic streams, it would have to be .writev(). Then consistent name for I2C's version would be still i2c.writev_to(addr, [cmd, data]).

So, for me personally overloading normal .writeto() feels more hacky than e.g. #4217 . Both that and this PR do what needs to be done anyway. #4217 at least does it once, done. But this PR does it in a way which leads to concerns like:

So each of possible hardware (and non-hardware) interfaces would need to invent a solution again and again - in the end they all will be alike, but with discrepancies and inconsistencies due to inconsistent design).

Anyway, what needs to be done, needs to be done.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

Then consistent name for I2C's version would be still i2c.writev_to(addr, [cmd, data]).

So it comes down to a name, not a signature, and whether an existing name is reused and overloaded, or a new method name is introduced.

From the point of view of minimal code, the reuse of a name and overload of a method is the way to go.

For a new method, if the signature is no longer to take variable arguments but rather a list/tuple of buffers, then writev doesn't seem like the best choice, but rather something like writel (l for list) or writem (m for multi).

Note that it's quite a general thing to be able to combine buffers in a list/tuple: memoryview is used to "cut" a region out of another buffer, and putting multiple buffers together in a list/tuple is like a "paste" operation.

Then again, for true stream objects, writing a list of buffers is not really needed, one can instead just have multiple calls, one for each buffer.

For SPI, which can in principle be transaction based with the CS line (as opposed to a true stream), would it need to be able to write a list of buffers in one go? Perhaps. But then, because of the nature of SPI, it would also need to be able to read into a list of buffers. Using separate method names would then lead to the addition of: SPI.writel(), SPI.readintol(), SPI.writel_readintol(). IMO it's simpler just to overload the existing write/readinto/write_readinto methods.

Since the need to write/read a list of buffers is really specific to non-stream objects, I don't see that much of an issue in just overloading the already-special methods like writeto.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

So it comes down to a name, not a signature, and whether an existing name is reused and overloaded, or a new method name is introduced.

Yes, or more specifically meta-principles (== consistent higher-level principles) which are applied to decide and resolve such cases.

From the point of view of minimal code, the reuse of a name and overload of a method is the way to go.

That only applies if "minimal code" is the one and only governing principle. But it's not, or nobody would spend extra code to implement @micropython.native and stuff. But they are implemented, because there's a desire to not just provide "minimal code", but also "decent performance" (selectable by user on a case by case basis). And ironically (and as discovered by many other projects), just emitting machine code for dynamic language doesn't make it fast enough. You need to cut piece by piece "overdynamic" behavior. And one such behavior is having too-overloaded methods, where each invocation starts with dispatching for a particular behavior. Cutting that off, and having a specific behavior per method is beneficial, if extra effort is to be spent in the direction of getting more speed in MicroPython. (And it would be very said, literally, a project failure from my PoV, if no effort in that direction is planned).

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

For a new method, if the signature is no longer to take variable arguments but rather a list/tuple of buffers,

Well, leaving write (write_to) alone to do just one thing, writev actually can be made to accept either a list of varargs. But given the motivation described above, sticking to just list would be a choice to make.

then writev doesn't seem like the best choice, but rather something like writel (l for list) or writem (m for multi).

Well, hopefully the target audience of the programming language are programmers. And programmers do know where the name "writev" comes from: https://linux.die.net/man/2/writev , http://gunkies.org/wiki/4.2_BSD :

readv(), writev(): 4.4BSD (these system calls first appeared in 4.2BSD),

4.2 BSD: Year Introduced: | 1983

So, this name is known for 35 years. There may be novice programmers who don't know where it comes from. But programmers has always been "people who are ready, and eager, to learn". Having some other target audience would be very weird for a programming language.

writel (l for list) or writem (m for multi).

"writel" means "write long". "writem" means nothing.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Since the need to write/read a list of buffers is really specific to non-stream objects

But that's how it all started. The idea is that there's a specific interface (in Java terms) of stream, and there's a "meta-interface" (concept" of stream. Anything which transfers series of data is by definition a stream conceptually. So, if stream-the-interface would have writev() methods (but it doesn't have to, by the reasons you described), the streams-by-concept would have methods writev_to(), writev_in(), writev_up(), etc., not methods where instead of "writev" something else is used.

Otherwise, I don't have other arguments. It's not about a single case again, it's about meta-principles of the API design. And you can just assess what's more important to have: consistent principles, or cut corners in adhoc way for each specific case for adhoc benefits.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

In #4763 I provide an implementation of the idea presented here, which works for all existing ports.

It uses i2c.writevto(addr, vector) as the new function, ie not overloading the existing writeto. There's no underscore in the name because that would lead to lots of underscores in other names to keep it consistent.

Honestly I can go either way with it being writeto or writevto. If anyone has a preference please let it be known, otherwise I'll just go with writevto given the discussion above.

dpgeorge added a commit that referenced this pull request May 20, 2019
docs/machine.I2C: Add writevto method to write a vector of byte bufs.
This allows to efficiently send to an I2C slave data that is made up of
more than one buffer.  Instead of needing to allocate temporary memory to
combine buffers together this new method allows to pass in a tuple or list
of buffers.  The name is based on the POSIX function writev() which has
similar intentions and signature.

The reasons for taking this approach (compared to having an interface with
separate start/write/stop methods) are:
- It's a backwards compatible extension.
- It's convenient for the user.
- It's efficient because there is only one Python call, then the C code can
  do everything in one go.
- It's efficient on the I2C bus because the implementation can do
  everything in one go without pauses between blocks of bytes.
- It should be possible to implement this extension in all ports, for
  hardware and software I2C.

Further discussion is found in issue #3482, PR #4020 and PR #4763.
@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Merged writevto version in 8bec0e8

@dpgeorge dpgeorge closed this May 20, 2019

@dpgeorge dpgeorge deleted the dpgeorge:docs-machine-i2c-buflist branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.