These are chat archives for Makuna/NeoPixelBus

20th
Aug 2018
Xonotron
@Xonotron
Aug 20 2018 12:08

Ok I found something.

I reduced the code even more, I wanted to check if using the millis() function could possibly be the culprit =D

#include <NeoPixelBus.h>

NeoPixelBus<NeoGrbFeature, Neo800KbpsMethod> strip(1, 4);

int brightness=32;
RgbColor yellow(brightness, brightness/4*3, 0);
RgbColor blue(0, 0, brightness);

bool isHigh=true;

void setup() {
  pinMode(12, INPUT_PULLUP);

  pinMode(2, OUTPUT);
  digitalWrite(2, LOW);

  strip.Begin();
  strip.Show();
}

void loop() {
  if(digitalRead(12)==LOW && isHigh) {
    isHigh=false;

    //strip.Begin();
    strip.SetPixelColor(0, yellow);
    strip.Show();

    digitalWrite(2, HIGH);
  }

  else if(digitalRead(12)==HIGH && !isHigh) {
    isHigh=true;

    strip.SetPixelColor(0, blue);
    strip.Show();

    digitalWrite(2, LOW);
  }
}

To prevent issues introduced by using a button I used another ESP32 to trigger input pin 12. Also using an on/off frequency of 10Hz makes it faster to check for errors. I used this simple sketch:

void setup() {
  pinMode(25, OUTPUT);
  digitalWrite(25, LOW);
  delay(1000);
}

void loop() {
  digitalWrite(25, HIGH);
  delay(50);
  digitalWrite(25, LOW);
  delay(50);
}

A rummaged a little bit around the i2s code (Esp32_i2s.c) and found out that the error rate is proportional to the size of I2S_DMA_SILENCE_LEN. Smaller values e.g. 32 makes it far worse whereas higher values makes it more stable. Setting it to 4092 results in very sporadic errors every 20-30 seconds at 10 Hz.

Setting I2S_DMA_SILENCE_LEN this high makes it a little bit slower but faster than using Begin() each time.

Perhaps that helps in finding the root cause.

Me No Dev
@me-no-dev
Aug 20 2018 16:16
I have an idea
Me No Dev
@me-no-dev
Aug 20 2018 16:21
first silence len should be maybe 256, then the buffer that you send to I2S should have a minimum length of around what the silence buffer length is
you can fill it with zeroes
maybe tweak the minimum buffer size until you reach stable condition
when the buffer is too small, the ISR fires too often for the software to handle and this causes issues
I should compare with my current code and see if I have improved anything over what @Makuna has here and maybe PR some changes
Me No Dev
@me-no-dev
Aug 20 2018 16:34
maybe this:
void IRAM_ATTR i2sDmaISR(void* arg)
{
    i2s_dma_item_t* dummy = NULL;
    i2s_bus_t* dev = (i2s_bus_t*)arg;
    portBASE_TYPE hpTaskAwoken = 0;

    if (dev->bus->int_st.out_eof) {
        i2s_dma_item_t* item = (i2s_dma_item_t*)dev->bus->out_eof_des_addr;
        item->data = dev->silence_buf;
        item->blocksize = dev->silence_len;
        item->datalen = dev->silence_len;
        if (xQueueIsQueueFullFromISR(dev->tx_queue) == pdTRUE) {
            xQueueReceiveFromISR(dev->tx_queue, &dummy, &hpTaskAwoken);
        }
        xQueueSendFromISR(dev->tx_queue, (void*)&item, &hpTaskAwoken);
    }
    dev->bus->int_clr.val = dev->bus->int_st.val;
    if (hpTaskAwoken == pdTRUE) {
        portYIELD_FROM_ISR();
    }
}
Xonotron
@Xonotron
Aug 20 2018 18:40

Thanks for chiming in, me-no-dev!

I replaced i2sDmaISR with the one you provided, but it didn't change anything.
Unfortunately the stuff you wrote about the minimum buffer size is way beyond my skill level, I have no clue where to start. I'm barely understanding what's going on inside your code.
But perhaps Makuna knows how to implement this?

I'm wondering why this hasn't been discovered before. Is it because NeoPixelBus is mainly used for animation where this would result only in a "dropped frame" and wouldn't stand out very much.?

Me No Dev
@me-no-dev
Aug 20 2018 19:29
it has not been discovered because I have tested only with 32 and more pixels :D was solving an issue where many pixels can not be updated and never tested just one
_i2sBufferSize should have a minimum size there
so you can try below that line:
if(_i2sBufferSize < 512){//128 samples
  _i2sBufferSize = 512;
}
then play with that number and the silence len to get minimum stable values
Xonotron
@Xonotron
Aug 20 2018 19:58

I inserted that after _i2sBufferSize = pixelCount * dmaPixelSize + resetSize; in NeoEsp32I2sMethod.h but nothing changes.

It behaves exactly the same if I set the pixel count to 64, 128 or even 256.

Me No Dev
@me-no-dev
Aug 20 2018 19:59
now that is surprising...
can you please pick a gpio, set it as output+low and add to beginning and end of the DMA ISR digitalWrite high and low, then scope that together with the pixel pin. you can trigger on the pixel pin
Xonotron
@Xonotron
Aug 20 2018 20:04
ah I see, yes I can do that
Me No Dev
@me-no-dev
Aug 20 2018 20:05
you can use logic analizer if you want also. it does not matter :)
change 2 to 3
Me No Dev
@me-no-dev
Aug 20 2018 20:10
or even 4 and use 64 as silence buf
Xonotron
@Xonotron
Aug 20 2018 20:14
it stops working at all if I set the dma_count to 3 or 4
Me No Dev
@me-no-dev
Aug 20 2018 20:14
really?
i downloaded the lib and will test it out but am running a different test ATM so after that
Xonotron
@Xonotron
Aug 20 2018 20:18
isr_01.jpg
the logic analyzer gives me this
the bigger gaps are 1.28ms, the normal ones 0.64ms
great you can test it, much appreciated!
Xonotron
@Xonotron
Aug 20 2018 20:24
isr_02.jpg
Me No Dev
@me-no-dev
Aug 20 2018 20:26
what is what in the images?
Xonotron
@Xonotron
Aug 20 2018 20:26

I just realized that it doesn't make much sense without the other two channels ':D

Channel one is the ISR, ch2 the input trigger and ch3 the output to the pixel

I took a snipped where one pixel change has been skipped
Me No Dev
@me-no-dev
Aug 20 2018 20:26
zoom in more on where ch3 is
so you catch a few ch1 also
ch2 i do not need :)
Xonotron
@Xonotron
Aug 20 2018 20:28
isr_03.jpg
When a colour change is being skipped, there is no output at all
isr_04.jpg
Oh look here. The skip happens when the ISR takes more time.
Xonotron
@Xonotron
Aug 20 2018 20:34
Ah no... too fast, this is just an aliasing issue...
But I can confirm, that the skip occurs each time the input is very near to the ISR
Xonotron
@Xonotron
Aug 20 2018 20:39
If it's shortly after the ISR it works, but if the trigger is very close before the ISR starts it seems to be ignored.
Me No Dev
@me-no-dev
Aug 20 2018 20:50
hold on
Xonotron
@Xonotron
Aug 20 2018 20:53
=)
Me No Dev
@me-no-dev
Aug 20 2018 21:36
oh i found it :D
Xonotron
@Xonotron
Aug 20 2018 21:37
oh I'm so eager to know!
Me No Dev
@me-no-dev
Aug 20 2018 21:38
it's a bug that comes out because of the timing of i2s interrupts and such
dma buffers need to be set to 4
and in i2sInitDmaItems
the queue size should substract 3 (in this case will result to 1)
what buffer is playing, what is loaded and what is returned in ISR are messed up in the timing
so you get the buffer that is already playing and there you go
i obviously have this fixed at a later point
one more change in the i2sWrite method
Me No Dev
@me-no-dev
Aug 20 2018 21:44
here are the optimized isr and write methods:
void IRAM_ATTR i2sDmaISR(void* arg)
{
    i2s_dma_item_t* dummy = NULL;
    i2s_bus_t* dev = (i2s_bus_t*)arg;
    portBASE_TYPE hpTaskAwoken = 0;

    if (dev->bus->int_st.out_eof) {
        i2s_dma_item_t* item = (i2s_dma_item_t*)dev->bus->out_eof_des_addr;
        item->data = dev->silence_buf;
        item->blocksize = dev->silence_len;
        item->datalen = dev->silence_len;
        if (xQueueIsQueueFullFromISR(dev->tx_queue) == pdTRUE) {
            xQueueReceiveFromISR(dev->tx_queue, &dummy, &hpTaskAwoken);
        }
        xQueueSendFromISR(dev->tx_queue, (void*)&item, &hpTaskAwoken);
    }
    dev->bus->int_clr.val = dev->bus->int_st.val;
    if (hpTaskAwoken == pdTRUE) {
        portYIELD_FROM_ISR();
    }
}

size_t i2sWrite(uint8_t bus_num, uint8_t* data, size_t len, bool copy, bool free_when_sent) {
    if (bus_num > 1 || !I2S[bus_num].tx_queue) {
        return 0;
    }
    size_t index = 0;
    size_t toSend = len;
    size_t limit = I2S_DMA_MAX_DATA_LEN;
    i2s_dma_item_t* item = NULL;

    while (len) {
        toSend = len;
        if (toSend > limit) {
            toSend = limit;
        }

        if (xQueueReceive(I2S[bus_num].tx_queue, &item, portMAX_DELAY) == pdFALSE) {
            log_e("xQueueReceive failed\n");
            break;
        }
        // data is constant. no need to copy
        item->data = data+index;
        item->blocksize = toSend;
        item->datalen = toSend;

        len -= toSend;
        index += toSend;
    }
    return index;
}
Xonotron
@Xonotron
Aug 20 2018 21:44
>the queue size should substract 3 
you mean like this?
I2S[bus_num].tx_queue = xQueueCreate(I2S[bus_num].dma_count-3, sizeof(i2s_dma_item_t*));
Me No Dev
@me-no-dev
Aug 20 2018 21:44
yes
Xonotron
@Xonotron
Aug 20 2018 21:44
IT WORKS :D
Me No Dev
@me-no-dev
Aug 20 2018 21:45
i know :D
Xonotron
@Xonotron
Aug 20 2018 21:45
awesome :)
Me No Dev
@me-no-dev
Aug 20 2018 21:45
it took me more than a week to track this issue
@Makuna apply the above and you no longer have a limitation of 4 or more pixels
you can try to lower the silence too, but that will result in ISR firing more often
Xonotron
@Xonotron
Aug 20 2018 21:49
i2sWrite is much shorter now
Me No Dev
@me-no-dev
Aug 20 2018 21:49
which means that pixels will be pushed sooner, but can also mean gaps if you are pushin lots of pixels
@Xonotron that i2sWrite that was there can do more things but they are irrelevant here
Xonotron
@Xonotron
Aug 20 2018 21:49
I admire this level of skill :)
Me No Dev
@me-no-dev
Aug 20 2018 21:50
was skill :D now it's work
Xonotron
@Xonotron
Aug 20 2018 22:07
the if(_i2sBufferSize < 512){//128 samples _i2sBufferSize = 512; } part is not needed anymore?
it's faster without it and seems to work fine...
Me No Dev
@me-no-dev
Aug 20 2018 22:08
not needed
even just the dma items to be fixed should be enough
the optimized methods could just further ensure least time spent in ISR and write
Xonotron
@Xonotron
Aug 20 2018 22:10
I did a performance comparison. a loop of 1000x strip.SetPixelColor(0, blue); strip.Show(); took about 80ms before and now around 500ms with the 4 dma buffers. that's not an issue, too?
Xonotron
@Xonotron
Aug 20 2018 22:16
Ah... and > it took me more than a week to track this issue
Do you have a time machine? I stumbled upon this only two days ago. Was it perfect timting? :)
Me No Dev
@me-no-dev
Aug 20 2018 22:20
no, i just stumbled on the same thing after i gave the code to @Makuna
Xonotron
@Xonotron
Aug 20 2018 22:21
=)