SPI port control might be better with a CS pin

[schmidtw]

One bit that seemed clunky to me was the handling of the CS pin across all chips. The control is completely outside of the SPI code. The logic for when to enable/disable the CS pin being external makes sense given some of the weird logic hardware has around the CS pin, but it seems lacking that there wouldn't be a member in the SPI struct named CS that can be optionally specified & if done, then the SPI code can configure the pin to be high, then enable the output of the CS pin. Then it's also convenient because you can write things like machine.SPI0.CS.Low() vs. needing to keep that variable around in application code.

Here is an example of what I'm referring to: https://github.com/tinygo-org/tinygo/pull/1614#issuecomment-773060457

[aykevl]

The machine package is a low level but portable abstraction over the hardware. It is a thin layer over the underlying hardware, just enough to be portable across chips. And most chips do not have hardware support for the CS pin (at least in SPI controller mode) so that's why the interface does not include it. For example Nordic Semiconductor suggest to use a regular GPIO pin as CS pin.

I think a better way to add support for it is through a driver in the drivers repository which puts a SPI and a pin object in a single struct, which drivers can then use as they see fit.

[schmidtw]

Good suggestion. The Atmega chips are the same where they use a GPIO pin & it's all externally controlled.

The potential bug I am trying to help users avoid is https://github.com/arduino/ArduinoCore-avr/blob/9f8d27f09f3bbd1da1374b5549a82bda55d45d44/libraries/SPI/src/SPI.cpp#L58.

Should there be a way to either specify a list of CS GPIO pins that the SPI code could initialize on your behalf? Or a function that you can call that initializes the GPIO pin for you? I think most users will get tripped up by something like this since we assume the CS is "disabled" by default, except that most registers (and hence GPIO) is set to 0 at chip reset ... making the CS "enabled" by default.

[aykevl]

Why would the CS pin need to be initialized in the SPI configure method? If there was a package in the drivers repository that handles this, it could be done correctly once with no need to teach users about it.

[schmidtw]

The SPI CS logic might be different between chips & to make the devices SPI peripheral agnostic the devices should generally be given an already configured SPI port so the devices can do the same pattern that is done with I2C vs. what is presently being done in ili9341 where there are per chip implementations that escape into the drivers.

I'll write up an example to show how I think this could work to make it easier (like i2c) and link this issue.

[deadprogram]

Hi @schmidtw

I think the reason for the per chip implementations on the ili9341 and others is due to the desire for a more optimized byte transfer to displays with SPI interfaces. These is some ongoing effort to accommodate this in PR https://github.com/tinygo-org/tinygo/pull/1453 for example. The CS pin question has not come up in those discussions, afaik, so this is certainly a good time to discuss as we look to improve SPI support.

As far as can we entirely hide the CS pin for consumers of the SPI interface? I am not sure. There are so many variations between no CS pin at all, like several dedicated displays, to specific pins being used in the chip's own SPI block implementation.

In any case, I look forward to seeing some specific ideas to continue the conversation.

[aykevl]

Indeed, as @deadprogram indicates the ili9341 per chip implementations are not intended and are unrelated to the CS pin.

A better example would be the BMI160: https://github.com/tinygo-org/drivers/blob/dev/bmi160/bmi160.go. It has a CS pin and the NewSPI function simply takes the CS pin and the (already configured) SPI peripheral.