Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESP32 hardware SPI receive broken since 2v06 #1963

Closed
gfwilliams opened this issue Dec 3, 2020 · 23 comments
Closed

ESP32 hardware SPI receive broken since 2v06 #1963

gfwilliams opened this issue Dec 3, 2020 · 23 comments
Assignees
@MaBecker
Labels
ESP32 This is only a problem on ESP32-based devices

Comments

@gfwilliams
Copy link
Member

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.
@gfwilliams gfwilliams added the ESP32 This is only a problem on ESP32-based devices label Dec 3, 2020
@MaBecker MaBecker self-assigned this Dec 3, 2020
@MaBecker
Copy link
Contributor

MaBecker commented Dec 3, 2020

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
Copy link
Member Author

gfwilliams commented Dec 4, 2020 •
edited

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
Copy link
Member Author

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

@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

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
Copy link
Contributor

MaBecker commented Dec 4, 2020

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 MaBecker mentioned this issue Dec 4, 2020
Open
32 tasks
@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

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

MaBecker added a commit that referenced this issue Dec 4, 2020
@MaBecker
Update SPI (fix #1963)
1245529 
* disabling DMA just if rx is specified
@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020 

>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
Copy link
Member Author

Great - thanks for this!

gfwilliams added a commit that referenced this issue Dec 4, 2020
@gfwilliams
Merge pull request #1964 from espruino/esp32_improvement
3604727 
Update SPI (fix #1963)
@gfwilliams
Copy link
Member Author

SPI2 now works as well as SPI1 - thanks!

But they're BOTH still broken! If you try the code from the test right at the start of this issue it still fails in exactly the same way:

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"

I think your "This is a test" only works because as mentioned before it's a multiple of 2 bytes.

@gfwilliams gfwilliams reopened this Dec 4, 2020
@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

thanks for testing is again with a single char.

This will not fix it but you mixed pin numbers for miso and mosi; s.setup({miso:D12, mosi:D13, sck:D14});

Hmm, just tested on SPI2 and 1 char is broken on my side too.

this is the section:

Espruino/targets/esp32/jshardwareSpi.c

Lines 186 to 190 in 3604727

if (count==1) {
int r = jshSPISend(device, tx?*tx:-1);
if (rx) *rx = r;
return true;
}

@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

I think your "This is a test" only works because as mentioned before it's a multiple of 2 bytes.

it work's with 3 chars too

 >print(JSON.stringify(s.send("ABC")));
"ABC"

@gfwilliams
Copy link
Member Author

What about print(JSON.stringify(s.send("Hello World"))); ?

@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

Wow, what is wrong here .......

    if (count==1) {
      jsiConsolePrintf("%d\n",tx[0]);  // added this line to my local coding 
      int r = jshSPISend(device, tx?*tx:-1);
      if (rx) *rx = r;
      return true;
    }
>print(JSON.stringify(s.send("Hello World")));
100   <- (count == 1)
"Hello Worl\u0000"

>print(JSON.stringify(s.send("ABC")));
"ABC"

>print(JSON.stringify(s.send("ABCE")));
"ABCE"

>print(JSON.stringify(s.send("ABCEF")));
"ABCEF"

>print(JSON.stringify(s.send("ABCEFG")));
"ABCEFG"

>print(JSON.stringify(s.send("ABCEFGI")));
"ABCEFGI"

>print(JSON.stringify(s.send("ABCEFGHI")));
"ABCEFGHI"

>print(JSON.stringify(s.send("ABCEFGHIJ")));
"ABCEFGHIJ"

>print(JSON.stringify(s.send("ABCEFGHIJK")));
"ABCEFGHIJK"

>print(JSON.stringify(s.send("ABCEFGHIJKL")));
76   <- (count == 1)
"ABCEFGHIJK\u0000"

>print(JSON.stringify(s.send("ABCEFGHIJKLM")));
"ABCEFGHIJKLM"

>print(JSON.stringify(s.send("ABCEFGHIJKLMN")));
"ABCEFGHIJKLMN"

@rujorgensen
Copy link

rujorgensen commented Dec 4, 2020 •
edited

I seem to have an issue with using arrays (sorry for throwing another issue in here, I know you've been working hard on this today 😊).

Perhaps I don't understand SPI.send, but assuming these are equivalent: print(JSON.stringify(s.send("AB"))); print(JSON.stringify(s.send([0x65, 0x66])));:

The same output is not generated with:

const PIN_SCK = D18;
const PIN_MISO = D19;
const PIN_MOSI = D23;

let s = SPI2;
s.setup({
    sck: PIN_SCK,
    miso: PIN_MISO,
    mosi: PIN_MOSI,
    baud: 600000,
});

print(JSON.stringify(s.send("AB")));
// Outputs "AB"
print(JSON.stringify(s.send([0x65, 0x66])));
// Outputs "[255, 0]"

@MaBecker
Copy link
Contributor

MaBecker commented Dec 4, 2020

look's like jshSPISend()need some attention too.

@MaBecker
Copy link
Contributor

MaBecker commented Dec 5, 2020

New test code to get a clue what's goin on

var s = SPI2;
s.setup({miso:D19, mosi:D23, sck:D18});
for (var i =1; i< 50; i++) {
  var d = "".padEnd(i,"0");
  print((i.toString(10)).padStart(2,"0"),JSON.stringify(s.send(d)));
}

output

>count: 1, rx: 1
01 "\u00FF"
count: 2, rx: 1
02 "00"
count: 3, rx: 1
03 "000"
count: 4, rx: 1
04 "0000"
count: 5, rx: 1
05 "00000"
count: 6, rx: 1
06 "000000"
count: 7, rx: 1
07 "0000000"
count: 8, rx: 1
08 "00000000"
count: 9, rx: 1
09 "000000000"
count: 10, rx: 1
10 "0000000000"
count: 10, rx: 1
count: 1, rx: 1
11 "0000000000\u0000"
count: 10, rx: 1
count: 2, rx: 1
12 "000000000000"
count: 10, rx: 1
count: 3, rx: 1
13 "0000000000000"
count: 10, rx: 1
count: 4, rx: 1
14 "00000000000000"
count: 10, rx: 1
count: 5, rx: 1
15 "000000000000000"
count: 10, rx: 1
count: 6, rx: 1
16 "0000000000000000"
count: 10, rx: 1
count: 7, rx: 1
17 "00000000000000000"
count: 10, rx: 1
count: 8, rx: 1
18 "000000000000000000"
count: 10, rx: 1
count: 9, rx: 1
19 "0000000000000000000"
count: 10, rx: 1
count: 10, rx: 1
20 "00000000000000000000"
count: 10, rx: 1
count: 11, rx: 1
21 "000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
22 "0000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 1, rx: 1
23 "0000000000000000000000\u0000"
count: 10, rx: 1
count: 12, rx: 1
count: 2, rx: 1
24 "000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 3, rx: 1
25 "0000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 4, rx: 1
26 "00000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 5, rx: 1
27 "000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 6, rx: 1
28 "0000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 7, rx: 1
29 "00000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 8, rx: 1
30 "000000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 9, rx: 1
31 "0000000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 10, rx: 1
32 "00000000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 11, rx: 1
33 "000000000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 12, rx: 1
34 "0000000000000000000000000000000000"
count: 10, rx: 1
count: 12, rx: 1
count: 12, rx: 1
count: 1, rx: 1
35 "0000000000000000000000000000000000\u0000"
>
  • There is a logic that splits the send buffer in chunks of 10,12,12,... bytes to pass to jshSPISendMany()
  • Every time jshSPISendMany() is called with count==1 the wrong data is returned.
  • Fix jshSPISend() to fix the issue.

@MaBecker
Copy link
Contributor

MaBecker commented Dec 5, 2020

Ok restoring jshSPISend() and placing the read a the correct position which is at the end and not at the beginning fixed that issue on my side.

@MaBecker
Copy link
Contributor

MaBecker commented Dec 5, 2020 

>count: 1, rx: 1
01 "A"
count: 2, rx: 1
02 "AA"
count: 3, rx: 1
03 "AAA"
count: 4, rx: 1
04 "AAAA"
count: 5, rx: 1
05 "AAAAA"
count: 6, rx: 1
06 "AAAAAA"
count: 7, rx: 1
07 "AAAAAAA"
count: 8, rx: 1
08 "AAAAAAAA"
count: 9, rx: 1
09 "AAAAAAAAA"
count: 10, rx: 1
10 "AAAAAAAAAA"
count: 10, rx: 1
count: 1, rx: 1
11 "AAAAAAAAAAA"
count: 10, rx: 1
count: 2, rx: 1
12 "AAAAAAAAAAAA"
count: 10, rx: 1
count: 3, rx: 1
13 "AAAAAAAAAAAAA"
count: 10, rx: 1
count: 4, rx: 1
14 "AAAAAAAAAAAAAA"
count: 10, rx: 1
count: 5, rx: 1
15 "AAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 6, rx: 1
16 "AAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 7, rx: 1
17 "AAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 8, rx: 1
18 "AAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 9, rx: 1
19 "AAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 10, rx: 1
20 "AAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 11, rx: 1
21 "AAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
22 "AAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 1, rx: 1
23 "AAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 2, rx: 1
24 "AAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 3, rx: 1
25 "AAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 4, rx: 1
26 "AAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 5, rx: 1
27 "AAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 6, rx: 1
28 "AAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 7, rx: 1
29 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 8, rx: 1
30 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 9, rx: 1
31 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 10, rx: 1
32 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 11, rx: 1
33 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 12, rx: 1
34 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
count: 10, rx: 1
count: 12, rx: 1
count: 12, rx: 1
count: 1, rx: 1
35 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
> 
```

@MaBecker
Copy link
Contributor

MaBecker commented Dec 5, 2020 •
edited

Perhaps I don't understand SPI.send, but assuming these are equivalent: print(JSON.stringify(s.send("AB"))); print(JSON.stringify(s.send([0x65, 0x66])));:

close, values are int and yes this works no too with the latest pr.

print(JSON.stringify(s.send("AB")));
// Outputs "AB"
print(JSON.stringify(s.send([0x65, 0x66])));
// Outputs "[101,102]"

@rujorgensen
Copy link

will let you know when it is time to merge, like to spend some more hours on testing.

Don't know if you still want feedback, but I'm getting a new, rather nasty, error 🙂:

ASSERT(jsvGetLocks(var) < 15) FAILED AT src/jsvar.c:721
  #1[r1,l2] Object {
    #2[r1,l2] ASSERT(jsvGetLocks(var) < 15) FAILED AT src/jsvar.c:701
HALTING.

with

const PIN_SCK = D18;
const PIN_MISO = D19;  // D19
const PIN_MOSI = D23;

const PIN_CS = D5;                               // NSS
const PIN_RESET = D21;
const IRQ_PIN = D22;

// *** Setup SPI

let spi = SPI2;
spi.setup({
    sck: PIN_SCK,
    miso: PIN_MISO,
    mosi: PIN_MOSI,
    baud: 600000,
});

// *** Setup SX127x

// Configuration
const txConfig = {
    bandwidth: 0,
    freq: 434330000,
    power: 17,
    forcePaBoost: true,
};

const sx = require("SX127x").connect({ spi: SPI2, cs: PIN_CS, rst: PIN_RESET });

// Poll DIO0 for interrupts
setInterval(function () { sx.onIRQ(); }, 100);
sx.setTxConfig(txConfig);
// Transmit
sx.send("Hello", function () {
    console.log("TX done");
});

I don't know if it would also have been there before, since HW SPI was not working then.

@MaBecker
Copy link
Contributor

MaBecker commented Dec 5, 2020 •
edited

Try this build http://www.espruino.com/binaries/travis/master/?C=M;O=D

Hmm just checked, there is no fw for ESP32.

@rujorgensen
Copy link

Try this build http://www.espruino.com/binaries/travis/master/?C=M;O=D

Hmm just checked, there is no fw for ESP32.

And aren't they all built before my comment too 🙂? I could pull something into my local repo, but I don't see anything new pushed.

MaBecker added a commit that referenced this issue Dec 6, 2020
@MaBecker
Update SPI (fix #1963)
6c18ec8 
* add callback for cout == 1
MaBecker added a commit that referenced this issue Dec 6, 2020
@MaBecker
Update SPI (fix #1963)
689a48d 
* fix build error
@MaBecker
Copy link
Contributor

MaBecker commented Dec 6, 2020

Ok, fixed the build error, now there is a ESP32 firmware created by travis.

gfwilliams added a commit that referenced this issue Dec 7, 2020
@gfwilliams
Merge pull request #1965 from espruino/esp32_improvement
6b19a02 
Update SPI (fix #1963)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MaBecker MaBecker

Labels
ESP32 This is only a problem on ESP32-based devices
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gfwilliams @rujorgensen @MaBecker