Push to 2D array (log) but last value pushed is everywhere

Question

I have this simple that is supposed to keep a log for me with a fixed number of entries. When I add to the log the oldest replaced is by the second oldest etc and the new entry added. Except when I add another entry both that and the one before have the same value (My C isn't any good, I'm more a ruby and lua guy).

What am I doing wrong here?

void setup(void) {
  Serial.begin(9600);
}

#define log_size 5
#define line_height 12
char *log_lines[log_size] = {"Hello", "World"};

void addToLog(char *text) {
  for(int i = log_size - 1; i > 0; i--) {
    log_lines[i] = log_lines[i-1];
  }
  log_lines[0] = text;
}

void printLog() {
  for(int i = 0; i < log_size; i++) {
    Serial.print(i);
    Serial.print(" ");
    Serial.println(log_lines[i]);
  }
}

void loop(void)
{
  char msg[20];
  sprintf(msg, "%d", millis());
  addToLog(msg);
  printLog();
  delay(1000);
}

The output after the first addToLog is

0 1000
1 Hello
2 World

second addToLog is

0 2001
1 2001
2 Hello
3 World

Answer 1

It is easy - in your log you store POINTERS to strings.

And you then create variable on the stack (ouch) and put its address to the log.

Then you leave the function (loop) (so that variable is forgotten and can be overwritten by anything). I will not say it is luck, that nothing uses the stack enought to make a problem before the loop is called again, as it disguise the problem. The POINTER in log_lines[0] points to UNALLOCATED place (where the variable msg once was).

Then the loop allocates variable msg (at the same place, as its previous run by the chance) so the POINTER stored in log_lines[0] (still pointing to the same place) points to the variable msg (and its content). By pure (bad) luck.

Then you overwrite the variable msg with a new value, so the log_lines[0] point to that new value.

Then you move the content of log_lines up, so the log_lines[1] now points to the variable msg (and its NEW value) as well as log_lines[0].

Then you assign log_lines[0] with address of variable msg (which it accidetally already contains)

Then you wonder, why variable msg accesed via log_lines[1] is exactly same as the (same) variable msg accesed vial log_lines[0] and why it contains the last (NEW) value, which you assigned into it.

---

Some of possible solutions are:

1) allocate place on heap (memaloc it) in the addToLog function, copy the text into it and store pointer to THIS place (and do not forget deallocate last log_lines before overwriting it - if it is not null (as untinitialized values are all 0)) which could lead to (relativelly) expensive heap manipulations and possible memory fragmentation.

2) allocate log_lines as array of char arrays of given len, copy the content of each line in another loop inside addToLog and then copy(and possibly truncate) the text to the log_line[0]

3) keep static index of last used log_line and in addToLog just increment it (wrapping around the len of the array), then overwrite the line pointed by index with (truncated) text and in printLog just start cycle on the index and climb (wrapping around len of the array) until you are again at index (which you would not print again and stop)

Answer 2

Here is a solution which does not use dynamic allocation nor does it copy any log message. It's essentially a zero-copy implementation of gilhad's solution 3. The idea is: instead of having an addToLog() function that takes an already prepared message (which it would need to copy), you have a get_log_buffer() function that tells you where to write the message in the first place. This way you can snprintf() directly into the right place in memory.

const size_t LOG_SIZE = 5;
const size_t MSG_SIZE = 20;

char log_stack[LOG_SIZE][MSG_SIZE];  // circular log buffer
size_t log_index;  // next available slot in the circular buffer

// Return the address of the next available message slot.
char *get_log_buffer()
{
    char *buffer = log_stack[log_index];
    if (++log_index == LOG_SIZE) log_index = 0;  // increment index
    return buffer;
}

void print_log()
{
    Serial.println(F("--------"));
    for (size_t i = 0, j = log_index; i < LOG_SIZE; i++) {
        j = (j ? j : LOG_SIZE) - 1;  // decrement index
        if (log_stack[j][0]) {  // should be a non-empty message
            Serial.print(i);
            Serial.print(" ");
            Serial.println(log_stack[j]);
        }
    }
}

void setup()
{
    Serial.begin(9600);
}

void loop()
{
    snprintf(get_log_buffer(), MSG_SIZE, "%lu", millis());
    print_log();
    delay(1000);
}

Note that log_stack and log_index are zero-initialized by the C runtime. Note also the use of snprintf() instead of sprintf() in order to avoid buffer overruns.