Simple/stupid array problem?

Using Arduino Programming Questions
1

Hi, Im trying to keep a log of events in an array and use it to display messages to an lcd, but Im stuck finding the mistake in the following, the full example is attached. When a new item is added, it appears in both of the last two positions

void udpateMSGLog(int msgKey){  //FIFO, cycle the msglog and append new msgKey
  Serial.println("udpateMSGLog");
  for (int x=0; x<=(lcdLines); x++){  
    if(x==lcdLines){
      msgLog[lcdLines] = msgKey;
      Serial.print(lcdLines);Serial.print(" ");Serial.println(msgLog[lcdLines]); 
    }else{
      msgLog[x]=msgLog[x+1];
      Serial.print(x);Serial.print(" ");Serial.println(msgLog[x]);
    }
  }     
}

Please point out my glaring mistakes I cant see them :frowning:
Thanks

arraytest.ino (1.63 KB)

2

itnoworking:
Hi, Im trying to keep a log of events in an array

    }else{

msgLog[x]=msgLog[x+1];
}

That looks as if you are trying to move values along in the array. That is not the best way to do things. You should create a circular buffer and update the indices that point to the head and tail of the buffer. That way there is no need to move the actual values.

The index for the tail should always point to the array element that can next be written to. Consequently the function to place a new value in the buffer would be as simple as

void saveValueInBuffer(int msgKey) {
    msgLog[tailNdx] = msgKey;
    tailNdx ++;
    if (tailNdx >  lcdLines) {
       tailNdx = 0;
    }
}

I'm not sure (without knowing more about your requirement) whether headNdx should also be updated.

You could have an somewhat similar function to read from the buffer starting at headNdx and ending at tailNdx.

...R

3

Many thanks, your method is clearly much more efficient.

My requirement is to monitoring temperatures and display an lcd message log detailing what previous condition occurred at the last x events. Each condition has char[] message in a 2d array, the index for the message is stored in the log and each log entry displayed on the appropriate lcd line. I think I would want to include a headNdx to keep a rolling log.

My original, now redundant, piece of code had a bug where new events were entered on the last two msgLog[] cells, I had ASSumed that this was due to incorrectly referencing the cells but was not able to find the cause. Can anyone point out where I have make the mistake as I would like to understand where I was going wrong.

Thanks

4

So didnt find the problem but now have an example that works.
If there is a neater way of doing this please let me know

Thanks

arraytest.ino (1.46 KB)

5

itnoworking:
If there is a neater way of doing this please let me know

For short programs it is more helpful to include them in your Post. People reading the Forum on a tablet cannot usually download programs.

const int lcdLines = 3; //inc 0
const int msgLenght=5;  //inc 0
int msg;
int msgLog[lcdLines+1]={0,0,0,0};
int tailNdx=0;
int logNdx=0;
char logMessages[][msgLenght]={{"zero"},{"one"},{"two"},{"three"}};

void setup() {
  // put your setup code here, to run once:
  Serial.println("setup");
  randomSeed(analogRead(0));
  Serial.begin(9600);
  delay(1000);
}

void loop() {
  Serial.println();
  msg=random(0,lcdLines);
  Serial.print("msg ");  Serial.println(msg);
  saveValueInBuffer(msg);
  displayLog();  
  displaylogMessages();
  delay(1000);
}

void saveValueInBuffer(int newMsg) {  
  Serial.print("tailNdx ");  Serial.println(tailNdx);
  msgLog[tailNdx] = newMsg;  
  tailNdx ++;
  if (tailNdx >  lcdLines) {
     tailNdx = 0;
  }
}

void displayLog(){  //display log, oldest to newest
  logNdx=tailNdx; //tailNdx has already been inc to nxt cell so points to oldest data
  //checklogNdx();
  Serial.print("logNdx "); Serial.println(logNdx);
  for (int x=0; x<=lcdLines; x++){
    Serial.print(msgLog[logNdx]); Serial.print(", ");
    logNdx++;
    checklogNdx();
  }
  Serial.println();
}

void checklogNdx(){
 if(logNdx > lcdLines){
  logNdx=0; 
 }
 //return logNdx;
}

void displaylogMessages(){
  logNdx=tailNdx; //tailNdx has already been inc to nxt cell so points to oldest data
  for (int x=0; x<=lcdLines; x++){    
    for (int y=0; y<=msgLenght-1; y++){
      Serial.print(logMessages[msgLog[logNdx]][y]);  
    }
    logNdx++;
    checklogNdx();
    Serial.println();
  }
}

The code looks OK to me but I have not tested it.

...R

6

itnoworking:
My original, now redundant, piece of code had a bug where new events were entered on the last two msgLog[] cells, I had ASSumed that this was due to incorrectly referencing the cells but was not able to find the cause. Can anyone point out where I have make the mistake as I would like to understand where I was going wrong.

The original problem was caused by your code exceeding the bounds of the msgLog array. When you declared msgLog as having three elements, those elements are 0, 1, and 2, but in your code, in the for() statements, you are setting x to 0, 1, 2, and 3. You got lucky, and the code doesn't crash when you write a value to msgLog[3], but it would appear that the value you were appending to the array just happened to be stored at that memory location, so got copied to msgLog[2] when you were shifting the array, creating the duplicate entry at the end.

7

Robin2:
For short programs it is more helpful to include them in your Post. People reading the Forum on a tablet cannot usually download programs.

const int msgLenght=5;  //inc 0

char logMessages[][msgLenght]={{"zero"},{"one"},{"two"},{"three"}};

The string "three" will not fit in a 5 element array. You need the trailing '\0' so your message length should be 6

8