Proposal: add unexported interfaces for compile-time interface implementation check

[soypat]

I don't mean to shoot down https://github.com/kenbell/tinygo-target-validator, but it would be relatively simple to check for interface implementation on various APIs. It consists of adding some machine package wide interfaces and asserting interface fulfillment as so:

machine/interfaces_uart.go

//go:build atsamd21 || rp2040

// This can be wrapped in an init() function to prevent uarter from appearing as a package wide type while developing.

type uarter interface {
       	io.Reader
	io.Writer

	Buffered() int
}

// If this causes error then UART0 does not implement the expected UART interface. Please check
// method implementation or remove your target from build tags.
var _ uarter = UART0

It would be relatively simple to implement and maintain. I can help out adding them.

[soypat]

@kenbell I think this may be of interest to you.

[deadprogram]

@soypat I was thinking of something similar. IMO that is something we should be doing anyhow at this point, as long as it does not increase memory footprint much if at all to do so. Probably some investigation is called for here.

[kenbell]

I don't recall fully, but I think @aykevl had an argument against interfaces in the machine package. IIRC, if code uses the interfaces rather than the structs the binary size will increase? (at least I think that's why there are interfaces in drivers package but not in machine?) If the interface is private, and only used to validate the peripheral meets the interface, maybe that concern goes away?

I think it would be good to have some place where 'the definitive interface' is defined for UART, SPI, PWM, I2C, etc - ideally public, so mocks for testing drivers could also use it, rather than having two - one in machine and another in the drivers repo.

My prototype also uses interfaces (but locally declared), eg this: https://github.com/kenbell/tinygo-target-validator/blob/master/tests/peripherals/pwm/core/main.go It would be good not to have these declarations in lieu of a definitive source.

There's some things that aren't part of the interface as such, that I'd like to check - e.g. the contents of the XXXConfig structs and any symbolic constants like PinInput, PinOutput, etc.

P.S. looks like the interfaces in the drivers package are missing the Config methods - would be good to include those as part of the interface for each bus type.

[deadprogram]

We should be able to use this script to test the size difference of any experiments in this area: https://gist.github.com/aykevl/bc593ce06b22d64d8dcbb335a40a1e40

[soypat]

So I've done some testing with the microbit target since that's what the test was initially written for. There was only one case of binary size change... and it actually got smaller? Here are results:

before  after   diff
 13720  13716     -4  -0.0%  microbit ./testdata/channel.go
  2552   2552      0   0.0%  microbit ./testdata/alias.go
 10488  10488      0   0.0%  microbit ./testdata/atomic.go
  4840   4840      0   0.0%  microbit ./testdata/binop.go
  6788   6788      0   0.0%  microbit ./testdata/calls.go
  2712   2712      0   0.0%  microbit ./testdata/env.go
  8248   8248      0   0.0%  microbit ./testdata/filesystem.go
 11060  11060      0   0.0%  microbit ./testdata/float.go
  2776   2776      0   0.0%  microbit ./testdata/gc.go
  3772   3772      0   0.0%  microbit ./testdata/go1.17.go
  8044   8044      0   0.0%  microbit ./testdata/goroutines.go
  4056   4056      0   0.0%  microbit ./testdata/init.go
  2512   2512      0   0.0%  microbit ./testdata/init_multi.go
 10244  10244      0   0.0%  microbit ./testdata/interface.go
 63408  63408      0   0.0%  microbit ./testdata/json.go
  2496   2496      0   0.0%  microbit ./testdata/ldflags.go
 12624  12624      0   0.0%  microbit ./testdata/map.go
 28636  28636      0   0.0%  microbit ./testdata/math.go
 10148  10148      0   0.0%  microbit ./testdata/print.go
  2444   2444      0   0.0%  microbit ./testdata/rand.go
 25976  25976      0   0.0%  microbit ./testdata/reflect.go
  4720   4720      0   0.0%  microbit ./testdata/slice.go
  8984   8984      0   0.0%  microbit ./testdata/sort.go
 38180  38180      0   0.0%  microbit ./testdata/stdlib.go
  4384   4384      0   0.0%  microbit ./testdata/string.go
  4112   4112      0   0.0%  microbit ./testdata/structs.go
 53808  53808      0   0.0%  microbit ./testdata/testing.go
  2452   2452      0   0.0%  microbit ./testdata/zeroalloc.go
sum: -4 (-0.0%)

I've added the spier, uarter, and i2cer interfaces in the following fashion:

interface_uart.go

package machine

import "io"

func init() {
	type uarter interface {
		io.Writer
		io.Reader

		Buffered() int
	}

	var _ uarter = &UART{}
}

[soypat]

@kenbell The definitive interface idea is tricky. We can't really place it inside tinygo since that would mean the go tool breaks when trying to parse an import path of the form import "interfaces" (this contradicts the drivers package ideology which lets you use the interfaces in native Go) and if you place it in some repo, then the tinygo repo would not be self contained anymore...

Alas, I think you are left with having a definitive interface inside tinygo which lets us catch bad interface implementation at compile time and thus during a pull request (and even before the PR happens since the code would not compile!). This is invaluable to letting us worry less about correct I2C, UART and SPI method signatures during code review. Downside is Drivers package would have to mirror any changes to the interface, which is not too hard to do since these are pretty stable.

Another upside to this is that the definitive interface implementation in tinygo repo can be more precise about what implements thje interface than the drivers package can. Consider the SPI interface in drivers repo:

type SPI interface {
		Transfer(b byte) (byte, error)
		Tx(w, r []byte) error
}

Not very precise when you see what the definitive interface could look like in tinygo:

package machine

func init() {
	type spier interface {
		Configure(config SPIConfig) error
		SetBaudRate(br uint32) error
		Transfer(b byte) (byte, error)
		Tx(w, r []byte) error
	}

	var _ spier = &SPI{}
}

Awesome, by adding the definitive interface I just broke a bunch of targets due to mismatching interfaces!

What y'all think?

[kenbell]

I'm not sure I follow about import "interfaces" problem. I'm going to do some experimentation with having a public interface in the machine package. That would then be 'definitive' - how much impact would that have in practice?

[soypat]

AFAIK this would have a negative impact if it were the defacto interface used in the drivers repo. It would mean that the drivers would only work when compiling with the tinygo tool since there is no native Go implementation inside the standard library. So users on native Go depending on the tinygo/drivers package would have a broken import path when trying to use drivers.

As for the separate interface idea I think there is benefit in that one can have the TinyGo defacto standard implementation, defined in all preciseness that all devices should follow, and then the more lax drivers interface, which defines the minimum you need to interface with the given protocol. This provides the net benefit of allowing tinygo devices to use the drivers package and external libraries to the tinygo ecosystem to also make use of drivers, like periph.io and gobot, without being constrained to the definitive interface, which we can make as precise as possible without impacting end users.

[kenbell]

I'm not sure about having precise and not-precise interfaces. There should only be a single definition? The Configure methods are tricky, because they depend on the machine.XXXConfig struct. I can see where there might be a standard set of members in that struct and some platform-specific ones.

Having a quick play, it seems it is possible to do something like this where the machine/interfaces package is 'standard Go'

machine/interfaces/spi.go...

package interfaces

type SPI interface {
	Transfer(b byte) (byte, error)
	Tx(w, r []byte) error
}

In drivers...

package drivers

import "github.com/tinygo-org/tinygo/src/machine/interfaces"

type SPI = interfaces.SPI

In machine package...

import "machine/interfaces"

var _ interfaces.SPI = &SPI{}

(I'm using replace github.com/tinygo-org/tinygo => ../tinygo in drivers go.mod to prototype)

[soypat]

So I agree Configure methods can be disparate, but at least we can agree con some fields of these structs, like config.Frequency. which could be added to the static check with something like

_ = SPIConfig{
	Frequency:0,
}

I still think we are better off having a strict interface for tinygo device implementation and a lax one for drivers. It would provide so many benefits in the long run: code would be more portable across devices since there would be no more Configure calls which don't return errors. The lax interface is also great since it provides an easy to implement interface so that we don't cut off other users of the drivers library which have different Configure method signatures.

About the tinygo interface implementation, if adding a interfaces package under machine works with the URL import path then it looks to be a viable option!

[soypat]

@deadprogram @aykevl I'd like to return to this issue, as API inconsitencies in the machine package are beginning to crop up more often by the day. I feel this is a serious problem that should be addressed since TinyGo is gaining a fair bit of attention.

Golden interface and Config struct

I propose we solve the problem by creating a file for each golden interface implementation within the machine package. i.e. machine/i2c_interface.go could look something like this:

//go:build !gameboyadvance && !mimxrt1062 && !mk66f18 && !attiny85 && !stm32 && !esp32c3 && !k210
// +build !gameboyadvance,!mimxrt1062,!mk66f18,!attiny85,!stm32,!esp32c3,!k210

var _ interface{  // 2
	SetBaudRate(br uint32) error
	Configure(config I2CConfig) error
	ReadRegister(address uint8, register uint8, data []byte) error
	WriteRegister(address uint8, register uint8, data []byte) error
	Tx(addr uint16, w, r []byte) error
} = (*I2C)(nil)

var _ = I2CConfig{ // 3
	Frequency: int(0),
}

Breakdown of the file:

1. Top lines are build tags that exclude those targets which do not have I2C hardware OR which do not yet have a a I2C implementation which conforms to the TinyGo standard (explained next)
2. There is an inline interface definition. This is the TinyGo standard for the I2C implementation. If the I2C type does not implement the standard, the whole package will not compile.
3. Finally a standarization of the I2CConfig type fields and field types.

Impact

This would

• Prevent new boards added to TinyGo from failing to conform to the standard unless explicitly stated in the corresponding build tag.
• Explicitly document which boards do not conform to the standard, which would make it easy for us to know where there is work to do.
• Standardize, that's always a good thing?
• Maybe compiler fails to optimize this code out in some certain case? though it seems to me highly unlikely

Refer to the PR that attempted a fix (which was way to ambitious) here.. I played with some of these ideas and tried to bring them to Tinygo but failed because the PR was too ambitious.

Suggestions on how to proceed

I would add these files immediately to the TinyGo repo along with their interface standardization

• machine/i2c_interface.go
• machine/spi_interface.go
• machine/uart_interface.go
• Any other interface that can be standardized. The interfaces can be as stringent as desired, since they are not seen by exogenous code modifying them will have NO effect on users (or even developers, these are anonymous interface definitions!).

Any board that does not conform to the standard today should be excluded with build tags. This will not be a breaking change and should have net 0 effect on TinyGo today. It will prevent non-conforming boards from being added in the future. As time goes on non-conforming boards could be fixed and removed from the build tags.

[aykevl]

I don't recall fully, but I think @aykevl had an argument against interfaces in the machine package. IIRC, if code uses the interfaces rather than the structs the binary size will increase? (at least I think that's why there are interfaces in drivers package but not in machine?) If the interface is private, and only used to validate the peripheral meets the interface, maybe that concern goes away?

Yes, if the interface is private and only used to do a type check, then it should be a no-op at runtime and not cause an increase in code size. In that case, it seems fine to me.

AFAIK this would have a negative impact if it were the defacto interface used in the drivers repo. It would mean that the drivers would only work when compiling with the tinygo tool since there is no native Go implementation inside the standard library. So users on native Go depending on the tinygo/drivers package would have a broken import path when trying to use drivers.

Agreed. We use interfaces in the drivers repo precisely so that these drivers can be used outside of TinyGo. If we're going to use interface definitions in the machine package, there was no point in using interfaces at all.

In any case, I like the idea of @soypat (although I would suggest using non-exported interface definitions instead, for readability and maintainability).

[aykevl]

Here is a related PR that approaches this issue in a very different way: https://github.com/tinygo-org/tinygo/pull/3170