ESP32 hardware SPI receive broken since 2v06

[gfwilliams]

See: http://forum.espruino.com/conversations/357245/#comment15677900

// Short D12 and D13 with a jumper

var s = new SPI();
s.setup({miso:D13, mosi:D12, sck:D14});
print(JSON.stringify(s.send("Hello World")));
// "Hello World"
print(JSON.stringify(s.send("Hello World")));
// "Hello World"

var s = SPI1;
s.setup({miso:D13, mosi:D12, sck:D14});
print(JSON.stringify(s.send("X")));
// "\xFF"
print(JSON.stringify(s.send("X")));
// "\x00"
print(JSON.stringify(s.send("XY")));
// "XY" <- actually works (seems it works if a multiple of 2 bytes?)
print(JSON.stringify(s.send("Hello World")));
// "Hello Worl\xFF"
print(JSON.stringify(s.send("Hello World")));
// "Hello Worl\x00"

// Trying to use SPI2 at this point causes a full reboot
// E (76511) spi_master: spi_bus_initialize(156): dma channel already in use

// But let's assume we don't use SPI1 so this
// actually works...

//var s = SPI2;
//s.setup({miso:D13, mosi:D12, sck:D14});
// print(JSON.stringify(s.send("XY")));
// "He"
// print(JSON.stringify(s.send("This is a test")));
// "Hell\u0000\u0000\u0000\u0000\u0000\u0000Hell"

// So it seems SPI2 is actually using SPI1's receive buffer?

This seem related to the SPI DMA modifications that went in after 2v06: #1887

@MaBecker @atc1441 could you take a look at this please? This seems totally broken and I'd have thought even the quickest of tests would have shown it wasn't working correctly.

[MaBecker]

Reading and writing at the same time

Looks like this is a knowing issue found here

Half-duplex transactions are not compatible with DMA when both writing and reading phases are used.
If such transactions are required, you have to use one of the alternative solutions:

1. Use full-duplex transactions instead.

2. Disable DMA by setting the bus initialization function’s last parameter to 0 as follows:
   ret=spi_bus_initialize(VSPI_HOST, &buscfg, 0);

This can prohibit you from transmitting and receiving data longer than 64 bytes. 3. Try using the command and address
fields to replace the write phase.

//    compiled and tested with ret=spi_device_queue_trans(SPIChannels[channelPnt].spi, &spi_trans, 0);
var s = SPI2;
s.setup({miso:D19, mosi:D23, sck:D18});
print(JSON.stringify(s.send("This is a test")));
"This is a test"

So what about a additional spi option like {dma:true} with default value false?
The use of dma transfer to displays is the huge performance kicker - as we all know - so not just a nice to have at all.

Trying to use both hardware SPI at this point causes a full reboot

There is a fixed dma channel with value 1 used in jshSPISetup() that is causing this full reboot.

Espruino/targets/esp32/jshardwareSpi.c

Line 133 in 24ce12a

esp_err_t ret=spi_bus_initialize(SPIChannels[channelPnt].HOST, &buscfg, 1);

Possible fix is to give each SPI it's own dma channel.

void jshSPISetup(
    IOEventFlags device, //!< The identity of the SPI device being initialized.
    JshSPIInfo *inf      //!< Flags for the SPI device.
) {
  int channelPnt = getSPIChannelPnt(device);
  int dma_chan = 0;  
  Pin sck, miso, mosi;
  if(SPIChannels[channelPnt].HOST == HSPI_HOST){
    dma_chan = 1;
    sck = inf->pinSCK != PIN_UNDEFINED ? inf->pinSCK : 14;
    miso = inf->pinMISO != PIN_UNDEFINED ? inf->pinMISO : 12;
    mosi = inf->pinMOSI != PIN_UNDEFINED ? inf->pinMOSI : 13;
  }
  else {
    dma_chan = 2;
    sck = inf->pinSCK != PIN_UNDEFINED ? inf->pinSCK : 5;
    miso = inf->pinMISO != PIN_UNDEFINED ? inf->pinMISO : 19;
    mosi = inf->pinMOSI != PIN_UNDEFINED ? inf->pinMOSI : 23;
  }

........

  esp_err_t ret=spi_bus_initialize(SPIChannels[channelPnt].HOST, &buscfg, dma_chan);

........

test result

>var s = SPI2;
=SPI2
=undefined
>print(JSON.stringify(s.send("This is a test")));
"This is a test"
=undefined

>var s = SPI1;
=SPI1
=undefined
>print(JSON.stringify(s.send("ABC")));
"ABC"
=undefined

What do you think?

[gfwilliams]

Thanks for looking into this!

So what about a additional spi option like {dma:true} with default value false?

Honestly that seems like a lot of added complexity. If you add some platform-specific fields like that nobody's really going to use them (or will use them wrong - eg for receiving). If it's some code that only you know about it might as well just be in your own branch :)

How about just automatically disabling DMA (maybe even using Espruino's fallback jshSPISendMany code) in the case that 'rx' is non-null (eg you're supposed to receive data)?

Possible fix is to give each SPI it's own dma channel.

That sounds perfect - as long as it doesn't break anything else?

[gfwilliams]

Also just wanted to check, some similar changes went in for ESP8266 - does this mean that is broken too?

[MaBecker]

Also just wanted to check, some similar changes went in for ESP8266 - does this mean that is broken too?

Yes I tried but it was never merged. As I am mot using SPI on ESP8266 devices I have no clue if is is working

[MaBecker]

Possible fix is to give each SPI it's own dma channel.

That sounds perfect - as long as it doesn't break anything else?

Who know's, as the main ESP32 developer are not longer active .....

[MaBecker]

Ok, let's do some LCD testing with out DMA .....

[MaBecker]

>var s = SPI2;
=SPI2
>s.setup({miso:D19, mosi:D23, sck:D18}); 
>print(JSON.stringify(s.send("This is a test")));
"This is a test"

Fast display updates work too.

[gfwilliams]

Great - thanks for this!