Discard `ReadRegister`+`WriteRegister` methods from `drivers.I2C` interface

[soypat]

Background

Today the I2C interface looks like so:

// I2C represents an I2C bus. It is notably implemented by the machine.I2C type.
type I2C interface {
    ReadRegister(addr uint8, r uint8, buf []byte) error
    WriteRegister(addr uint8, r uint8, buf []byte) error
    Tx(addr uint16, w, r []byte) error
}

As the name suggests, the machine package has a file called i2c.go that takes the usual I2C implementation, which is often implemented with only theTxmethod for most if not all microcontrollers, and implements theReadRegisterandWriteRegister` methods for the I2C type roughly as follows:

func (i2c *I2C) WriteRegister(address uint8, register uint8, data []byte) error {
    return i2c.Tx(uint16(address), append([]byte{address}, data...), nil)
}

func (i2c *I2C) ReadRegister(address uint8, register uint8, data []byte) error {
    return i2c.Tx(uint16(address), []byte{register}, data)
}

It cannot be denied this is extremely convenient for writing drivers.

That said, it does pose an unavoidable disadvantage: incompatibility with periph's I2C implementation. This means tinygo drivers cannot be used with the periph package.

Possible Solution

If we discard the ReadRegister and WriteRegister methods we will immedately break lots of drivers. There are 207 and 206 usages of these methods in this repository alone across 47 files.

Since most of these drivers store the I2C interface in-struct we can change the in-struct storage to be a type wrapping the drivers.I2C type which does implement these two methods like the machine package does and all should be solved.

Impact

This would allow gophers that work with Raspberry Pi 3/4 to use TinyGo drivers freely. It would also mean that we can omit the ReadRegister WriteRegister implementations in the machine package, which to me seem out of place- an I2C implementation is "completely" defined by only the Tx method.

I've also tended to always avoid these two aforementioned methods since I am aware they allocate memory and I can easily implement methods on my device that avoid allocations since I usually know the size of the buffers I'm working with.

[deadprogram]

Since most of these drivers store the I2C interface in-struct we can change the in-struct storage to be a type wrapping the drivers.I2C type which does implement these two methods like the machine package does and all should be solved.

That is probably not a trivial effort, but I can see the advantage of doing this.

I've also tended to always avoid these two aforementioned methods since I am aware they allocate memory and I can easily implement methods on my device that avoid allocations since I usually know the size of the buffers I'm working with.

I was thinking of changing the implementation in the machine package of those methods to use a pre-allocated buffer instead of the make. Of course, we would need to add a warning about it not being "thread safe".

If we copy the same implementation here in drivers, that would probably work to implement this proposal.

[aykevl]

Yes, I am in favor of this change :) compatibility with periph would be really nice. We could start by phasing out ReadRegister and WriteRegister, possibly by renaming the current machine.I2C to something different (machine.I2CLegacy?) and converting drivers one by on to the new interface. That way, the converted drivers will immediately see the benefit without needing to do this conversion for all drivers at once.

Since most of these drivers store the I2C interface in-struct we can change the in-struct storage to be a type wrapping the drivers.I2C type which does implement these two methods like the machine package does and all should be solved.

Sounds like a good idea. For convenience, we could add a [8]byte field that is used for WriteRegister if it is small enough, to avoid heap allocations in the common case (without checking, I expect the vast majority of WriteRegister calls to have 7 bytes or less of payload).

I was thinking of changing the implementation in the machine package of those methods to use a pre-allocated buffer instead of the make. Of course, we would need to add a warning about it not being "thread safe".

As long as this buffer exists per I2C peripheral, that's fine. It's only a problem when one buffer is shared between I2C buffers, which I think is a bad idea because people are not going to read that warning and will likely start using these I2C peripherals in different goroutines anyway. I'm not a fan of this idea, as the machine package doesn't know the maximum size that might be transmitted, while a driver does.

[deadprogram]

This was already released so now closing. Thanks!