Why is SPI1 an extra class?

[christoph]

The Teensy LC has two (almost) identical SPIs. I was hoping that I could make use of that fact in DmaSpi, but SPI1 is implemented as a totally different class. They are entirely unrelated and don't have a shared base class, with minor additions in SPI1 for the FIFO features (and I think that's the only difference).

I'd really like to see a base class for both SPIs, because with that an instance of BasicSPI (or however it might be named) can be passed to the code that wants to use an SPI, and it would also be easier to write flexible code that is not hardwired for a specific SPI.

Related to this is the naming of *_SPI_t structs in cores/teensy3/kinetis.h: There's an KINETISK_SPI_t and a KINETISL_SPI_t, both wrapped in a conditional for each piece of hardware. I suggest to rename them to (or add a new define with the name) KINETIS_SPI_t that can be used in other code to "just get an SPI struct" (or a pointer or reference). Free functions can be used to manipulate such a struct on a low level, similar to the deprecated SPI::setBitOrder(...) methods. This would simplify the implementation of higher level SPI classes such as SPIClass and SPI1Class.

Why would I like a common SPI base for DmaSpi? This is due to the way the asynchronous chip selects work: They begin and end a transaction, and they also select the appropriate slave by pulling its SS line low. Beginning and ending a transaction requires a reference to the SPI used, and that must either be hardwired into the chip select class or passed to the chip select object at runtime. Either way, having a common base minimizes duplicated code. If the SPI class is hardwired into the chip select class, every chip needs its own chip select class and that would be a waste of resources. For now, my ActiveLowChipSelect covers most of the cases.

Regards

Christoph

 

[KurtE]

For what it is worth, While playing with trying to make a faster version of the ILI9341 driver for the LC, I have been playing with support for both objects, where for some of it I do have to do stuff like:

    void spiBegin(void)  __attribute__((always_inline)) {
        if (_fSPI1)
            SPI1.beginTransaction(SPISettings(ILI9341_SPICLOCK, MSBFIRST, SPI_MODE0));
        else
            SPI.beginTransaction(SPISettings(ILI9341_SPICLOCK, MSBFIRST, SPI_MODE0));
    }

For other stuff where I am playing with the actual SPI registers, my init sets up a pointer
KINETISL_SPI_t *_pKSPI;

That is setup in the ::begin method:

void ILI9341_TLC::begin(void)
{
    // verify SPI pins are valid;
    // SPI0 ...
    if ((_mosi == 11 || _mosi == 7) && (_miso == 12 || _miso == 8) && (_sclk == 13 || _sclk == 14)) {
        SPI.setMOSI(_mosi);
        SPI.setMISO(_miso);
        SPI.setSCK(_sclk);
        _pKSPI = &KINETISL_SPI0;
    } else if ((_mosi == 21 || _mosi == 0) && (_miso == 1 || _miso ==5) && (_sclk == 20)) {
        SPI1.setMOSI(_mosi);
        SPI1.setMISO(_miso);
        SPI1.setSCK(_sclk);
        _fSPI1 = 1;
        _pKSPI = &KINETISL_SPI1;
	} else 
        return; // not valid pins...

which I then use to access the SPI registers. Example;

    void writeSPIByte(uint8_t val) {
        uint32_t sr;
        do {
            sr = _pKSPI->S;
    		if ((sr & SPI_S_SPRF)) 
                uint32_t tmp __attribute__((unused)) = _pKSPI->DL;
		} while (!(sr & SPI_S_SPTEF)) ; // room for byte to output.
		_pKSPI->DL = val;
        fByteOutput = 1;

Note: yesterday I noticed that my Arduino 1.0.6 (installed using windows installer), updated the spi.h/cpp files as part of the install, but this version did not include SPI1, nor the definitions for it.

 

[christoph]

I think a pointer or a reference to an SPI struct could simply be passed to the class constructor:

extern SPIClass SPI; // default constructor for SPI0
#if defined(__arm__) && defined(TEENSYDUINO) && defined(KINETISL)
extern SPIClass SPI1(&SPI1); // here with a pointer to SPI1
#endif

Or at least have SPI1Class derive from SPIClass. That would avoid about 90% of the duplicated code.

 

[PaulStoffregen]

I think a pointer or a reference to an SPI struct could simply be passed to the class constructor:

When considering SPI library changes, I have very little tolerance for any change that adds even a few extra clock cycles to the commonly used functions. Today, those functions are inline and optimized. Requiring them to all incur function call overhead, and especially a vtable function call, it totally unacceptable.

I am willing to consider changes, but any proposed change must be carefully analyzed for its effect on performance. With slower protocols like Serial and I2C, we can afford some extra overhead for flexibility. But not with SPI. I know this rules out a lot of popular C++ techniques and it probably makes some things impossible. I'm usually pretty willing to consider ideas that have some performance trade-off, but in this case SPI performance is far too important.

I'm listening for suggestions, but they must be vetted against this stringent performance requirement.

 

[christoph]

So how about the CRTP? Guaranteed compile-time polymorphism and the same amount of code is saved. I still have extra work in that case, though. But I can probably find a workaround for that.

 

[christoph]

Regardless of possible savings in duplicate code, is it possible to add a reference to the underlying KINETISx_SPI_t? That would also make things easier. I also asked for a method that switches between 8- and 16-bit transfers on github, but we can of course also discuss that here.