Arrays: Is a subscript getting out of range at run time?

Watcher · June 18, 2015, 4:41am

Hi all,

I am making extensive use of arrays in my sketch and also passing arrays as arguments in functions.
The contents are eventually written on an SD card. The text 'line' to be written on the SD card is constructed by concatenating the contents of other variables and there are also conversions from int to char at various points

The sketch seems to work fine at first but when left running overnight, it freezes and the system becomes unresponsive. In addition, I find that some data is sometimes getting corrupt. For example certain variable which is set at setup at a certain value and never changes later on, is found to contain corrupt data. I am suspecting that for some reason this is caused by some array subscript getting out of range at run time eventhough I did try to make sure that array boundaries are never violated by inserting validation checks.

My question is:

If this were the case, would it explain the freezes I am experiencing?
Is there a way to detect arrays getting out of range.

I was thinking to post my code as well, the sketch however is quite big.

PaulMurrayCbr · June 18, 2015, 5:11am

Sounds like a memory leak. Are you using malloc() or new and not freeing the memory you allocate?

The other likely problem is array bounds, yes.

There isn't really a way to detect arrays going out of range as such. But one method is to move all acceses to arrays into functions that do bounds checking. And the way to do that is to create a bounds checked array class that overloads the '[' operator. I'm sure there are heaps of examples.

A more important questiton is: if you do detect an out-of bounds condition, what then? Simplest solution is to print something to serial and then to enter a HALT state. Be sure that what you send to serial is meaningful! Easiest way to do that is for your array class to also include a label constant to be printed as part of the message.

boolean WOAH_THERE = false;
void loop() {
  if(WOAH_THERE) return;  

  … rest of your code …

Consider using longjmp() or throwing an exception (does C++ have exceptions?) to back all the way out of whatever it is you are doing.

Watcher · June 18, 2015, 5:24am

Hi... and thanks for your message.

No I am not using malloc().. I am not using pointers either since I am not confident using them
I just use direct access to array data.
The following function logs data onto SD:

boolean LogSDCard(char LogData[35], byte Start, byte Len, byte time){ .....

What I am suspecting is that LogData [] subscript is getting out of bounds before it is passed to the function above.

system · June 18, 2015, 6:37am

Post code.

Watcher · June 18, 2015, 7:10am

Thanks for your messages.

I am posting the two main functions that handle arrays. I understand its not easy to follow the program flow here however...

void DoTargetBusCommands (byte cmd, byte Target){
  
 if ((readEEPROM(disk1, 4800+(cmd-1)*140+(Target-1)*7)==0x00)or (readEEPROM(disk1, 4800+(cmd-1)*140+(Target-1)*7)==0xff)) {return;}     
   
 byte TargetState=readEEPROM(disk1, 4+4800+(cmd-1)*140+(Target-1)*7);  // Variable to hold the target state to be sent
 DataLog ="";
        
    OutPacket[1]=DeviceSubNet;
    OutPacket[2]=DeviceID; 
    OutPacket[3]=highByte(DeviceType);   
    OutPacket[4]=lowByte(DeviceType); 
        
    if (readEEPROM(disk1, 4800+(cmd-1)*140+(Target-1)*7)==0x59) {       // Single channel lighting command
    
    OutPacket[0]=15;     // Length of packet
    OutPacket[5]= 0x00;  // op code 1 
    OutPacket[6]= 0x31;  // op code 2
    OutPacket[7]=  readEEPROM(disk1, 1+4800+(cmd-1)*140+(Target-1)*7); // target device subnet
    OutPacket[8]=  readEEPROM(disk1, 2+4800+(cmd-1)*140+(Target-1)*7); // targer device ID
    OutPacket[9]=  readEEPROM(disk1, 3+4800+(cmd-1)*140+(Target-1)*7); // Channel No
    
    
    PORT.println (CurrentStatus[cmd]);
    if ((CurrentStatus[cmd]==0)and ReverseControl) {OutPacket[10]=100;}
    if (((CurrentStatus[cmd]>0)and ReverseControl) or (TargetState==0)) {OutPacket[10]=0;}
    if (not ReverseControl) {OutPacket[10]= TargetState;}             // Brightness Level
    
    OutPacket[11]= readEEPROM(disk1, 5+4800+(cmd-1)*140+(Target-1)*7); // High 8 bit running time
    OutPacket[12]= readEEPROM(disk1, 6+4800+(cmd-1)*140+(Target-1)*7); // Low 8 bit running time
    Pack_crc(OutPacket,OutPacket[0]-2);  // Get CRC codes
    SendFeedback();
      
    for (int x=11; x<31; x++) {ReplyBuffer[x-11]=readEEPROM(disk1, (x-11)+2800+(cmd-1)*20); }
   
   LogSDCard(" COMMAND:  ",0,11,2); 
   LogSDCard(ReplyBuffer,0,20,1);
   LogSDCard("    ",0,4,1);
   DataLog=String(OutPacket[10]);
   string2char();
   LogSDCard(ReplyBuffer,0,DataLog.length(),1);
   LogSDCard("%  SubNet:",0,10,1);
   DataLog=String(OutPacket[7]);
   string2char();
   LogSDCard(ReplyBuffer,0,DataLog.length(),1);
   LogSDCard("  ID:",0,5,1); 
   DataLog=String(OutPacket[8]);
   string2char();
   LogSDCard(ReplyBuffer,0,DataLog.length(),1);
   LogSDCard("  CH:",0,5,1); 
   DataLog=String(OutPacket[9]);
   string2char();
   LogSDCard(ReplyBuffer,0,DataLog.length(),1);
   LogSDCard("Origin:",0,7,1); 
   IPAddress remote = Udp.remoteIP();
     
   for (int i =0; i < 4; i++)  {
     DataLog=String(remote[i]); 
     string2char();
     LogSDCard(ReplyBuffer,0,DataLog.length(),1);
     if (i < 3) { LogSDCard(".",0,1,1); }
    }
  
    return;
   }
      
    if (readEEPROM(disk1, 4800+(cmd-1)*140+(Target-1)*7)==0x55) {    // AREA 
    OutPacket[0]=13;     // Length of packet
    OutPacket[5]= 0x00;  // op code 1 
    OutPacket[6]= 0x02;  // op code 2
    OutPacket[7]=  readEEPROM(disk1, 1+4800+(cmd-1)*140+(Target-1)*7); // target device subnet
    OutPacket[8]=  readEEPROM(disk1, 2+4800+(cmd-1)*140+(Target-1)*7); // targer device ID
    OutPacket[9]=  readEEPROM(disk1, 3+4800+(cmd-1)*140+(Target-1)*7); // Area No
    
    if ((CurrentStatus[cmd]==0)and ReverseControl) {OutPacket[10]= readEEPROM(disk1, 4+4800+(cmd-1)*140+(Target-1)*7);}
    if ((CurrentStatus[cmd]>0 )and ReverseControl) {OutPacket[10]=0;}
    if (not ReverseControl) {OutPacket[10]= readEEPROM(disk1, 4+4800+(cmd-1)*140+(Target-1)*7);} // Scene No
     
    Pack_crc(OutPacket,OutPacket[0]-2);  // Get CRC codes
    SendFeedback();
   
   
    return;




boolean LogSDCard(char LogData[35], byte Start, byte Len, byte time){
   
  if (logging<1) { return 0xf5; }                        // exit if logging is disabled
 
  File dataFile = SD.open("datalog.txt", FILE_WRITE);
  if (time==2) {GetTime();}
  if (dataFile) {                                          // if the file is available, write to it:
     if ((time==2) or (time==0)) {dataFile.println();PORT.println();PORT.println("*** Data Logged ***");}
     if (time==2) {dataFile.print(timenow);PORT.print(timenow);}
     dataFile.print("  ");
     if   (Start+Len>34) {PORT.println("Error- Subscript out of range");PORT.print(Start+Len);dataFile.println("Error- Subscript out of range"); dataFile.close();return 0xf5;}
     for (int x=Start; x<Start+Len+1; x++){
         dataFile.write (LogData[x]); 
         PORT.write(LogData[x]);}
        
     dataFile.close();
     return 0xf8;
   }  
 
  else {                                                   // if the file isn't open, pop up an error:
    digitalWrite (ErrorLed,HIGH);
    PORT.println("Error opening datalog.txt");
    return 0xf5;
   }
 }

system · June 18, 2015, 7:13am

Sorry, I thought the "all" was implicit in "post code".

Watcher · June 18, 2015, 7:43am

Code length exceeds the allowed limits here...
So i ve used an attachment

Thanks

terminal_test.ino (79.7 KB)

tammytam · June 18, 2015, 10:06am

Thats a lot of code to comb through. What I have noticed however, is you have a string called DataLog, which you use as a scratch pad, then you have a char array called ReplyBuffer that you copy into from your Datalog string each time its used, then you finally push a copy of the entire array over using the function LogSDCard().

String class is convenient, but might be worth a look at just maintaining a single char array for your logs. Can use the following functions to do what youre currently doing with the string class:

char text[35];
strcpy( text, "Reply" );  // copies in the 2nd parameter
int length = strlen( text ); // Returns the length of the string similar to your use of DataLog.length()
strcat( text, " again" ); // concatenates the 2nd parameter onto the end of the string, so it now read "Reply again
text[0] = 0; // effectively clears the string, similar to your use of DataLog=" ";

sprintf also deserves a mention also.

This can al then be wrapped up into one function call, so any validation can be done there. Currently all it takes is for you to miss one DataLog=" "; and your system will likely crash.

system · June 18, 2015, 10:14am

I really despair when I see blocks of near-identical code like this

    OutPacket[0]=33;      // Length of packet
    OutPacket[1]=DeviceSubNet; 
    OutPacket[2]=DeviceID; 
    OutPacket[3]=highByte(DeviceType); 
    OutPacket[4]=lowByte(DeviceType);  
    OutPacket[5]=0xef;  // op code 1 
    OutPacket[6]=0xfe;  // op code 2
    OutPacket[7]=IncPacket[1];  // target originating
    OutPacket[8]=IncPacket[2];  // target originating

repeated over and over again.

Watcher · June 18, 2015, 10:36am

String class is convenient, but might be worth a look at just maintaining a single char array for your logs. Can use the following functions to do what youre currently doing with the string class:

strcat( text, " again" ); // concatenates the 2nd parameter onto the end of the string, so it now read "Reply again

Thanks for your input and suggestions tammytam regarding string manipulation!

How do I deal with cases where the variable holding the data to be concatenated to text[] is a numeric one?

Thanks

system · June 18, 2015, 10:38am

How do I deal with cases where the variable holding the data to be concatenated to text[] is a numeric one?

sprintf?
(Maybe I didn't understand the question - all variables are numeric)

Watcher · June 18, 2015, 10:45am

You are right about repeated code AWOL. I do plan to make do something about that too!

Regarding numeric variables i meant for example:

DataLog=String(OutPacket[8]);

which holds a number as opposed to others that hold text.

Could you give an example of the usage of sprintf? I did try to use it at one point but couldn't understand the syntax.

jurs · June 18, 2015, 11:13am

Watcher:
The sketch seems to work fine at first but when left running overnight, it freezes and the system becomes unresponsive.

Maybe an overusage of RAM, caused by:

not using the F-macro to save RAM
RAM fragmentation due to "String" class usage causing dynamic RAM fragmentation

Which Atmega board are you using?
How much of free memory does your application have on startup?
Did you check for possible memory leaks?

As a first step I'd provide more available RAM by using the F-macro when printing string constants. Instead of:

   PORT.println(" h - This help screen");   
   PORT.println(" s - Show System status, free RAM etc");  
   PORT.println(" t - Show Bus System Time");
   PORT.println(" n - Attempt module time Sync with NTP Server");

I'd better use the F-macro to print constant strings directly from flash memory:

   PORT.println(F(" h - This help screen"));   
   PORT.println(F(" s - Show System status, free RAM etc"));  
   PORT.println(F(" t - Show Bus System Time"));
   PORT.println(F(" n - Attempt module time Sync with NTP Server"));

Next thing would be: Completely avoid 'String' class objects and use C-Strings/char-arrays only.

Watcher · June 18, 2015, 12:05pm

Thanks jurs! I ll try that and see if it helps. Btw there re about 2k free ram when running on Mega

tammytam · June 18, 2015, 12:14pm

Its a good point by jurs to use the F() macro.

So to create a string with numbers, use sprintf:

char text[64];
char more_text[32];

int a_number = 349;
int b_number = 781;

strcpy( more_text, "some_text" );
sprintf( text, "%d", a_number ); // text now contains "349"
sprintf( text, "Number : %d AnotherNumber: %d (%s)\n", a_number, b_number, more_text ); // text now contains "Number : 349 AnotherNumber: 781 (some_text)"

Formatted strings can take a bit to get used to, but simply, it scans from left to right, entering the text as it appears until it hits a format specifier (there's a lot of these, but the popular ones are %d for signed int, %s for string %x hex, arduino doesn't support %f in sprintf for float ), it then replaces that specifier with the next parameter in your list.

Watcher · June 18, 2015, 2:37pm

Thanks again tammytam. I have replaced println with println(F... and the free mem has jumped from around 2kb to about 3.3kb

I will now leave it running overnight and see if it freezes again.

Later I ll try get rid of Strings as well..

Watcher · June 21, 2015, 6:06pm

Update:
1.Replaced println with println (f... )
2. Got rid of all string class use and replaced with char arrays

Good news:System now doesn't freeze as it did earlier!
However I still detect corrupt data is some variables resulting in unexpected behaviour.

Note: free mem is more than 3 kb at run time.

Any ideas?

Thanks for your precious help!

system · June 21, 2015, 7:45pm

Any ideas?

See reply #3

Watcher · June 22, 2015, 4:58am

OK... here s the now revised code

terminal_test_revised.ino (81.9 KB)

Topic		Replies	Views
[FYI] Check your bounds when using arrays Programming	4	3221	May 5, 2021
Problem concerning arrays and/or subtraction of zero during intialization Programming	4	797	May 5, 2021
Array subscript s outside array bounds Programming	5	869	April 20, 2024
an easier way of working with arrays Libraries	6	2088	May 6, 2021
Need help simple code 10 lines but gave a strange bug.. Programming	8	710	May 5, 2021

Arrays: Is a subscript getting out of range at run time?

Related topics