spi/pin: how should we handle calls to Configure() inside drivers?

[deadprogram]

In the case of the I2C interface, since no driver calls the I2C Configure() method, we did not have any troubles refactoring all the I2C drivers to use the interface.

However in the case of both the SPI and the Pin objects passed to the New() function , some of the drivers then go on to call the Configure() method.

For example: https://github.com/tinygo-org/drivers/blob/release/flash/transport_spi.go#L54

Since Confgure() expects a concrete type as param here, it becomes difficult to then replace the call with an interface that does not depend on the machine package.

What should we do about this?

One option would be to expect any configuration to already be done and hence to not allow any drivers to call Configure().

Another would be to refactor the machine package itself to expect a pointer so that we can define Configure(interface{}) in the drivers.SPI interface.

Any other ideas on how to handle this?

[deadprogram]

Another would be to refactor the machine package itself to expect a pointer so that we can define Configure(interface{}) in the drivers.SPI interface.

To clarify, this would mean changing the function signature from:

Configure(cfg SPIConfig)

to

Configure(cfg *SPIConfig)

[conejoninja]

Is there any other driver that configures the spi? I think the drivers should not configure the transports, since it means they know more than they should. In the example it's setting a freq that might not be supported. Another option might be to extended the SPI interface to add a "SetClockSpeed" method, but I'm not convinced (better to keep the interfaces as simple as possible). I'm trying to understand the use case and why this needs to be done in the driver and not in main()

[deadprogram]

The only other places where there is use of SPI that goes beyond the interface are

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

I'm not sure why that functionality is in the driver?

Anyhow, the Pin also has many places where Configure() is being called from drivers. This would also require a similar change to the Configure() method.

[aykevl]

One option would be to expect any configuration to already be done and hence to not allow any drivers to call Configure().

Strongly in favor of this one, for the same reasons as @conejoninja.

[deadprogram]

I agree that we need to require all setup take place outside the driver.

We will have to make a number of changes to remove those calls.

So this change this should allow us to define an interface for Pin which will allow us to write unit tests for the SPI based drivers that use chip select.

In a future iteration I think we should define another function on the SPI interface ChipSelect(index int, state bool) error so we can put the implementation in the machine package while still keeping the shared interface.

[deadprogram]

PR #215 mostly solves this issue.

[aykevl]

I have an idea which would still make it possible to call Configure in some way (which also happens to be similar to tinygo-org/tinygo#1537). Which is to make variants of the Configure function, for example:

• spi.ConfigureWith(freq, sck, sdo, sdi)
• i2c.ConfigureWith(freq, sck, sda) or i2c.ConfigurePins(sck, sda)
• pin.ConfigureAs(mode)

I'm not sure about the names but you can see the idea here. These methods will simply call the appropriate Configure function, but avoids I2CConfig, SPIConfig, etc.

Alternatively, we could make the machine package available to the Go toolchain by moving it to tinygo.org/x/machine for example and let the stubs use machine.I2CConfig etc. anyway. The latter might also pave the way to use the TinyGo drivers on a Raspberry Pi or other SBC, which would be cool.

But this is only when it is needed within the driver, in my opinion in many cases the peripheral configuration should still happen outside the driver.

[deadprogram]

I was going to propose something like spi.Configure(*SPIConfig) but what you suggest is better as it makes things even more explicit.

[jadefox10200]

I am trying to use the driver mcp2515 and getting the following error:

panic: compiler: could not store type code number inside interface type code

goroutine 9 [running]:
github.com/tinygo-org/tinygo/transform.LowerReflect({0x9bc0880})
	/Users/distiller/project/transform/reflect.go:185 +0x905
github.com/tinygo-org/tinygo/transform.Optimize({0xc001c67dc8}, 0xc0001fc7e0, 0x2, 0x0, 0x5)
	/Users/distiller/project/transform/optimizer.go:91 +0x4a5
github.com/tinygo-org/tinygo/builder.optimizeProgram({0x796f980}, 0xc0001fc7e0)
	/Users/distiller/project/builder/build.go:740 +0x1ed
github.com/tinygo-org/tinygo/builder.Build.func2(0x0)
	/Users/distiller/project/builder/build.go:386 +0x657
github.com/tinygo-org/tinygo/builder.jobWorker(0x0, 0x0)
	/Users/distiller/project/builder/jobs.go:182 +0xad
created by github.com/tinygo-org/tinygo/builder.runJobs
	/Users/distiller/project/builder/jobs.go:101 +0x1a5

Is this related? It seems to be failing on my Begin function:

package main

import (
	"machine"
	"time"

	mcp "tinygo.org/x/drivers/mcp2515"
)

func main() {

	cs10 := machine.D10

	err := machine.SPI0.Configure(machine.SPIConfig{
		Frequency: 500,
	})

	if err != nil {
		println(err)
		return
	}

	// //pointer to a Device is returned. Must pass chip select pin and the SPI0
	can := mcp.New(machine.SPI0, cs10)
	//
	// //configure the mcp2515 device
	can.Configure()
	//
	// //start it:
	//(d *Device) Begin(speed byte, clock byte) error {
	err = can.Begin(mcp.CAN125kBps, mcp.Clock8MHz)
	if err != nil {
		println("Error on begin")
		println(err)
	}
	//
	// //loop through and check for messages: (I gueess??)
	for {
		if can.Received() {
			msg, err := can.Rx()
			if err != nil {
				println("ERROR ON RCV:")
				println(err)
				continue
			}
			println("GOT DATA:")
			println(string(msg.Data))
			time.Sleep(500 * time.Millisecond)
		}
	}

}

I'm using a arduino uno with a mcp2515 shield from Sparkfun.