facing a problem Nested if else in atmega 168p

Go To Last Post
33 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi moderators and forum members,

I am facing a problem nested if else in atmega 168p.

part of code:

 

void Sensor_buzzer (){

if (Check_Gas_Voltage(Sensor1)){        // expression 1

 uint8_t i=0;

if (i==0){                                           // expression 2

_delay_ms(300);              

Buzzer_Tune();

Sensor_Relay_Mechanism();

i++;

}

else if (i>0){                                   // expression 3

Buzzer_Tune();

}

else;

_delay_ms(50);

}

I am going to run (expression 2)  one time.

But, (expression 2) runs continously. (expression 3)  is not running.

Could you advise me?  

 

 

 

This topic has a solution.

sadf

Last Edited: Wed. Jan 9, 2019 - 01:38 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are COnstantly setting i to 0.  As soon as you exit the IF statement, once the Check_Gas_Voltage(Sensor1) hits again your next instruction sets i to 0.

 

JIm

If you want a career with a known path - become an undertaker. Dead people don't sue! - Kartman

Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB user

Last Edited: Tue. Jan 8, 2019 - 06:50 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Every pass through, "i" is reset to zero. You need to make "i" static.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

How to declare static variable?

sadf

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
 static uint8_t i=0;

This is really (really) basic "C-stuff"!

 

The initializer will be used the very first time the function is used. The value persists after the function is exited. That variable value will then be used the next time the function is used. If i is changed during this second use, then that changed value will be used on the THIRD call, and so forth.

 

You only increment i in one case, so there is no chance that i will grow to the point where it rolls over, back to zero. So, you should be  good to go.

 

By the way, this has nothing to  do with "if" statements or the nesting of "if" statements.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Tue. Jan 8, 2019 - 07:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I can't believe that folks really write code like that without indentation! How on earth do you follow your own code? Let alone expecting others to be able to read it too. Surely it is more readable as:

void Sensor_buzzer (){

    if (Check_Gas_Voltage(Sensor1)){        // expression 1

        uint8_t i = 0;

        if (i == 0){                                           // expression 2

            _delay_ms(300);              

            Buzzer_Tune();

            Sensor_Relay_Mechanism();

            i++;

        }

        else if (i > 0){                                   // expression 3

            Buzzer_Tune();

        } 

    }

    else;

    _delay_ms(50);

}

Apart from anything else that last "else;" looks VERY suspicious.

 

The whole sense of this code looks odd anyway. So if i starts at o (we know it does) then on the first iteration the i++ takes it to 1 and ever after (i == 0) is not true, (i > 0) is true so it will then just do "Buzzer_tune" for ever more. So it's hardly a "counter"as the code might initially suggest - it's really just a "bool".

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Agree -

 

I was focussing on the multiple executions where one was expected. It is possible that the lack of indentation is due to NOT using the code display option. For the OP, that button with "<>" gives you a code window that supports full indents. The regular text window does not. For code such as what was originally posted, the readability is nearly zero. Cliff shows what a difference the indentation makes.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
Apart from anything else that last "else;" looks VERY suspicious.

 

Yeah, that else doesn't do anything, right? Might as well not be there, or maybe it's just a placeholder and will do something in the future.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

El Tangas wrote:
Yeah, that else doesn't do anything, right? Might as well not be there,
My worry actually was that the original intention might have been that the _delay_ms(50) that follows it is intended to be predicated on the else but the semi-colon puts paid to that.

 

Just having a hanging "else;" otherwise seems to make very little sense and may just mislead the reader. In fact they may read it as if that semi-colon were not there if the intention is actually otherwise.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If the code had indentation, we could have a clue as to the original intention, but as it is, who knows?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

And people moan about the way Python does statement blocks! ;-)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
And people moan about the way Python does statement blocks! ;-)
;-)

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

And people moan about the way Python does statement blocks! ;-)

 

No, I use perl wherever python would apply, so I moan about @#$%&{};-)

 

--Mike

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Stop LITHPING )))))))))))))))))))))))))

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The "else;" is probably your problem. The first statement-or-block after the else is what it controls. In this case, that's the bare semicolon, and the line after that is outside the if/else entirely.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The OP has started another thread on the code from this thread most likely because  of the Python derailment here.

 

Lets try to stay on the topic.  Seems the OP has a new issue...

 

From the Other thread:

https://www.avrfreaks.net/forum/...

 

void Sensor_buzzer (){
static uint8_t i=0;

if (Check_Gas_Voltage(Sensor1)){        // expression 1

if (i==0){                                           // expression 2

_delay_ms(300);           
i++;
}

else if (i>0){                                   // expression 3

Buzzer_Tune();
Sensor_relay_mechanism();

} 

}

else{ i=0;};

_delay_ms(50);

}

I am going to do that (expression 2) runs one time during the gas leakage and reset. If gas leaks continously, it should be first time delay 3 seconds and making a sound. If gas leaks less than 3 seconds, it should be reset void function (start from (expression 1)).
The problem is that if gas leaks less than 3 seconds, it will making a sound one time. How to solve this problem?

 

 

The OP still needs to work on their indentation, but lets see what advice we can supply.

 

JIm

If you want a career with a known path - become an undertaker. Dead people don't sue! - Kartman

Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:
The OP has started another thread on the code from this thread most likely because of the Python derailment here.

It may have been better to let the new (and clean) thread live, even only for the benefit of future readers. This one could fester along in it's derailed state without harm.

 

jgmdesign wrote:
but lets see what advice we can supply

My advice to the OP is very simple: Throw away the code you're written so far and take some time out to learn about state machines. You really do need to learn this.

 

There's a brief tutorial on AVRfreaks [TUT][SOFT] Simple State Machines for Embedded Systems pt 1 {Hmm! That whole Tutorials section could really do with an index}

There's also a nice guide here which seems quite low-level: How to Code a State Machine in C or C++

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think the takeaway here is you need to DESIGN before coding. Frosty’s suggestion of a state machine is a good one. It is very simple and a fundamental technique. You really should only need one delay of, say, 10ms and count the other times from that.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman,
When you say 'delay of 10ms' would you be referring to a timer that ticks every 10ms?

Jim

If you want a career with a known path - become an undertaker. Dead people don't sue! - Kartman

Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I didn’t want too great a leap, so delays, but, yes a timer tick would be my choice.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you for your advise,

I am trying to solve this problem.  I wrote some code. It seems to to work but it works sometimes incorrectly. It starts from case 2. 

I tested artificially using potentiometer. I rose up and down signal. 

 

void Set_case_Signal (void){
    static uint8_t state=0;    
switch (state){
        
        case 0 :  // start ticking every 1000ms
        if (Check_Gas_Detection_Voltage_Relay(Sensor_1)){  
              _delay_ms (300);
                  }
             state++;
        break;
        
        case 1 :   if(Check_Gas_Detection_Stop_Relay(Sensor_1)){
                        state=0;
                        }     
                        else {state ++;}       
                       break;
        
        case 2 :     if (Check_Gas_Detection_Voltage_Relay(Sensor_1)){
                     Buzzer_Tune1();
                     _delay_ms(40);
                     Sensors_Relay_Mechanism();
                     if(Check_Gas_Detection_Stop_Relay(Sensor_1)){
                         state=0;
                     }
                      else { state++;}
                     }
     
                     break;
        
        case 3 :    if (Check_Gas_Detection_Voltage_Relay(Sensor_1)){ 
                    Buzzer_Tune1();
                    _delay_ms(40);
                    if (Check_Gas_Detection_Stop_Relay(Sensor_1)){ state=0;}
                     else { state=3;}
                         }
                      else if(Check_Gas_Detection_Stop_Relay(Sensor_1)){
                          state=0;
                        }
                    break;
                    
        default: //trap error condition 
                  state=0;
        break;
              }

 

void Sensors_Buzzers_Mechanism (){

    Set_case_Signal();
            
      }
 

sadf

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You’ve written some code that is near unreadable, but where is the design? Are we supposed to figure out what you expect from reading the code?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Casper_0770 wrote:

... but it works sometimes incorrectly. It starts from case 2. 

 

How do you know that?

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
You’ve written some code that is near unreadable, but where is the design? Are we supposed to figure out what you expect from reading the code?

 

And as posted there is a missing closing brace somewhere.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I uploaded code to my hardware.

sadf

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Casper_0770 wrote:
I uploaded code to my hardware.

 

And.....?

 

Maybe your hardware doesn't work.

 

Let me rephrase my question...when the software doesn't work as you think it should, what do you observe to be able to say that case 2 is the first case executed?

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I also tested Proteus. My hardware is working. First case executed. It skips sometimes case0 and case1. When I give case1 condition, it is not returning case 0 sometimes. It is returning some times back.

sadf

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There could be a myriad of things happening. The code you’ve shown may be working exactly as you designed it. However, we have no idea of the magic functions you call because you’ve not shown them.

To debug such things i would normally use some printf()s to dump the state and other values to a terminal program. Hopefully this shows you what is actually happening and then you can fix it. Electrons are invisible - you need to make them visible otherwise you are blind. Proteus has tools to help with this.

Last Edited: Sun. Jan 13, 2019 - 02:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think you need to specify how the program flow should go.

 

What do we know?...

 

The first time 'Set_case_Signal ()' runs it will always run case 0

The second time it runs it will always run case 1

The third time it will run either case 0 or case 2 depending on what this function call returns, 'Check_Gas_Detection_Stop_Relay(Sensor_1)'

The fourth time, if it ran case 2 last time, will be either case 0, case 2 again, or case 3, depending on the result of your two function calls.

The fifth time, if it ran case 3 last time, it will run case 0 or stay in case 3, again depending on the return values.

 

We know that the switch statement works correctly. So the only thing that can affect the program flow is the return values from your other functions. So if the program flow is wrong then so are the return values from your functions.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh, and this topic is a perfect example of why some of us repeatedly tell people to design their programs on paper before they write a single line of code.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
You’ve written some code that is near unreadable,

Not necessarily - I think I've grasped Casper_0770's intention. You just have to guess what Check_Gas_Detection_Voltage_Relay() does. My guess is that it's comparing a voltage from his gas sensor with an alarm limit. Check_Gas_Detection_Stop_Relay() is no doubt similar, hopefully with some hysteresis.

 

Casper_0770; it can help hugely to name your states, I recommend an enumerated type for this: (NB: some statements removed for brevity}

typedef enum alarm_states {
  ALARM_IDLE,
  ALARM_DEBOUNCE,
  ALARM_ACTIVATE,
  ALARM_SOUND
} alarm_state_t;

void Set_case_Signal(void) {
  static alarm_state_t state = ALARM_IDLE;
  switch (state) {
    case ALARM_IDLE:  // start ticking every 1000ms
      ...
      break;

    case ALARM_DEBOUNCE:
      ...
      break;

    case ALARM_ACTIVATE:
      ...
      break;

    case ALARM_SOUND:
      ...
      break;

    default:  // trap error condition
      state = ALARM_IDLE;
      break;
  }

 

PS: I forgot to add you have mixed up states and transitions. Your state 1 is really a transition because your instrument spends no time in that state. We can leave that subtlety for another day however.

 

Last Edited: Sun. Jan 13, 2019 - 03:55 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dear Mr N.Winterbottom,

I wrote a code considering your advise (state machine). But it is not working. Maybe I missed something or library. Also, I considered timer flag, it is not working.

#define F_CPU 16000000UL
#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <time.h>
#include "greatlib.h"

uint8_t Channel=0;
uint16_t to;

int main(void)
{	
	//lcd_init();
	
	Buz_Tune();
	Init_BlinkingLED();
	ADC_Init();
	Blinking_delay ();
	//lcd_write_string_position("PPM1 PPM2 PPM3",0);
     Buzzer_Tune2();
	   /* Replace with your application code */
	  uint8_t timer_flag = 0;

	   void timer_isr (void)       //define global flag
	   {
		   //runs every 10ms
		   //reload timer
		   //set flag
		   timer_flag = 1;

	   }
     while (1)
    {
 if (timer_flag){
   
		 
		 Sensors_Buzzers_Mechanism();              // well....
  
         //clear the flag until next interrupt
        timer_flag = 0;
 }
 
	}
}

I wrote a code considering your advise (state machine). But it is not working.  It skips ALARM_IDLE,ALARM_DEBOUNCE and ALARM_ACTIVATE cases. It is working without delay. 

How to fix timer flag and consider timer a second?

void Set_case_Signal(void) {
	static alarm_state_t state = ALARM_IDLE;
	 static uint8_t timer = 5; 
	switch (state) {
		case ALARM_IDLE: if(Check_Gas_Detection_Voltage_Relay(Sensor_1)) { // start ticking every 1000ms
		           timer=5;
			 state=ALARM_DEBOUNCE; }
                         break;
		case ALARM_DEBOUNCE:               
		          state=ALARM_ACTIVATE;
		          break;

		case ALARM_ACTIVATE:
		timer--;
		 if(timer==1){
              state=ALARM_SOUND;
		 }
		 
		break;

		case ALARM_SOUND: if(Check_Gas_Detection_Voltage_Relay1(Sensor_2)){
                          Buz_Tune();
						  Sensors_Relay_Mechanism();      
                         
		} 
		                 state= ALARM_GAS;                    
		break;
        case ALARM_GAS: if(Check_Gas_Detection_Stop_Relay(Sensor_1)){state=ALARM_IDLE;}
			             else if (Check_Gas_Detection_Voltage_Relay(Sensor_1)){
							 Buzzer_Tune1();
							 Sensors_Relay_Mechanism();
							 } 
							 break;
		default:  // trap error condition
		state = ALARM_IDLE; //timer=5;
		break;
	}

}

 

 

sadf

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This is another example of the missing volatile keyword gotcha:

 

  uint8_t timer_flag = 0;

must be

 volatile uint8_t timer_flag = 0;

There are about a thousand posts on AVRfreaks on this requirement. The funny thing is that's probably not an exaggeration.

 

I'll see if I can find a good example. Unless another freak beats me. {to it}