tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
587 stars 180 forks source link

I2C: discard WriteRegister and ReadRegister methods in interface #578

Closed soypat closed 1 year ago

soypat commented 1 year ago

Resolves #559. So I managed to pull this off because all usages of the I2C type were VERY consistent, so props to the tinygo developers!

soypat commented 1 year ago

@aykevl Hey, do you think it's worth rewriting WriteRegister to use the stack in the optimistic case data is small?

func WriteRegister(bus drivers.I2C, addr uint8, reg uint8, data []byte) error {
    var stackbuf [8]byte
    if len(data) <= 7 {
        stackbuf[0] = reg
        copy(stackbuf[1:], data)
        return bus.Tx(uint16(addr), stackbuf[:len(data)+1], nil)
    }
    buf := make([]uint8, len(data)+1)
    buf[0] = reg
    copy(buf[1:], data)
    return bus.Tx(uint16(addr), buf, nil)
}
aykevl commented 1 year ago

Yeah that's an option, but actually I think a more reliable option would be to add a new type for drivers to use:

type I2CWrapper struct { // name TBD
    drivers.I2C
    buf [8]byte
}

func (bus *I2CWrapper) WriteRegister(addr uint8, reg uint8, data []byte) error {
    if len(data) > len(bus.buf)-1 {
        panic("out of range") // or some other panic message
    }
    bus.buf[0] = reg
    copy(bus.buf[1:], data)
    return bus.Tx(uint16(addr), bus.buf[:len(data)+1], nil)
}

With this, you could verify that a driver never sends more than 7 bytes (for example) and replace bus drivers.I2C with bus somepkg.I2CWrapper.

The reason for this is that for some chips, Tx lets the parameters escape (so your alloc on the stack is still heap-allocated). Allocating the buffer while allocating the driver avoids this issue. This is not possible in the general case, but for specific drivers you know how much you're ever going to send at once.

soypat commented 1 year ago

Yeah that's an option, but actually I think a more reliable option would be to add a new type for drivers to use:

Agreed, though for now lets leave it at this? Wrapping all i2c instances seems like labored work. I just had to replace patterns throughout the whole repo to get this PR.

deadprogram commented 1 year ago

Thanks for all the work on this @soypat

Merging now, and if we decide to do further work on the i2c interface it can happen from that point.