SPI.transfer(buffer, size) definition incompatible with other cores

[s-light]

challenge

i try to use the ulrichstern/Tlc59711 LED-Driver library to use with an ESP32 Dev Module.

this currently results in this compilation error:

/home/stefan/mydata/arduino_sketchbook/libraries/ulrichstern_Tlc59711/Tlc59711.cpp: In member function 'void Tlc59711::xferSpi()':
/home/stefan/mydata/arduino_sketchbook/libraries/ulrichstern_Tlc59711/Tlc59711.cpp:148:39: error: no matching function for call to 'SPIClass::transfer(uint16_t*&, int)'
       SPI.transfer(buffer2, bufferSz*2);
                                       ^
In file included from /home/stefan/mydata/arduino_sketchbook/libraries/ulrichstern_Tlc59711/Tlc59711.cpp:11:
/home/stefan/.arduino15/packages/esp32/hardware/esp32/2.0.0/libraries/SPI/src/SPI.h:70:10: note: candidate: 'void SPIClass::transfer(uint8_t*, uint32_t)'
     void transfer(uint8_t * data, uint32_t size);
          ^~~~~~~~
/home/stefan/.arduino15/packages/esp32/hardware/esp32/2.0.0/libraries/SPI/src/SPI.h:70:10: note:   no known conversion for argument 1 from 'uint16_t*' {aka 'short unsigned int*'} to 'uint8_t*' {aka 'unsigned char*'}
/home/stefan/.arduino15/packages/esp32/hardware/esp32/2.0.0/libraries/SPI/src/SPI.h:71:13: note: candidate: 'uint8_t SPIClass::transfer(uint8_t)'
     uint8_t transfer(uint8_t data);
             ^~~~~~~~
/home/stefan/.arduino15/packages/esp32/hardware/esp32/2.0.0/libraries/SPI/src/SPI.h:71:13: note:   candidate expects 1 argument, 2 provided

after some research i found that in the ESP32 core the definition of SPI.transfer(buffer, size) differs to other cores.

Minimal Example

with this the error can be reproduces if the board is a ESP32 Dev Kit
with AVR or SAMD it is compiling fine.

#include <SPI.h>

void setup() {
    SPI.begin();
    SPI.beginTransaction(SPISettings(2*1000*1000, MSBFIRST, SPI_MODE0));

    uint16_t *buffer;
    const uint16_t bufferSz = 10;
    buffer = reinterpret_cast<uint16_t*>(malloc(2*bufferSz));

    SPI.transfer(buffer, bufferSz*2);
}

void loop() {
}

down the rabbit hole

currently the definition is:
libraries/SPI/src/SPI.h#70

void transfer(uint8_t * data, uint32_t size);

libraries/SPI/src/SPI.cpp#229

void SPIClass::transfer(uint8_t * data, uint32_t size) 
{ 
	transferBytes(data, data, size); 
}

this was added in #2136.

in the avr core its defined as

inline static void transfer(void *buf, size_t count) {

and in the samd core as

void transfer(void *buf, size_t count);

solution

so to be cross-platform compatible it is needed to change the parameters to the same types - void *data, size_t count

i don't know yet what is need to get the called transferBytes function to work with this.
i tested with this implementation:

void SPIClass::transfer(void * data, size_t size)
{
    transferBytes(
        reinterpret_cast<uint8_t*>(data),
        reinterpret_cast<uint8_t*>(data),
        size);
}

its inspired by the other implementations - and it seems to work.
but i am not 100% sure if there are other side effects - so please review!

Hardware

Board: ESP32 Dev Module
Core Installation version: 2.0.0
IDE name: Arduino IDE
Flash Frequency: 80Mhz
PSRAM enabled: no
Upload Speed: 921600
Computer OS: Kubuntu 20.04

[me-no-dev]

how about you uint8_t *buffer; buffer = reinterpret_cast<uint8_t*>(malloc(2*bufferSz)); instead?

[s-light]

i can change the library - the buffer is used as uint16_t type - so this does not make much sens to change.
i can also do

SPI.transfer(reinterpret_cast<uint8_t*>buffer, bufferSz*2);

in the library.

my main request was to fine tune for an cross-core-compatible API

[Rotzbua]

@s-light Do not use the core implementations as reference because they may have different implementations. Better use the official ArduinoCore-API repository https://github.com/arduino/ArduinoCore-API .

The reference API has the virtual void transfer(void *buf, size_t count) signature. https://github.com/arduino/ArduinoCore-API/blob/e03b65374c614130aa1b11597e07b3b5089a726d/api/HardwareSPI.h#L109-L111

So I agree that there should be a change or pass through to have better interoperability.

[s-light]

@Rotzbua thanks for the pointer to the core api.
i did not know that this exists...

so the best way would be

void SPIClass::transfer(void *buf, size_t count)
{
    transferBytes(
        reinterpret_cast<uint8_t*>(buf),
        reinterpret_cast<uint8_t*>(buf),
        count);
}

should i create a pullrequest with this change?

[FrankBoesing]

I agree, this is just incompatible to arduino, which uses void*

[VojtechBartoska]

Hello @s-light, is this issue still valid?

[s-light]

i will look at it and recheck...
please give me some days.

[Rotzbua]

@s-light @VojtechBartoska I checked. The issue is still valid.
Still: void transfer(uint8_t * , uint32_t )
Should: void transfer(void *, size_t )