Add writeto_then_readfrom to I2C API. Deprecate stop kwarg.

[tannewt]

writeto will now always output a stop bit and writeto_then_readfrom
has been added to do a write -> no stop -> repeated start -> read
sequence. This is done to match the capabilities of Blinka on Linux.

Code broken by this change will have been broken on Blinka anyway.
To fix, if stop=False then use writeto_then_readfrom otherwise use
writeto then readfrom_into.

[deshipu]

This is going to break a lot of stuff. Would it be possible to have the writeto_then_readfrom added for one release, and then remove the stop parameter in the next, to give more time for users to update? Otherwise I will have to maintain two versions of all of my libraries.

[dhalbert]

I wonder if there are pathological I2C chips for which we still want fine control of stop. Blinka is constrained by the underlying Linux support, and we don't need to match its foibles if there are cases where it's limiting.
@ladyada do you have a comment here?

[ladyada]

the I2C FRAM needed repeated-start for reading the device id but not for data. https://github.com/adafruit/Adafruit_FRAM_I2C/blob/master/Adafruit_FRAM_I2C.cpp#L109

most chips specify they want a stop but will work fine with repeated start, so we default to repeated start in the register library and it works just fine.
https://github.com/adafruit/Adafruit_CircuitPython_Register/blob/master/adafruit_register/i2c_struct.py#L86

i prefer we keep stop in and throw an exception for linux when it isnt (currently) supported. having fine control of stop is a feature of a low level i2c controller. blinka can't support everything on every platform - for example pullup resistors aren't supported on almost all linux boards, or independent SPI chip selects.

[deshipu]

Well, if a chip wants a stop, then that's fine — you can do that simply with a second transaction. The problem is when you want to do a repeated start, without the stop. writeto_then_readfrom is one case of that, but there are three other combinations (write-write, read-read and read-write) that can potentially be used by some chips. I think I only saw write-write myself in the wild (write the register address, do a repeated start, and write the register value). EDIT: I checked, and it wasn't actually a repeated start for write, so I have only seen the write-read in practice.

But I think it would be nice to have a version in which both writeto_then_readfrom and stop=False work, so that we have some more time to do the transition and don't have to release separate versions of everything for CP 4.x and CP 5.x, or do some weird boilerplate try blocks.

[tannewt]

@ladyada It's not clear to me that we need fine grained control of stop. Looking at the FRAM, the device ID read looks like the standard write (address 0xf8 has data direction 0 for write), no stop, repeated start, read (0xf9 is the same address with direction bit high) that we'll now have with the composite call.

I'm all for not limiting ourselves to what Linux can do. However, in this case I think Linux does everything that I2C devices need and by aligning our APIs through implicit control of stop we'll make maintenance of libraries easier.

Going forwards if a stop bit isn't desired, the only way to do it will be writeto_then_readfrom. To have a stop bit, one simply calls writeto and then readfrom_into.

@deshipu Is there a reason you are using busio directly rather than with BusDevice which already does the checking for writeto_then_readfrom? Couldn't it also work on rPi if it was switched over?

5.x stable is a couple months away still and breaking code is the most reliable way to get it updated. Leaving both options also leaves the option not to update it. I'm happy to help fixup code if it's open source and would prefer to break and fix now to have it finished.

[ladyada]

im ok with it for 5.0 :)

[dhalbert]

Do we need to test this with an FRAM breakout or do we know it will work?

[ladyada]

it shoud be fine, if you need a stop, you'd implement as two commands instead of one...

[deshipu]

@tannewt BusDevice is huge in terms of memory and rather slow. I'm controlling displays on PewPew with I2C, so speed is rather important.

I'm not concerned with me failing to update the library. The scenario I'm concerned with is me updating the library, which then fails to work with all the older versions of CircuitPython, because people didn't update their boards immediately (or can't do it, for instance because they use HUZZAH). That would mean I either would have to effectively support twice as much code (and have confusing releases for different versions), or have boilerplate code detecting what APIs are available and branches that do different things depending on that. I'm not particularly thrilled by either.
(I'm thinking about my libraries for the PewPew Lite Featherwing, by the way, not the standalone PewPew — those will have the library frozen, so no problems with them).

Having a release with both writeto_then_readfrom and stop would create w window in which both old and new code will work — that will let me write code that works both in stable release and in the betas, not leaving either group of people in void.

[tannewt]

@deshipu I'd suggest snagging writeto_then_readfrom from I2CDevice on it's own then. https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/blob/master/adafruit_bus_device/i2c_device.py#L145

I don't want to leave both because it means 1) new code could be added using the old way and 2) existing code with the old way may not be found and updated.

@dhalbert Please weigh in on this PR.

[dhalbert]

I have been trying to avoid this because I don't have much background in it. 😃 .
If there are really zero I2C chips that need separate stop control with write-then-read, then it's ok with me in the long run. If not, then leaving it in isn't wrong.

@deshipu, if you can do the attribute check as suggested by @tannewt, then that might be good enough.

If busdevice is too slow, we should consider how to speed it up (make it native?). In the code link above, we could at least check for the attribute just once and remember the result.

[deshipu]

I rest my case.

If we ever come upon a chip that requires an interaction that we don't support, we can always add it.

[tannewt]

Ok, I've done as @deshipu asked. writeto_then_readfrom is added and stop is documented as deprecated. Once 5.x is stable we can remove the stop= kwarg.

Please take another look.

[tannewt]

Ok, thanks @deshipu. Did all you asked. The build is failing due to code size. It's the same builds I'm tweaking in #2085 so I'll sit on this until then.

[tannewt]

@dhalbert please take another look. Had to turn off bitbangio for itsy bitsy.

[dhalbert]

Too bad about bitbangio. In the long run we need to find some more space somewhere, like LZW message compression, or ... Worth discussing, later, but let's get an alpha out.