SPI.Configure() signature is inconsistent across platforms

[bgould]

In machine_atsamd21.go and machine_atsamd51.go, SPI.Configure() signature declares that it returns an error. This does not appear to be consistent with other platforms, such as machine_nrf.go, machine_generic.go, etc which do not declare the error. It might make sense to align these signatures so that code is more portable across platforms.

[deadprogram]

Agreed, they should be the same.

Probably we should now define an interface like SPIer. Just kidding about this name. Perhaps better to name it it SPIPort. I do not want ot name it SPIReaderWriter because it is does not fit into the normal Go Reade or Writer interfaces, which would probably make it confusing for people who are used to the standard Go interfaces of those same names.

In any case, whatever we end up calling it, we can then implement this interface to ensure that the code fulfills the expectations and matches the code signatures.

What do you think?

[aykevl]

I agree we should use a consistent interface.

Regarding interfaces: I think all drivers should eventually be switched to accept a SPI interface instead of a fixed machine.SPI. That would make it possible to write software implementations of those drivers and would perhaps also make it possible to use these drivers for different purposes outside of TinyGo.

[glococo]

HI ! I was trying to use the SPI on AVR but found no SPI / SPI0 on [ boards_[with_atmega], machine_atmega, machine_avr, machine_atmega328p ]

But found SPI on machine_nrf.go and spi.go which rised me the following questions: 1) Which function or declaration have precedence ? spi.go-> func (spi SPI) Tx(w, r []byte) error machine_nrf.go -> func (spi SPI) Tx(w, r []byte) error { 2) Both have differenes and the SPI struct is also different.

I was thinking of dirty implementing the ISP for myself but wish to know the ISP consistency status.

Thank you so much

[deadprogram]

If you are interested in implementing SPI for AVR, that sounds great. Please take a look at https://gist.github.com/deadprogram/1e914f0c40e80ff9b10588bfe61379d7 it will probably help get you started.

There has been API changes to how register values are set since when that gist was created. You will need to use the functions exported by the volatile package see https://github.com/tinygo-org/tinygo/blob/master/src/machine/machine_atmega.go for examples.

[glococo]

It seems interesting, SPI ports range from 0 to 3 in AVR-8bit family. https://www.microchip.com/ParamChartSearch/chart.aspx?branchID=30047&popular=1

I will try for fun to implement it inside machine_atmega. I discover Go just few days ago.

[aykevl]

spi.go-> func (spi SPI) Tx(w, r []byte) error machine_nrf.go -> func (spi SPI) Tx(w, r []byte) error {

These are the same? If you're wondering whether it should return an error or not, for new code it is better to let Tx return an error (this can be nil if no error handling is implemented).

Both have differenes and the SPI struct is also different.

The struct may be different, yes. That is not a problem. The important thing is that the public interface (such as the Tx function) has the same signature everywhere. This is not yet the case, that should indeed be fixed (patches welcome!).

It seems interesting, SPI ports range from 0 to 3 in AVR-8bit family.

Yes, there are many different AVR chips. That's why an SPI implementation is usually made for a (group of) chips, not for all chips from a given vendor.

[glococo]

Ok, I think I have made something flexible, however I need to figure out how to check if a register exists or not in avr device/avr and make sure this update is safe with the GC, if I should use pointers somewhere, etc..

``` include "runtime/volatile" // SPI Interrupt vector type SPI_RX_Async func(byte, error) // Atmega SPI type SPI struct { SPCR *volatile.Register8 SPSR *volatile.Register8 SPDR *volatile.Register8 RX_async SPI_RX_Async } // Starter SPI interface, maybe should be declared in board_... files var SPI0 = SPI{ } // SPIConfig is used to configure the SPI Port type SPIConfig struct { Master bool SCK Pin MOSI Pin MISO Pin LSBFirst bool Port uint8 Mode uint8 Frequency uint8 } func (spi SPI) Configure(c SPIConfig) error { if ( c.SCK==Pin(0) || c.MOSI==Pin(0) || c.MISO==Pin(0) ) { return errors.New("SPIConfig is incomplete") } var SPCR uint8 if c.LSBFirst { SPCR |= avr.SPCR_DORD } switch c.Mode { case SPI_Mode0: case SPI_Mode1: SPCR |= avr.SPCR_CPOL case SPI_Mode2: SPCR |= avr.SPCR_CPHA case SPI_Mode3: SPCR |= avr.SPCR_CPOL | avr.SPCR_CPHA } if c.Master { SPCR |= avr.SPCR_MSTR switch c.Frequency { case SPI_Clk_4: case SPI_Clk_16: SPCR |= avr.SPCR_SPR0 case SPI_Clk_64: SPCR |= avr.SPCR_SPR1 case SPI_Clk_128: SPCR |= avr.SPCR_SPR1 | avr.SPCR_SPR0 default: SPCR |= avr.SPCR_SPR0 } c.SCK.Configure( PinConfig{ Mode: PinOutput }) c.MOSI.Configure( PinConfig{ Mode: PinOutput }) } else { // SPI-Slave c.MISO.Configure( PinConfig{ Mode: PinOutput }) } switch { case (c.Port==SPI_Port2 && avr.SPCR2!=nil): spi.SPCR= avr.SPCR2 spi.SPDR= avr.SPDR2 spi.SPSR= avr.SPSR2 case (c.Port==SPI_Port1 && avr.SPCR1!=nil): spi.SPCR= avr.SPCR1 spi.SPDR= avr.SPDR1 spi.SPSR= avr.SPSR1 case (c.Port==SPI_Port0 && avr.SPCR0!=nil): spi.SPCR= avr.SPCR0 spi.SPDR= avr.SPDR0 spi.SPSR= avr.SPSR0 default: spi.SPCR= avr.SPCR spi.SPDR= avr.SPDR spi.SPSR= avr.SPSR } spi.SPCR.Set( SPCR | avr.SPCR_SPE ) return nil } func (spi SPI) Transfer( data byte ) (byte, error){ spi.SPDR.Set( data ) for !spi.SPSR.HasBits( avr.SPSR_SPIF ) { } return spi.SPDR.Get(), nil } ``` Specs: 0) TinyGo harmonized SPI calls 1) The SPI struct should be able to handle SPI0....SPI3, and registers without suffix or with number suffix. 2) I also would like to let you attach an async callback to each SPI int vector I'm stuck with those avr.SP_Rx registers check. Compiler complain. Thanks ps: I found in stm32f4... machine file. unsafe.Pointer( ... ps2: attiny and potentially others have miso, mosi or sck on pins 0, so pin(0) detection should be changed. Is there any way of setting a default value, like -1 or NoPin in that struct ? ps3: I exposed those 3 registers in the SPI struct to not just be able to store which SPI port it construct. but also to write or read to them if someone want to, since almost if not every AVR have the same register names.

[deadprogram]

This was addressed some time ago in TinyGo, so now closing. Thanks everyone!