espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.68k stars 7.42k forks source link

Array filling causes "random" to misfire and can't step by 1 address #5158

Closed MikeyMoMo closed 3 years ago

MikeyMoMo commented 3 years ago

On ESP32, with or with PSRAM, I allocate an array. I need 76,800 16 bit fields to contain color information. I can, finally, allocate it, write to it and read back the same stuff I stuffed into it. That took a week of messing around. Very tricky! Then, when I folded that back into the program that needed it, suddenly random() went from taking microseconds to return a number to almost 18 seconds per call. This happens when i write to element #63727 in the array. Before that, no problem. After that, it works but takes way too long! Full program, below, but some discussion, first.

I am allocating the array, thus:

int displayPixelCt = TFT_HEIGHT TFT_WIDTH; // Display size. unsigned short workBuff; int pixelCount = displayPixelCt; // Got that way during debugging...

I do an malloc, thus:

workBuff = (unsigned short *)malloc(pixelCount); // Trying to allocate 76800 16 bit "words".

That gives this in the monitor:

00:02:36.333 -> Array should have 76800 16-bit integers. 00:02:36.333 -> FreeHeapSize before malloc 290768 00:02:36.333 -> Array workBuff located at 0x3ffc1004 - 0x3ffe6804 00:02:36.333 -> FreeHeapSize before malloc 290768, after 213952, 00:02:36.333 -> used: 76816

Reason for unsigned short, later.

Then I fill and check the array with two routines pointing at workBuff.

FillArray(workBuff);
CheckArray(workBuff);

I am calling these with higher and higher ending element to set and check. When random takes over a second, I stop. Here's the ending lines from the monitor:

00:25:43.924 -> Array size to load: 63725 00:25:43.924 -> 34 It took 0 ms to get that random number. 00:25:43.924 -> Filling array 00:25:43.924 -> Array size to load: 63726 00:25:43.959 -> 49 It took 0 ms to get that random number. 00:25:43.959 -> Filling array 00:25:43.959 -> Array size to load: 63727 00:26:01.829 -> 400 It took 17884 ms to get that random number. 00:26:01.829 -> It took 17.88 SECONDS to get that random number! WRONG!! 00:26:01.829 -> Failed with 63727 elements loaded 00:26:01.829 -> Last element at address 0x3ffe01e2

Now, the other problem is the addressing. I am using this format of statement to select the address to store the data into:

  for (i = 0; i < arrayWorkingSize; i++) {
    Serial.printf("Setting location %i at address %p to %i\n",
                    i, array + i, i % 65535);
    array[i] = i % 0xFFFF;  // 16 bits can only hold that much.

That should be stepping by 16 bits each time. But, it seems that it is stepping much more than that. Here's what I see from the address printf:

00:02:56.193 -> Setting location 0 at address 0x3ffc1004 to 0 00:02:56.193 -> Setting location 1 at address 0x3ffc1006 to 1 00:02:56.193 -> Setting location 2 at address 0x3ffc1008 to 2 00:02:56.193 -> Setting location 3 at address 0x3ffc100a to 3 00:02:56.193 -> Setting location 4 at address 0x3ffc100c to 4 00:02:56.193 -> Setting location 5 at address 0x3ffc100e to 5

That's stepping by 2, not 1 and that 2 seems to be two 32 bit "words" (from another thread I have read). That's about 4 times what I need. That's why the unsigned short declaration, above. I am trying to get it to store in successive, 16-bit memory addresses, but can't.

I think this is all connected and if any part of it is cracked, it will lead to a solution for the whole thing. I was a week getting the array to load up all the way and, not that that is working, it screws up other stuff.

Please help me untangle this!! It is making me crazy. I started in 1965 with Fortran, quickly moved to Assembler, have written in about 12 languages and taught several but this is the most confusing thing I have ever run across. MANY THANKS for anyone who can untangle this mess! I will post the entire program, now. Just load it into Arduino IDE and onto any ESP32. I have tried it on 3 different boards. All fail the same way.

Mike

//This now works!
#define TFT_WIDTH 320
#define TFT_HEIGHT 240
int displayPixelCt = TFT_HEIGHT * TFT_WIDTH;
int preHeapSize;
long int i;
unsigned long startMillis;
unsigned short *workBuff;
int pixelCount = displayPixelCt;
int arrayWorkingSize = displayPixelCt / 2;
/***************************************************************************/
void setup()
/***************************************************************************/
{
  Serial.begin(500000); delay(1000); Serial.println("\n");
  showSizes();
  Serial.printf("Array should have %i 16-bit integers.\n", displayPixelCt);
  preHeapSize = xPortGetFreeHeapSize();
  Serial.printf("FreeHeapSize before malloc %i\n", preHeapSize);

  workBuff = (unsigned short *)malloc(pixelCount);

  //************************
  //The following is totally correct!  Don't change anything or delete it!
  //  workBuff = (int*)ps_malloc(displayPixelCt * sizeof(uint16_t));
  //************************
  if (!workBuff) {
    Serial.println("Allocation of workBuff failed");
    return;
  }
  Serial.printf("Array workBuff  located at %p - %p\n", workBuff, workBuff + displayPixelCt);
  //  Serial.printf("Array workBuff2 located at %p - %p\n", workBuff2, workBuff2 + displayPixelCt);
  Serial.printf("FreeHeapSize before malloc %i, after %i, \nused: %i\n",
                preHeapSize, xPortGetFreeHeapSize(),
                preHeapSize - xPortGetFreeHeapSize());
  for (arrayWorkingSize = 63700;
       arrayWorkingSize < pixelCount;
       arrayWorkingSize++) {
    FillArray(workBuff);
    CheckArray(workBuff);
    startMillis = millis();
    Serial.print(random(500));
    Serial.printf(" It took %.0f ms to get that random number.\n",
                  (float)(millis() - startMillis));
    if ((millis() - startMillis) > 1000) {
      Serial.printf("It took %.2f SECONDS to get that random number! WRONG!!\n",
                    (float)(millis() - startMillis) / 1000.);
      Serial.printf("Failed with %i elements loaded\n", arrayWorkingSize);
      Serial.printf("Last element at address %p\n", workBuff + arrayWorkingSize);
      while (1);
    }
  }
}
/***************************************************************************/
void FillArray (unsigned short * array)
/***************************************************************************/
{
  //  Serial.println("/***************************************************************************/");
  //  Serial.println("/***************************************************************************/");
  Serial.printf("Filling array\n");
  //  Serial.println("/***************************************************************************/");
  //  Serial.println("/***************************************************************************/");
  Serial.printf("Array size to load: %i\n", arrayWorkingSize);
  for (i = 0; i < arrayWorkingSize; i++) {
    if (i < 500 && arrayWorkingSize == 63700)
      Serial.printf("Setting location %i at address %p to %i\n",
                    i, array + i, i % 65535);

    array[i] = i % 0xFFFF;  // 16 bits can only hold that much.

    //    if (i > displayPixelCt - 500 || i < 500) {
    //      Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
    //  }
  }
  //  i = 0;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt / 2;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 3;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 2;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 1;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
}

/***************************************************************************/
void CheckArray (unsigned short * array)
/***************************************************************************/
{
  //  Serial.println("/***************************************************************************/");
  //  Serial.println("/***************************************************************************/");
  //  Serial.printf("Checking array\n");
  //  Serial.println("/***************************************************************************/");
  //  Serial.println("/***************************************************************************/");
  //  Serial.printf("Array size to check: %i\n", arrayWorkingSize);
  for (i = 0; i < arrayWorkingSize; i++) {
    if (i > displayPixelCt - 500)
      //      Serial.printf("Verify location %i contains %i\n", i, array[i]); // Just checking...
      if (array[i] != i % 65535) {
        Serial.printf("Location %i mismatch at address %p\n", i, array + i);
        Serial.printf("Should be %i, contains %i\n", i % 65535, array[i]);
        while (1);
      }
  }
  //  i = 0;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  //  for (i = displayPixelCt / 2 - 10; i < displayPixelCt / 2 + 10; i++)
  //  //    Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 3;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 2;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
  //  i = displayPixelCt - 1;
  //  Serial.printf("Location %i contains %i\n", i, array[i]); // Just checking...
}
void loop() { }
void showSizes() {
  Serial.printf("              bool %i\n", sizeof(bool));
  Serial.printf("           boolean %i\n", sizeof(boolean));
  Serial.printf("              byte %i\n", sizeof(byte));
  Serial.printf("              char %i\n", sizeof(char));
  Serial.printf("     unsigned char %i\n", sizeof(unsigned char));
  Serial.printf("           uint8_t %i\n", sizeof(uint8_t));

  Serial.printf("             short %i\n", sizeof(short));
  Serial.printf("          uint16_t %i\n", sizeof(uint16_t));
  Serial.printf("              word %i\n", sizeof(word));

  Serial.printf("               int %i\n", sizeof(int));
  Serial.printf("      unsigned int %i\n", sizeof(unsigned int));
  Serial.printf("            size_t %i\n", sizeof(size_t));

  Serial.printf("             float %i\n", sizeof(float));
  Serial.printf("              long %i\n", sizeof(long));
  Serial.printf("     unsigned long %i\n", sizeof(unsigned long));
  Serial.printf("          uint32_t %i\n", sizeof(uint32_t));

  Serial.printf("            double %i\n", sizeof(double));

  Serial.printf("         long long %i\n", sizeof(long long));
  Serial.printf("unsigned long long %i\n", sizeof(unsigned long long));
  Serial.printf("          uint64_t %i\n", sizeof(uint64_t));
}
lbernstone commented 3 years ago

This has nothing to do with arduino, so I would recommend you ask at esp-idf. Read the documentation for heap memory. You can allocate in either 8-bit or 32-bit increments. The addresses are certainly listed as bytes. Just like here, esp-idf is going to be interested in you following the issue template...

MikeyMoMo commented 3 years ago

Wow! I gave every bit of information to recreate the problem but you want structure or you can't read it? How odd... Like the ones that scream INDENT!!! INDENT!!!! Can't read if it not indented... Sad... I have been trying to get answers from ESP for almost 2 weeks and they have tried but can't tell me why this strange behavior happens and don't seem capable of escalating it. They created the thing and can't make it work. One of their solutions was broken as sent to me. They did not even test it! So they are no use, either. Maybe I can shoehorn 2 16 bit values into each 32 bit of memory now that I know they are unable to allocate 16 bit RAM. 8 and 32 but no 16. That is so strange! This whole thing is strange. It does not follow much logic. It runs without error but returns garbage. After fixing that, it runs without error but affects program execution in a very bad way. Looks like there are no answers! Have to try to fix it myself with all the big guns remaining silent. This is so broken!

h2zero commented 3 years ago

I'll take the bait. The code works fine, I only changed long int i; to int i; to avoid compilation errors.

Results: No occurrence of slow rand performance.

Notes:

workBuff = (unsigned short *)malloc(pixelCount); // Trying to allocate 76800 16 bit "words".

That won't allocate what you intended, instead you're getting 76800 bytes (need a * 2 here), but that fails because you cannot allocate that much on the ESP32. It may be possible to allocate 2 76800 byte buffers separately however (untested).

That should be stepping by 16 bits each time. But, it seems that it is stepping much more than that. Here's what I see from the address printf: 00:02:56.193 -> Setting location 0 at address 0x3ffc1004 to 0 00:02:56.193 -> Setting location 1 at address 0x3ffc1006 to 1 00:02:56.193 -> Setting location 2 at address 0x3ffc1008 to 2 00:02:56.193 -> Setting location 3 at address 0x3ffc100a to 3 00:02:56.193 -> Setting location 4 at address 0x3ffc100c to 4 00:02:56.193 -> Setting location 5 at address 0x3ffc100e to 5

The addresses are 8 bit, looks to be working fine.

MikeyMoMo commented 3 years ago

Wow! I gave every bit of information to recreate the problem but you want structure or you can't read it? How odd... Like the ones that scream INDENT!!! INDENT!!!! Can't read if it not indented... Sad... I have been trying to get answers from ESP for almost 2 weeks and they have tried but can't tell me why this strange behavior happens and don't seem capable of escalating it. They created the thing and can't make it work. One of their solutions was broken as sent to me. They did not even test it! So they are no use, either. Maybe I can shoehorn 2 16 bit values into each 32 bit of memory now that I know they are unable to allocate 16 bit RAM. 8 and 32 but no 16. That is so strange! This whole thing is strange. It does not follow much logic. It runs without error but returns garbage. After fixing that, it runs without error but affects program execution in a very bad way. Looks like there are no answers! Have to try to fix it myself with all the big guns remaining silent. This is so broken!

MikeyMoMo commented 3 years ago

The esp-idf is IDF, this is ARDUINO. Not sure why you don't get the difference! I am not using IDF. The link posted is for ESP-IDF.

The allocation is already 32 bit, not 8 bit. 16 bit is, for some silly reason, not available. It is the logical thing to be there but they don't give it. I am already over allocating for what I need because of this. So, if I allocate 1/2 of the 78600, I can put in 16 bits, shift left then put in the other 16 bits to store two color values in each 32 bit allocation. This is a hassle and it means that I cannot use the pixel push for the entire array. Instead, I must unpack each 32 bits into two 16 bit values and make up another single row array, put that on the screen and repeat for screen height. It slows it down but, maybe not enough to be visible. Just a pain because I have to call pixel push 240 times instead of once because of two reasons. ESP did not provide a build buffer for the screen like "good" builders of hardware do and they don't have a 16 bit allocation available from malloc, just 8 and 32. Don't know why they left this out.

There were no compilation errors. No idea why you mentioned that, and you don't say why. Only the slowdown after I load up that one array element. The allocation is fine. I CAN get that much memory. I DON'T need to multiply by 2! No idea why that get mentioned by so many people. The allocation is actually twice as large as I want, already, because of the 32 bit allocation. Why would I multiply by 2 AGAIN??? Yes, that is too much of an allocation. And 4 times what I need!

So I copied out the code I posted, above and made the one change mentioned (removed the "long") and am trying it. NO compile errors! Since you did not mention what the compile error is, I can't help there. And, with that one change, I get the long random just after loading element 63,700. It is odd that memset will load values into this array without problem. I cannot find source code for memset so I can't tell what the difference is. Here's the latest after making that one change to the long int definition.

13:41:00.599 -> Setting location 499 at address 0x3ffc149a to 499 13:41:17.039 -> 461 It took 16453 ms to get that random number. 13:41:17.039 -> It took 16.45 SECONDS to get that random number! WRONG!! 13:41:17.039 -> Failed with 63700 elements loaded 13:41:17.039 -> Last element at address 0x3ffe025c

I don't get it. You say it works this way. I have tested it on 3 different type boards and it fails the same way on all 3. The code is good. It works perfectly fine on the unit with PSRAM when I use PSRAM but has the long random if PSRAM if not used. It is that simple. I have reinstalled the Arduino IDE. I have refreshed the libraries. What else is there to do?!?!?! 3rd week trying to shoot this one bug. Very irritated!!

TD-er commented 3 years ago

I do realize you probably started adding debug code after you found out things were way too slow. But I would just like to point out that sending lots of data to the serial port may also skew your observations. Not 100% sure if the ESP32 behaves the same, but on ESP8266, if your serial buffer is full, writing to it may become blocking until all data is flushed. Just keep that in mind when debugging like this.

On the random issue... If you have the WiFi/Bluetooth off, it will use a pseudo random number generator. So can you reproduce this also with WiFi enabled (not sure if you actually need to connect to something) ? If not, then I guess you may have found some issue with the (pseudo) random generator with this specific range of 500 and this specific calls. So another test could be to not impose a range, but rather apply your own range:

unsigned int myRandom(unsigned int range) {
  unsigned int ret = random();
  ret = ret % range; // modulo
  return ret;
}
MikeyMoMo commented 3 years ago

Serial port has nothing to do with it. While waiting for that 18 seconds, there is no serial port traffic. The actual problem is the array processing with malloc and memset.


From: TD-er @.> Sent: Monday, May 10, 2021 5:33 PM To: espressif/arduino-esp32 @.> Cc: Mike Morrow @.>; Author @.> Subject: Re: [espressif/arduino-esp32] Array filling causes "random" to misfire and can't step by 1 address (#5158)

I do realize you probably started adding debug code after you found out things were way too slow. But I would just like to point out that sending lots of data to the serial port may also skew your observations. Not 100% sure if the ESP32 behaves the same, but on ESP8266, if your serial buffer is full, writing to it may become blocking until all data is flushed. Just keep that in mind when debugging like this.

On the random issue... If you have the WiFi/Bluetooth off, it will use a pseudo random number generator. So can you reproduce this also with WiFi enabled (not sure if you actually need to connect to something) ? If not, then I guess you may have found some issue with the (pseudo) random generator with this specific range of 500 and this specific calls. So another test could be to not impose a range, but rather apply your own range:

unsigned int myRandom(unsigned int range) { unsigned int ret = random(); ret = ret % range; // modulo return ret; }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/espressif/arduino-esp32/issues/5158#issuecomment-836466194, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYPSIIS7W32YYKXOBRSPALTM6R5JANCNFSM44HJAIXA.

stale[bot] commented 3 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 3 years ago

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.