Why does this code lead to a reinit of my Mega 2560

This forum is dedicated to software development related to MultiWii.
It is not the right place to submit a setup problem.
Software download
Post Reply
User avatar
jevermeister
Posts: 708
Joined: Wed Jul 20, 2011 8:56 am
Contact:

Why does this code lead to a reinit of my Mega 2560

Post by jevermeister »

Hi,
I recently programmed a new buzzer code.

I used chars to define a beep pattern.
However, the following code semas to lead to a crash, if I try to enable GPS RTH or doing wild stuff with my copter.

I only can reproduce the reinit of the Mega if I enable the buzzer code via define buzzer.
So I changed the code to use uint16 instead of char and deleted the switch case code. and now it works.

This code leads to a reinit very often:

Code: Select all

//===================== Priority driven Handling =====================
  // beepcode(length1,length2,length3,pause)
  //D: Double, L: Long, M: Middle, S: Short, N: None
  if (warn_failsafe == 2)      beep_code('L','N','N','D');                 //failsafe "find me" signal
  else if (warn_failsafe == 1) beep_code('S','M','L','M');                 //failsafe landing active         
  else if (warn_noGPSfix == 1) beep_code('S','S','N','M');       
  else if (beeperOnBox == 1)   beep_code('S','S','S','M');                 //beeperon
  else if (warn_vbat == 4)     beep_code('S','M','M','D');       
  else if (warn_vbat == 2)     beep_code('S','S','M','D');       
  else if (warn_vbat == 1)     beep_code('S','M','N','D');           
  else if (warn_runtime == 1 && f.ARMED == 1)beep_code('S','S','M','N'); //Runtime warning           
  else if (toggleBeep > 0)     beep(50);                                   //fast confirmation beep
  else {
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }   
}

void beep_code(char first, char second, char third, char pause){
  char patternChar[4];
  uint16_t patternInt[4];
  static uint8_t icnt = 0;
 
  patternChar[0] = first;
  patternChar[1] = second;
  patternChar[2] = third;
  patternChar[3] = pause;
  switch(patternChar[icnt]) {
    case 'M':
      patternInt[icnt] = 100;
      break;
    case 'L':
      patternInt[icnt] = 200;
      break;
    case 'D':
      patternInt[icnt] = 2000;
      break;
    case 'N':
      patternInt[icnt] = 0;
      break;
    default:
      patternInt[icnt] = 50;
      break;
  }
  if(icnt <3 && patternInt[icnt]!=0){
    beep(patternInt[icnt]);
  }
  if (icnt >=3 && (buzzerLastToggleTime<millis()-patternInt[3]) ){
    icnt=0;
    toggleBeep =0;
  }
  if (blinkdone == 1 || patternInt[icnt]==0){
    icnt++;
    blinkdone=0;     
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }
 
}


this code seems to be stable, I could not reproduce the reinit:

Code: Select all

//===================== Priority driven Handling =====================
  // beepcode(length1,length2,length3,pause)
  //None: 0, Short:50, Medium: 100, Long: 200, Double: 2000 .
  if (warn_failsafe == 2)      beep_code(200,0,0,2000);                 //failsafe "find me" signal
  else if (warn_failsafe == 1) beep_code(50,100,200,100);                 //failsafe landing active         
  else if (warn_noGPSfix == 1) beep_code(50,50,0,100);       
  else if (beeperOnBox == 1)   beep_code(50,50,50,100);                 //beeperon
  else if (warn_vbat == 4)     beep_code(50,50,100,2000);       
  else if (warn_vbat == 2)     beep_code(50,100,0,2000);       
  else if (warn_vbat == 1)     beep_code(100,0,0,2000);           
  else if (warn_runtime == 1 && f.ARMED == 1)beep_code(50,50,100,0); //Runtime warning           
  else if (toggleBeep > 0)     beep(50);                                   //fast confirmation beep
  else {
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }   
}

void beep_code(uint16_t first, uint16_t second, uint16_t third, uint16_t pause){
  uint16_t patternInt[4];
  static uint8_t icnt = 0;
 
  patternInt[0] = first;
  patternInt[1] = second;
  patternInt[2] = third;
  patternInt[3] = pause;

  if(icnt <3 && patternInt[icnt]!=0){
    beep(patternInt[icnt]);
  }
  if (icnt >=3 && (buzzerLastToggleTime<millis()-patternInt[3]) ){
    icnt=0;
    toggleBeep =0;
  }
  if (blinkdone == 1 || patternInt[icnt]==0){
    icnt++;
    blinkdone=0;     
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }
}


I don't see any mistakes, the chars is using the same amount of memory the uint16 is using. so the only difference is the switch case block.

But what went wrong?

Anyone got an idea?

Nils

mr.rc-cam
Posts: 457
Joined: Wed Jul 27, 2011 11:36 pm

Re: Why does this code lead to a reinit of my Mega 2560

Post by mr.rc-cam »

I suspect it is this:

Code: Select all

if (icnt >=3 && (buzzerLastToggleTime<millis()-patternInt[3]) )


because it can allow icnt to be > 3 and cause a corruption of ram when you do this:

Code: Select all

patternInt[icnt] = some_number; 


Try this:
Change

Code: Select all

icnt++;

to:

Code: Select all

if(icnt<3) {
  icnt++;
}


- Thomas

Sebbi
Posts: 478
Joined: Sun Jul 08, 2012 1:08 am
Location: Germany
Contact:

Re: Why does this code lead to a reinit of my Mega 2560

Post by Sebbi »

Your switch statement

Code: Select all

switch(patternChar[icnt]) {

will cause a crash with icnt > 3 ... there could be situations when this is the case ...

Code: Select all

if (icnt >=3 && (buzzerLastToggleTime<millis()-patternInt[3]) ){
    icnt=0;
    toggleBeep =0;
  }

icnt will onyl be reset if the second condition is true, but ...

Code: Select all

  if (blinkdone == 1 || patternInt[icnt]==0){
    icnt++;
    blinkdone=0;     
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }

... could still increase it to a value > 3 ...

User avatar
jevermeister
Posts: 708
Joined: Wed Jul 20, 2011 8:56 am
Contact:

Re: Why does this code lead to a reinit of my Mega 2560

Post by jevermeister »

Hi,
Yep,
both of you where kind of right.

First of all, letting icnt getting bigger than 3 was a bad mistake

But the reason why the code crushed was this:

Code: Select all

patternInt[icnt] = something;   

if icnt >3
the switch case can handle icnt >3 (I tested that)

nevertheless my code was very wrong so I fixed it according to your suggestions:

(I also replaced patternint[] with a single 16bit int because an array is not necessary here.


Code: Select all

  //===================== Priority driven Handling =====================
  // beepcode(length1,length2,length3,pause)
  //D: Double, L: Long, M: Middle, S: Short, N: None
  if (warn_failsafe == 2)      beep_code('L','N','N','D');                 //failsafe "find me" signal
  else if (warn_failsafe == 1) beep_code('S','M','L','M');                 //failsafe landing active         
  else if (warn_noGPSfix == 1) beep_code('S','S','N','M');       
  else if (beeperOnBox == 1)   beep_code('S','S','S','M');                 //beeperon
  else if (warn_vbat == 4)     beep_code('S','M','M','D');       
  else if (warn_vbat == 2)     beep_code('S','S','M','D');       
  else if (warn_vbat == 1)     beep_code('S','M','N','D');           
  else if (warn_runtime == 1 && f.ARMED == 1)beep_code('S','S','M','N'); //Runtime warning           
  else if (toggleBeep > 0)     beep(50);                                   //fast confirmation beep
  else {
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }   
}

void beep_code(char first, char second, char third, char pause){
  char patternChar[4];
  uint16_t Duration;
  static uint8_t icnt = 0;
 
  patternChar[0] = first;
  patternChar[1] = second;
  patternChar[2] = third;
  patternChar[3] = pause;
  switch(patternChar[icnt]) {
    case 'M':
      Duration = 100;
      break;
    case 'L':
      Duration = 200;
      break;
    case 'D':
      Duration = 2000;
      break;
    case 'N':
      Duration = 0;
      break;
    default:
      Duration = 50;
      break;
  }
   
  if(icnt <3 && Duration!=0){
    beep(Duration);
  }
  if (icnt >=3 && (buzzerLastToggleTime<millis()-Duration) ){
    icnt=0;
    toggleBeep =0;
  }
  if (blinkdone == 1 || Duration==0){
    if (icnt < 3){icnt++;}
    blinkdone=0;     
    buzzerIsOn = 0;
    BUZZERPIN_OFF;
  }
 
}


I tried everything to crash this code but it seems to work now.

Thank you for helping me find this error guys!

Nils

Post Reply