spi: remove machine.SPI and replace with drivers.SPI interface

[deadprogram]

This PR removes machine.SPI and replaces them with the drivers.SPI interface for almost all SPI drivers.

There are a couple of drivers that will require additional or special handling. Primarily this has to do with #206

The flash driver will require changes to any consumers to set the SPI speed before calling into the driver.

The ili9342 driver has very tight coupling to specific SPI code that probably belongs in the machine package:

https://github.com/tinygo-org/drivers/blob/release/ili9341/spi_atsamd21.go
https://github.com/tinygo-org/drivers/blob/release/ili9341/spi_atsamd51.go

[aykevl]

I'm all in favor of this change, but there is an unresolved issue: how to configure the SPI peripheral from a driver?

The most difficult case is the flash driver. The flash chip is only known during configuration, not before (unless the user tells the driver which flash chip it is, which is less flexible). Therefore, either the flash driver would have to assume a (lower) safe default speed or have a SetFrequency.

I'm leaning towards adding a SetFrequency (which is really a "set max frequency" as the actual frequency may be lower than requested due to hardware limitations). This would of course need to be supported in the machine package for every supported chip. But still not sure about it.

What do you think? @deadprogram @conejoninja

[deadprogram]

I think adding a SetFrequency(speed uint32) error to the various SPI implementations in the machine package is best way to handle.

Note that we are going to have to deal with similar issue when we need to implement drivers.Pin interface.

What should we do about the ili9342?

[aykevl]

What should we do about the ili9342?

I think it should use plain SPI communication as defined in drivers.SPI. A quick look seems to suggest this is purely a performance optimization. Instead, Tx should be improved so that it is fast enough for the ili9342 (just like is done for the nrf).

[deadprogram]

@bgould and @sago35 what do you think about this?

[sago35]

@aykevl @deadprogram
The ili9341 SPI implementation is for speed optimization.

If machine.SPI.Tx() is fast, we can use machine.SPI.Tx().

I'm considering the following code.
The following code seems to work well for the most part, but I need to check it a little more, because there is an incorrect behavior on SERCOM0 (RTL8720DN) of WioTerminal.

I am also considering using DMA.
https://github.com/sago35/tinygo-dma

func (spi *machine.SPI) Tx(tx, rx []byte) {
	spi.Bus.DATA.Set(uint32(tx[0]))
	for !spi.Bus.INTFLAG.HasBits(sam.SERCOM_SPIM_INTFLAG_DRE) {
	}

	for i := 1; i < len(rx); i++ {
		spi.Bus.DATA.Set(uint32(tx[i]))
		for !spi.Bus.INTFLAG.HasBits(sam.SERCOM_SPIM_INTFLAG_RXC) {
		}
		rx[i-1] = byte(spi.Bus.DATA.Get())
		//time.Sleep(10 * time.Microsecond)
	}
	for !spi.Bus.INTFLAG.HasBits(sam.SERCOM_SPIM_INTFLAG_RXC) {
	}
	rx[len(rx)-1] = byte(spi.Bus.DATA.Get())
}

If you rewrite ili9341/spi_atsamd51.go as follows, it will run at similar speed.
txBuf should eventually be turned off, but it's necessary now.
pd.bus.Tx(*(*[]byte)(unsafe.Pointer(&data))) is treated as half length and cannot be used.

var txBuf = [(136 + 8) * (100 + 8) * 2]uint8{}

func (pd *spiDriver) write16sl(data []uint16) {
	// This part of the data can be eliminated by reversing the endian of the data
	// and using []bytes as input data.
	// Or we can create something like Tx(d []uint16) that takes []uint16 as an argument.
	for i, c := 0, len(data); i < c; i++ {
		txBuf[i*2+0] = uint8(data[i] >> 8)
		txBuf[i*2+1] = uint8(data[i])
	}

	pd.bus.Tx3(txBuf[:len(data)*2])
}

[deadprogram]

Ideally what I would like to be able to do is remove entirely the samd21 and samd51 special code used by that driver. If we can make it so it only needs to use the drivers.SPI interface then we can write unit tests like I have started to do for the I2C drivers.

[sago35]

You can use a standard machine.SPI driver with the following combination.

• https://github.com/tinygo-org/drivers/commits/spi-bus-samd5x
• https://github.com/tinygo-org/tinygo/commits/samd5x-spi

However, the 44 fps is slowed down to 24 fps.
There are two reasons for this:

• []uint16{} needs to be reordered
• Waiting for TXC every 2 bytes.

There are two ways to increase the speed
Whichever method you choose, you'll get about 42 fps

1. Eliminate TXC waiting in spi.tx()
   • However, if you want to wait for it to complete, you need a different interface.
2. Create RGBBitmap8(), which takes []uint8 as its argument.
   • You only need to wait for TXC once at the end, not every 2 bytes.

[sago35]

@aykevl @deadprogram
Also, I get the following error when I try to use drivers.SPI interface.
How can I solve this problem?

https://github.com/tinygo-org/drivers/commits/spi-bus-samd5x-drivers

sago35@sago35-B450GT3  ~/dev/src/github.com/tinygo-org/drivers (spi-bus-samd5x-drivers)
$ ~/tinygo/tinygo/bin/tinygo-0.16.0-dev-1096596 version
tinygo version 0.16.0-dev-1096596 linux/amd64 (using go version go1.14.4 and LLVM version 10.0.1)

sago35@sago35-B450GT3  ~/dev/src/github.com/tinygo-org/drivers (spi-bus-samd5x-drivers)
$ ~/tinygo/tinygo/bin/tinygo-0.16.0-dev-1096596 flash --target wioterminal --size short ./examples/ili9341
/pyportal_boing/
# tinygo.org/x/drivers/examples/ili9341/pyportal_boing
tinygo.org/x/drivers/examples/ili9341/pyportal_boing/<init>: interp: unknown GEP

traceback:
tinygo.org/x/drivers/examples/ili9341/pyportal_boing/<init>:
  %3 = getelementptr inbounds { %machine.SPI }, { %machine.SPI }* %2, i32 0, i32 0, !dbg !2109

[deadprogram]

I changed the pattern for using the wifinina driver over this, and also to make it more standardized like other drivers.

See https://github.com/tinygo-org/drivers/blob/spi-bus/wifinina/wifinina.go#L272-L281 and https://github.com/tinygo-org/drivers/blob/spi-bus/examples/wifinina/mqttclient/main.go#L63-L68

[aykevl]

@sago35

Also, I get the following error when I try to use drivers.SPI interface.
How can I solve this problem?

You are probably initializing some global here, you can move the initialization to the main function to work around it.
tinygo-org/tinygo#1430 should eventually fix it.

[aykevl]

However, the 44 fps is slowed down to 24 fps.

Which method are you using? DrawRGBBitmap?

One workaround could be to send buffers in batches, for example by allocating 32 bytes, preparing it and sending that. That should avoid most of the inefficiency.

In general I think the API for these displays can be improved for speed, but I'm not yet sure how exactly. The original SetPixel method is very simple, but also very slow.

[deadprogram]

i am eager to merge this PR, but still seems like we need to resolve a couple of items.

• Do we need to add a SetFrequency() method to the spi interface, and also make sure we have implementations in the TinyGo machine package?
• How do we handle the desire to bulk stream data to a SPI device vs. read/write without having a lot of machine package hacks so we can stick to whatever the SPI interface provides? Do we need to add something to the interface, or is this really a matter of machine package optimizations?

@sago35 @aykevl @bgould or anyone else please chime in. Thanks.

[aykevl]

• Do we need to add a SetFrequency() method to the spi interface, and also make sure we have implementations in the TinyGo machine package?

I would suggest doing the flash driver changes in a later commit, after TinyGo has gained something like a SetFrequency (which hopefully isn't too hard to implement for all chips). In other words, reverting the flash changes in this PR and doing them together in a later PR.

Right now the default SPI speed is 4MHz (see tinygo-org/tinygo#1447). I checked the supported SPI chips and all of them support at least 8MHz (with many supporting crazy high clock frequencies such as 65MHz) so there isn't a need to use a lower frequency at startup. So an alternative to delaying the machine.SPI transition for the flash driver could be to rip out all clock frequency switching and just using the default 4MHz default for now.

• How do we handle the desire to bulk stream data to a SPI device vs. read/write without having a lot of machine package hacks so we can stick to whatever the SPI interface provides? Do we need to add something to the interface, or is this really a matter of machine package optimizations?

For regular (blocking) SPI transactions the current API should suffice and fast implementations are possible (see tinygo-org/tinygo#1517 for example).
However, we might want to look into asynchronous operations. This would likely require a new API that starts a transaction without waiting for it to complete, and some way to re-synchronizing (waiting until the transaction is finished).

---

Aside from my issue with the flash driver, this change looks good to me.

[sago35]

However, the 44 fps is slowed down to 24 fps.
Which method are you using? DrawRGBBitmap?

One workaround could be to send buffers in batches, for example by allocating 32 bytes, preparing > it and sending that. That should avoid most of the inefficiency.

Yes, I am using DrawRGBBitmap.

It is more efficient to create a Buffer of a certain size first.
That's what DrawRGBBitmap should do, but it has the problem that the Byte-Order is different.
I' m planning to improve DrawRGBBitmap in another PR.

---

i am eager to merge this PR, but still seems like we need to resolve a couple of items.

ILI9341-pyportal-boing is a slow process, but I will try to improve ILI9341-driver later.
By using the following changes, the standard SPI-driver can handle ILI9341.
It is very, very slow, but we can improve it later.

d970077

[deadprogram]

OK I have now put the flash driver back how it was before. I have also added a make unit-test task which currently attempts to run go test on any drivers that do not have any dependency on the machine package. That is also something to be addresses in future PRs.

Everything is green, so I think this can now be merged.

[deadprogram]

Going to merge now, thanks everyone for the feedback!