String Vs char array speed issues

Projects Programming
1

Hi all,

I'm experiencing an unexpected slow response when converting an old String-based function to a char array.

Years ago I wrote a function to scan a keyboard matrix of 160 buttons. It was based on a String vector which was being populated with the state of each button (i.e. "0000100001111.....").
At each cycle, the old string would be compared to the new one and if a button was pressed (value 0) or released (value 1) the microcontroller would send the code (XabcZ) where abc is the number of the button whose status had changed and Z is the status (0 or 1).

As I learned afterwards (I'm not a native programmer) using such a massive amount of strings was consuming the memory and this explains why the microcontroller would eventually hang in a couple of hours, then I decided to switch to char arrays.

The new algorithm works but it's surprisingly so slow than the micro takes now seconds to execute the other functions while with the previous algorithm everything was working almost in real-time. I tested this with my Arduino Due but also with Arduino Uno (using less buttons due to reduced number of DIN/DOUT) and different versions of Teensy. All of them behave in the same way.

For the sake of clarity, other functions (which have not been changed) are related to a MAX7219 led displays matrix whose input is received through the serial port.

Here below I paste the two versions, the older one with Strings and the newer one with char arrays.

Any help or clarification no what I'm doing wrong would be much appreciated.

Old Version:

[code]
#define activerows 10
#define columns 16
byte rows_pin[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
byte columns_pin[] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};
String newState, oldState, newPinState, oldPinState, out;
int mapping[] = {1, 3, 6, 11, 13, 15, 17, 52, 55, 65, 63, 61, 59, 71, 87, 85, 83, 116, 123, 99, 151, 145, 142, 140};

void scan_buttons_old() {
  newState = "";
  for (int j = 0; j < activerows; j++) {
    for (int i = 0; i < columns; i++) {
      //update_encoder();
      digitalWrite(columns_pin[i], LOW);
      newPinState = String(digitalRead(rows_pin[j]));
      digitalWrite(columns_pin[i], HIGH);
      oldPinState = String(oldState.charAt(i + j * columns));
      if (newPinState != oldPinState) {
        if (newPinState == "1") {
          for (int k = 0; k < (sizeof(mapping) / sizeof(int)); k++) {
            if (((i + j * columns) == mapping[k]) && (oldState.charAt(-1 + i + j * columns) != "0")  && ((i + j * columns) != 0)) {
              out = "X";
              out += (-1 + i + j * columns);
              out += String(oldState.charAt(-1 + i + j * columns));
              out += ":";
              Serial.println(out);
              skip_routine = true;
              break;
            }
          }
        }
        if (!skip_routine) {
          out = "X";
          out += (i + j * columns);
          out += newPinState;
          out += ":";
          Serial.println(out);
        }
      }
      newState += newPinState;
      skip_routine = false;
    }
  }
  oldState = newState;
}

[/code]

and the new version...

#define String_State_Size 161
#define activerows 10
#define columns 16
byte rows_pin[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
byte columns_pin[] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};
bool stringnewstate[String_State_Size], stringoldstate[String_State_Size];
bool pinStateSTR, oldpinStateSTR;
char strout[6];
int mapping[] = {1, 3, 6, 11, 13, 15, 17, 52, 55, 65, 63, 61, 59, 71, 87, 85, 83, 116, 123, 99, 151, 145, 142, 140};

void scan_buttons() {
  for (int j = 0; j < activerows; j++) {
    for (int i = 0; i < columns; i++) {
      digitalWrite(columns_pin[i], LOW);
      pinStateSTR = digitalRead(rows_pin[j]);
      digitalWrite(columns_pin[i], HIGH);
      oldpinStateSTR = stringoldstate[i + j * columns];
      if (pinStateSTR != oldpinStateSTR) {
        if (pinStateSTR == 1) {
          for (int k = 0; k < (sizeof(mapping) / sizeof(int)); k++) {
            if (((i + j * columns) == mapping[k]) && ((i + j * columns) != 0) && (stringnewstate[-1 + i + j * columns] != 0)) {
              strcpy(strout, "");
              strcat(strout, "X");
              strcat(strout, (char *)integerToString(-1 + i + j * columns));
              strcat(strout, (char *)integerToString(stringnewstate[-1 + i + j * columns]));
              strcat(strout, ":");
              Serial.println(strout);
              skip_routine = true;
              break;
            }
          }
        }
        if (!skip_routine) {
          strcpy(strout, "");
          strcat(strout, "X");
          strcat(strout, (char *)integerToString(i + j * columns));
          strcat(strout, (char *)integerToString(pinStateSTR));
          strcat(strout, ":");
          Serial.println(strout);
        }
      }
      stringnewstate[i + j * columns] = pinStateSTR;
      skip_routine = false;
    }
  }

  for (int i = 0; i < String_State_Size; i++) {
    stringoldstate[i] = stringnewstate[i];
  }
}

If I call scan_buttons() in the loop() then the response is really slow. If I call scan_buttons_old() it's fast.

Thanks everyone for the support.

Best Regards

2

Please post your entire sketch. The problem may be in how you call the two functions.

3

The overhead memory required for an object of the String class is only 6 bytes (on an Uno anyway). So, the amount of memory you're wasting doesn't change that much between using String objects, char arrays, or bool arrays. In all cases your scheme is inefficient because you're using a whole byte (8 bits) to store 1 bit of information.

The information you're now storing in 160 bytes (where did 161 come from?) could be stored in 20 bytes by using 1 bit for each button.

4

Ok, I will post the entire sketch (I need to clear some old parts which are commented and to format it correctly).

Meanwhile, regarding the bytes and the bits.

Yes, understood. How can I switch to a bit array? I thought boolean would do so but I was clearly wrong.
The 161 comes from 160 characters (16 x 10) + a NULL termination character. Is that also wrong?

By the way, with “consuming memory” I was meaning heap fragmentation with the strings which I learned about through the following link:

5

Here below the entire sketch as requested.

I call the function in the loop() (the scan_buttons() is commented and the scan_buttons_old() is currently being used).

If you just comment the _old function and activate the new one the LEDs being turned on by the check_serial() function will take forever to light. For 64 leds it can take up to 10 seconds while when I activate the _old function the check_serial() is activating the same number of LEDs in less than one second.

OVH_Sketch_for_forum.ino (12.8 KB)

6

Not sure of the exact reason why it's so slow but it doesn't matter, just do it using bits anyway. You have 16 columns, that will fit an unsigned int perfectly in bits. You #defined column and row size so you would begin by defining your state tables:

#define activerows 10
#define columns 16

// #define String_State_Size 161 No, don't hard code it
#define String_State_Size activerows * columns

unsigned int newstate[activerows];
unsigned int  oldstate[activerows];

The rest of it would use bit() and intelligent indexing to access those state tables. I didn't dig to find out why you are double-buffering the states. Usually you only need one memory buffer for key states.

It would look something like:

    oldpinState = bitRead( j, oldstate[i] );

You make the common canard of concatenating for serial output. It is not necessary, you can simply consecutively write things to output:

        strcpy(strout, "");
          strcat(strout, "X");
          strcat(strout, (char *)integerToString(i + j * columns));
          strcat(strout, (char *)integerToString(pinStateSTR));
          strcat(strout, ":");
          Serial.println(strout);

Serial.print(strout, "X");
          Serial.print((char *)integerToString(i + j * columns));
          Serial.print((char *)integerToString(pinStateSTR));
          Serial.print":");
          Serial.println(strout);
7

Serial.print() can do integers and single characters, so more like:

    Serial.print('X');
    Serial.print(-1 + i + j * columns);
    Serial.print(stringnewstate[-1 + i + j * columns]);
    Serial.println(':');
8

poweruser77:
Yes, understood. How can I switch to a bit array? I thought boolean would do so but I was clearly wrong.

Here's how I might create an array using one bit per button and then use functions to write and read the bits:

constexpr size_t numRows = 10;
constexpr size_t numCols = 16;
constexpr size_t numButtons = numRows * numCols;
constexpr size_t numArrayElements = (numButtons >> 3) + (((numButtons & 0x3) > 0) ? 1 : 0);

uint8_t byteArray[numArrayElements];

bool setBitValue(size_t row, size_t column, uint8_t value);
uint8_t getBitValue(size_t row, size_t column);

void setup() {
  Serial.begin(115200);
  delay(1000);

  Serial.println(getBitValue(9, 13));
  setBitValue(9, 13, 1);
  Serial.println(getBitValue(9, 13));
  setBitValue(9, 13, 0);
  Serial.println(getBitValue(9, 13));
}

void loop() {
}

bool setBitValue(size_t row, size_t column, uint8_t value) {
  size_t bitIndex = row * numCols + column;
  size_t byteIndex = bitIndex >> 3;

  if (byteIndex >= numArrayElements) {
    return false;
  }

  uint8_t bitPosition = bitIndex - (byteIndex << 3);

  if (value > 0) {
    byteArray[byteIndex] |= (1 << bitPosition);
  } else {
    byteArray[byteIndex] &= ~(1 << bitPosition);
  }

  return true;
}

uint8_t getBitValue(size_t row, size_t column) {
  size_t bitIndex = row * numCols + column;
  size_t byteIndex = bitIndex >> 3;

  if (byteIndex >= numArrayElements) {
    return 0;
  }

  uint8_t bitPosition = bitIndex - (byteIndex << 3);

  if ((byteArray[byteIndex] & (1 << bitPosition)) > 0) {
    return 1;
  } else {
    return 0;
  }
}
9

Many thanks!

Lots of inputs, I need some time to better understand some of those things :sweat_smile: . Especially the last code snippet from gfvalvo.

I'll try every solution and come back with the results!

Thanks again

10

poweruser77:
Here below the entire sketch as requested.

I agree with the others that using bits to hold values is more efficient. But it can make the programming more complex. I don't think that there would be a noticeable delay from using a char array to store the button states if the program is written efficiently. I have a program in which I use a char array to hold the states of model railway turnouts. It includes comparing an array of current values with a separate array (sent from the PC) with new values and I am not conscious of any time problem. Using a char array makes debugging much easier.

What's the purpose of all the strcpy() and strcat() stuff - I reckon that's what wasting the time.

...R

11

Robin2:
What's the purpose of all the strcpy() and strcat() stuff - I reckon that's what wasting the time.

Actually just “translating” the old code into the new structure. I agree it’s not efficient.
I implemented the suggestion of printing directly to the serial port but unfortunately no noticeable speed increase.
Will also try the other solutions which seems more efficient than mine.
I still can’t figure why a simple char array is making things so slow..but anyway that’s it.

12

Is the strout array too small? The format for the output as described in your original post would need five characters, to which you are adding a colon at the end, which then leaves no space for the terminating null. Writing past the end of the array could be overwriting some other variable, which could be causing the code to run slowly.

13

poweruser77:

What's the purpose of all the strcpy() and strcat() stuff - I reckon that's what wasting the time.

Actually just “translating” the old code into the new structure. I agree it’s not efficient.

That does not answer my question.

In my mind if you have a char array such as "10001000111100" then you can just view it with Serial.print()

And you would only print it occasionally - not while you are iterating over the array.

...R

14

I'm not sure I got your point but let me clarify better what the purpose was.

I'm interfacing with software which receives strings to determine the status of a button in the matrix. The matrix is made not only by temporary buttons but also two or three state switches which can hold a certain position.
The status of a button must be transmitted only when it changes, otherwise no transmission should occurr.

The protocol I decided to use has the following format: XyyyZ:
Where:
X : identifies the section. I have many devices and each one transmits with its own identifier
yyy = i + j * column goes from 0 to 160 in this case
Z is the status (0 or 1)
':' is the separator required by the receiving software

If the button 35 goes from 0 to 1 the output string would be : X351:

In the previous version I was building the output string by deleting everything (strcpy(strout, ""):wink: and then concatenating the parts of the output. This is not efficient as many of you also pointed out.
Therefore this code is not there anymore. I'm going straight to the Serial.print like:

        Serial.print('P');
        Serial.print(-1 + i + j * columns);
        Serial.print(pinStateSTR);
        Serial.println(':');

Hope it answers your question.

15

david_2018:
Is the strout array too small? The format for the output as described in your original post would need five characters, to which you are adding a colon at the end, which then leaves no space for the terminating null. Writing past the end of the array could be overwriting some other variable, which could be causing the code to run slowly.

The variable probably needs to be strout[7].
Anyhow, avoiding to use strout (see the previous post) didn't speed up the execution.
By following the same line I wonder if the routine:

  for (int i = 0; i < String_State_Size; i++) {
    stringoldstate[i] = stringnewstate[i];
  }

might cause some issues when stringoldstate and stringnewstate have been populated up tho position 160 and String_State_Size has been #defined 161...
Question: if I need to store 160 characters I thought I should create a [161] char array because I need a NULL character at the end. Did I misunderstand this?

16

poweruser77:
Question: if I need to store 160 characters I thought I should create a [161] char array because I need a NULL character at the end. Did I misunderstand this?

It depends what you are storing in the array. A terminating null is required for a complete text string. If you do not treat the array as a complete string, you don't need any extra storage for a null because you don't use it. The only other need would be if you positioned a sub string at the end, it would need a null. I suspect that since you don't treat your data as a string, you don't need the extra byte.

To summarize, if you are accessing individual bytes with an index, no need. If you are using string functions, need.

17

I notice that in the code you posted as an attachment, the scan_buttons_old function calls update_encoder(), while the scan_buttons function has delay(1), which causes the scan to take about 10 times longer.

18

poweruser77:
The status of a button must be transmitted only when it changes, otherwise no transmission should occurr.

In my projects I take the exact opposite approach. I have found it much simpler to send a string with the states of all the items every time (perhaps 5 times per second) even if nothing has changed. The identity of the button is its position in the string. In the case of the turnouts for my model railway the string looks like "SSSTSSSTSS" where 'S' means straight and 'T' means turned. If the next message is also "SSSTSSSTSS" nothing happens. If "STSTSSSTSS" is sent the turnout number1 (counting from 0) must change from straight to turned.

This way there is no need to construct a string. To change the S to a T all that is needed is turnoutList[[1] = 'T';

To check if a turnout needs to be changed all that is needed is something like this

for (byte n = 0; n < numTurnouts; n++) {
   if (turnoutActual[n] != turnoutList[n]) {
        // code to change turnmout
        turnoutActual[n] = turnoutList[n];
   }
}

which completes very quickly

...R