ATmega8 4 LED Switch

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

Hello, I recently started learning micro controller programming in c. I made a simple 4 led switch, with a switch for each led. I tried writing code which turns on LED 3 when switch 3 is activated and turns it off when switch 4 is activated, till switch 3 is reactivated (SWITCH 4 TURNS LED 3 OFF WHILE SWITCH 3 IS SIMULTANEOUSLY ON). I made the following code, please take a look at it and help me 

 

 

#include<avr/io.h>   
#define F_CPU 100000UL	   
#include<util/delay.h>

void main(void)
{  //unsigned sw[1] = !(PIND & (1<<PD2)), ack[1] = !(PIND & (1<<PD3));
   DDRB = 0b00001111;   //PORTB output
   PORTB = 0x00;  //PORTB '0'
   DDRD = 0b00001111;   //PORTD input
   PORTD = 0xff;  //PORTD internall pullup
   
   while(1)
   {
		if(!(PIND & (1<<PD0))) PORTB = 0b00000001;    //when button pressed, turn ON led
		else PORTB = 0x00; 						    //when not pressed, turn OFF led
		
		if(!(PIND & (1<<PD1))) PORTB = 0b00000010;    //when button pressed, turn ON led
		else PORTB = 0x00; 						    //when not pressed, turn OFF led
		
		if( !(PIND & (1<<PD2)) && !(PIND & (1<<PD3)) ) PORTB = 0x00;    //when button pressed, turn ON led
		else PORTB = 0x00; 						    //when not pressed, turn OFF led
		
		if(!(PIND & (1<<PD2))) PORTB = 0b00000100;    //when button pressed, turn ON led
		else PORTB = 0x00; 						    //when not pressed, turn OFF led
		
		if(!(PIND & (1<<PD3))) PORTB = 0b00001000;    //when button pressed, turn ON led
		else PORTB = 0x00; 						    //when not pressed, turn OFF led
   }
} 

 

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

dhirenp03 wrote:
please take a look at it and help me

What do you expect to happen?  What >>is<< happening?

 

Are your switches connected active-high or active-low?

Are your LEDs connected active-high or active-low?

 

Buttons bounce.

 

You will need some masks to only look at and manipulate the bits for any particular test.  More than one of your tests can come true.

 

 

 

 

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

Making the pins connected to the buttons outputs is counter-productive.

Stefan Ernst

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

Making the pins connected to the buttons outputs is counter-productive.

More than that, it will eventually damage the AVR.

"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."

"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

What do you expect to happen?  What >>is<< happening?

 

Are your switches connected active-high or active-low?

Are your LEDs connected active-high or active-low?

 

Buttons bounce.

 

You will need some masks to only look at and manipulate the bits for any particular test.  More than one of your tests can come true.

 

I intend to turn off output from switch 3 when switch 4 is activated(while switch 3 is also active) and the led 3 goes off till switch 3 is turned off and back on again. So the switch 4 needs to be active-low 

to ground the output from switch 3.Please tell me whats wrong with the second IF-ELSE clause. 

 

Making the pins connected to the buttons outputs is counter-productive.

More than that, it will eventually damage the AVR. 

I intend to use it to with relays rather than switches, which shouldn't be a problem.  

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

I intend to use it to with relays rather than switches, which shouldn't be a problem.  

I'm sorry, but that makes no sense.

 

The issue is this:

   DDRD = 0b00001111;   //PORTD input

The four lower bits are '1', making those pins outputs, not inputs.  To make them inputs, leave them as '0'.

 

There is, I'm afraid, a great deal more wrong with your code as well.  For starters, you're making direct assignments to PORTB in order to turn the LEDs on and off, where you should be performing a read-modify-write operation, with |= to set a bit, and &= to clear a bit.  Simply assigning 0x00000001 to PORTB turns on LED 1, but turns off all of the other LEDs.  To turn LED 1 on without affecting the others, you need:

PORTB |= 0b00000001;

Similarly, assigning 0x00 to PORTB turns off not only LED1, but all LEDS.  To turn off only LED 1:

PORTB &= 0b11111110;

 

Have a read of this tutorial:

http://www.avrfreaks.net/forum/tut-c-bit-manipulation-aka-programming-101?page=all

 

There are still other issues with your code, but start there.  See what you can make of it, progress as far as you can and return here when you have other questions.

 

And welcome to AVR Freaks!

"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."

"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

Thanks a lot for your help. The code now looks as follows  

#include<avr/io.h>   
#define F_CPU 100000UL	   
#include<util/delay.h>

void main(void)
{ 
   DDRB = 0b00001111;   
   PORTB = 0x00;  
   DDRD = 0b00001111;  
   PORTD = 0xff;  
   
   while(1)
   {
		if(!(PIND & (1<<PD0))) PORTB |= 0x01;   
		else PORTB &= 0x00; 						    
	
		if(!(PIND & (1<<PD1))) PORTB |= 0x02;    
		else PORTB &= 0x00; 						   
		
		if(!(PIND & (1<<PD2))) PORTB |= 0x04;    
		else PORTB &= 0x00;
		
		if( !(PIND & (1<<PD3)) && !(PIND & (1<<PD4)) ) 
		  {
		   while(!(PIND & (1<<PD3)))
		    {PORTB &= ~(0x08<<PORTB);
			 
			}
		  } 
        if(!(PIND & (1<<PD4)))
		   {while(!(PIND & (1<<PD3)))
		     {if(!(PIND & (1<<PD3))) PORTB |= 0x08;} 
		   }
       
		if(!(PIND & (1<<PD3))) PORTB |= 0x08;   
		else PORTB &= 0x00; 						    
 
		
		
   }
} 

It works fine when individual switches are activated, but when multiple or rather all switches are active, a random unstable behavior is observed each time. Is it because of any time difference in execution of the parent while loop of the cpu frequency or any thing else? If there is some issue with the code please help.

 

There are still other issues with your code, but start there.  See what you can make of it, progress as far as you can and return here when you have other questions.

 

And welcome to AVR Freaks!

Thanks again. 

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

Each of your if/else statements is acting independently; therefore, when encountering the PORTB &= 0x00; statement, all bits are set to 0.

David (aka frog_jr)

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

You didn't do what I suggested:

Similarly, assigning 0x00 to PORTB turns off not only LED1, but all LEDS.  To turn off only LED 1:

PORTB &= 0b11111110;

 

Have you read the tutorial I linked to?

"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."

"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

Did you really mean to define your

#define F_CPU 100000UL

as 100KHz ?

Ross McKenzie ValuSoft Melbourne Australia

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

Did you really mean to define your

#define F_CPU 100000UL

as 100KHz ?

He's not using any delays or other time-sensitive code, so it's not an issue... yet.  It will come back to bite him, though ;-)

 

What is a problem... still... is that the switches on port D are still being made an output:

DDRD = 0b00001111

Did you understand what we meant when we said that was both wrong and very bad?

"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."

"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

You didn't do what I suggested:

Similarly, assigning 0x00 to PORTB turns off not only LED1, but all LEDS.  To turn off only LED 1:

PORTB &= 0b11111110;

 

Have you read the tutorial I linked to?

Yes tried what you said, LED  1 works fine but then all other LEDs stay on even if the switches are in off held down. 

 

Now the major issue is that when switch 4 is pressed, every 1 of 5 times it so happens that the Led 3 instead of going off, brightens up, which is very unstable.

Did you understand what we meant when we said that was both wrong and very bad?

No, I'm 17 and still learning, so if i'm still making a very "not to be made" mistake please do tell me. 

 

Each of your if/else statements is acting independently; therefore, when encountering the PORTB &= 0x00; statement, all bits are set to 0.

So, should I use Switch-Case statement  or is there some other method to tackle this. Thanks everyone for your time and help.

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

Let's forget about your special case with LED 3 and buttons 3/4.  Just get a 1-to-1 relationship with switches and buttons going.

 

You need to extrapolate from what I was saying and adjust the code for the other switches.

 

Here, I'll do the first two...:

		if(!(PIND & (1<<PD0))) PORTB |= 0x01;
		else PORTB &= 0xFE;

		if(!(PIND & (1<<PD1))) PORTB |= 0x02;
		else PORTB &= 0xFD;

... now you do the rest.  You see what I'm doing?

 

Once you've got this 1-to-1 relationship working, you can start adding changes to get your special handling of LED 3 and buttons 3/4.

 

I'd have to guess you still haven't read the tutorial link I posted.  I must say, if you continue to refuse to advice, I'm going to rapidly lose interest.

"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."

"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

You're getting me wrong,  I had already read the tutorial and tried what you suggested, but then it didn't seem to work out, maybe because i might have made a mistake. Now the 1-to-1 switching is working well. I still don't fully understand Bit masking so if I make a mistake, don't take it as that i haven't read the bit manipulation post.

#include<avr/io.h>   	   
#include<util/delay.h>

void main(void)
{ 
   DDRB  |= 0x0F;   
   PORTB |= 0x00;  
   DDRD  |= 0x0F;  
   PORTD |= 0xFF;  
   
   while(1)
   {
		if(!(PIND & (1<<PD0))) PORTB |= 0x01;
		else PORTB &= 0xFE;

		if(!(PIND & (1<<PD1))) PORTB |= 0x02;
		else PORTB &= 0xFD;
		
		if(!(PIND & (1<<PD2))) PORTB |= 0x04;    
		else PORTB &= 0xFB;
		
		if( !(PIND & (1<<PD3)) && !(PIND & (1<<PD4)) ) 
		  {while(!(PIND & (1<<PD3)))
		        { PORTB &= (0xF7<<PORTB); }
		  } 
		  
                if(!(PIND & (1<<PD4)))
		  {while(!(PIND & (1<<PD3)))
		        { if(!(PIND & (1<<PD3))) PORTB |= 0x08; } 
		  }
       
		if(!(PIND & (1<<PD3))) PORTB |= 0x08;   
		else PORTB &= 0xF7; 						    
   }
} 

The 4th switch still exhibits the same unstable behavior.

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

Remove the DDRD line. Reread the first part of post #6 to understand why.

Stefan Ernst

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

Some comments on your code:

   DDRB  |= 0x0F;   //  PORTB bits 0-3 output, bits 4-7 input
   PORTB |= 0x00;   //  Does nothing (OR with 0 does not modify any bits)
   DDRD  |= 0x0F;   //  PORTD bits 0-3 output, bits 4-7 input
   PORTD |= 0xFF;   //  PORTD bits 0-3 set high, bits 4-7 pullups active

Although in this case you are initializing the registers immediately following a reset, using the OR ("|") is ok; however, it is generally better not count on the settings of registers in case some previous function has set them outside your control. IIF you KNOW the registers configuration, then using OR or AND ("|" or "&") is fine.

 

dhirenp03 wrote:
Now the 1-to-1 switching is working well.
If you have a switch (for example) on PORTD bit 0 with the other side of the switch connected to GND then you are shorting the PORTD output since you have set PORTD bit 0 as output and set it to a high level. This is NOT the correct way to operate switches...

 

The following would be a better configuration if the switch is connected between the port pin and GND:

DDRD = 0x00;     // PORTD all bits input
PORTD = 0xFF;    // PORTD all pullups active

 

 

David (aka frog_jr)

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

The 4th switch still exhibits the same unstable behavior.

At this point there's no way to tell if that's because you're still trying hard to burn out your AVR with this:

   DDRD  |= 0x0F;  

It seems either you've not understood, or you have ignored, the advice you've been given.  As @frog_jr has re-iterated, it should be this:

   DDRD  |= 0x00;
   DDRD   = 0X00;  // (Thanks for the note @frog_jr!)

Period.  No ifs ands or buts.  Using relays (whatever that means) won't make a jot of difference.  You're still burning out your AVR.

 

However, you're handling the 4th LED three times, one right after the other, using different rules, each one overriding the previous.  Sounds like a recipe for conflict.

 

I'd also like to comment a bit on your code formatting.  While not important for the proper functioning of your code, formatting is >>very<< important to the humans reading it, trying to understanding it, or maintaining it.  Proper formatting can make a bug jump out at you.  Poor formatting can hide a bug from view.

 

Also, as a matter of form, try always to use braces for if/else expressions.  Although the language allows single-expression if and else clauses, they can later become the source of bugs when you modify a clause to contain more than one expression.  For example:

if (foo)
  this();
else
  that();

... is perfectly legal.  If you later decide to change that to:

if (foo)
  this();
else
  that();
  those();

...  you will not get what you expect.  Better to:

if (foo) {
  this();
}
else {
  that();
}

... and later:

if (foo) {
  this();
}
else {
  that();
  those();
}

 

So here's your code reformatted to be more readable to a human, with some comments added to describe what actually happens:

#include<avr/io.h>
#include<util/delay.h>

void main(void) {
  DDRB  |= 0x0F;
  PORTB |= 0x00;
  DDRD  |= 0x0F;  // <-- make the switches outputs (still wrong!)
  PORTD |= 0xFF;

  while(1) {

    // SW 1 and LED 1
    if (!(PIND & (1<<PD0))) { // <-- true if SW 1 is closed
      PORTB |= 0x01;          // <-- turn LED 1 ON
    }
    else {
      PORTB &= 0xFE;          // <-- turn LED 1 OFF
    }

    // SW 2 and LED 2
    if (!(PIND & (1<<PD1))) { // <-- true if SW 2 is closed
      PORTB |= 0x02;          // <-- turn LED 2 ON
    }
    else {
      PORTB &= 0xFD;          // <-- turn LED 2 OFF
    }

    // SW 3 and LED 3
    if (!(PIND & (1<<PD2))) { // <-- true if SW 3 is closed
      PORTB |= 0x04;          // <-- turn LED 3 ON
    }
    else {
      PORTB &= 0xFB;          // <-- turn LED 3 OFF
    }

    // Intended special handling of SW 3/4 and LED 3
    if ( !(PIND & (1<<PD3)) && !(PIND & (1<<PD4)) ) { // <-- true if both SW 4 and SW ?? are closed (what is on PD4?)
      while(!(PIND & (1<<PD3))) {                     // <-- true while SW 4 is closed
        PORTB &= (0xF7<<PORTB);                       // <-- turn LED 4 OFF
      }
    }
    // Continued intended special handling of SW 3/4 and LED 3, but this is a completely separate 'if' expression, independent of the previous one
    if (!(PIND & (1<<PD4))) {                         // <-- true if SW ?? is closed (what is on PD4?)
      while (!(PIND & (1<<PD3))) {                    // <-- true while SW 4 is closed
        if (!(PIND & (1<<PD3))) {                     // <-- true if SW is closed
          PORTB |= 0x08;                              // <-- turn LED 4 ON
        }
      }
    }

    // SW 4 and LED 4
    if (!(PIND & (1<<PD3))) { // <-- true if SW 4 is closed
      PORTB |= 0x08;          // <-- turn LED 4 ON
    }
    else {
      PORTB &= 0xF7;          // <-- turn LED 4 OFF
    }

  }

}

I would ask you to answer the question: "Is what is actually happening the same as what I want to happen?"

 

I would suggest you pay particular attention to your use of PD4, and I would ask you what is attached to it, and if perhaps you really meant to say PD4 and not PD2...?

 

You may also want to search for the phrase 'magic numbers' w.r.t. programming, and why you should avoid them.  Using them makes it very easy to overlook bugs.

 

EDIT:  code edit, thanks @frog_jr

"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."

"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]

 

Last Edited: Sun. Oct 1, 2017 - 05:40 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for all your help and advice. Here is the current working code.

#include<avr/io.h>
#include<util/delay.h>

void main(void) {
  DDRB  |= 0x0F;
  PORTB |= 0x00;
  DDRD  |= 0x0F;  
  PORTD |= 0xFF;

  while(1) {

    // SW 1 and LED 1
    if (!(PIND & (1<<PD0))) { // <-- true if SW 1 is closed
      PORTB |= 0x01;          // <-- turn LED 1 ON
    }
    else {
      PORTB &= 0xFE;          // <-- turn LED 1 OFF
    }

    // SW 2 and LED 2
    if (!(PIND & (1<<PD1))) { // <-- true if SW 2 is closed
      PORTB |= 0x02;          // <-- turn LED 2 ON
    }
    else {
      PORTB &= 0xFD;          // <-- turn LED 2 OFF
    }

    // SW 3 and LED 3
    if (!(PIND & (1<<PD2))) { // <-- true if SW 3 is closed
      PORTB |= 0x04;          // <-- turn LED 3 ON
    }
    else {
      PORTB &= 0xFB;          // <-- turn LED 3 OFF
    }
	
    // SW 4 and LED 4
    if (!(PIND & (1<<PD3))) { // <-- true if SW 4 is closed
      PORTB |= 0x08;          // <-- turn LED 4 ON
    }
    else {
      PORTB &= 0xF7;          // <-- turn LED 4 OFF
    }
	
    // Intended special handling of SW 4/5 and LED 4
    if ( !(PIND & (1<<PD3)) && !(PIND & (1<<PD4)) ) { // <-- true if both SW 4 and SW 5 are closed
      while(!(PIND & (1<<PD3)))                      //checks whether sw 4 is closed or not
           {
		    PORTB &= (0xF7<<PORTB);                // <-- turn LED 4 off if SW 5 is closed
            
			   // SW 1 and LED 1
            if (!(PIND & (1<<PD0))) { // <-- true if SW 1 is closed
				PORTB |= 0x01;          // <-- turn LED 1 ON
				}
			else {
				PORTB &= 0xFE;          // <-- turn LED 1 OFF
				}
			
			if (!(PIND & (1<<PD1))) { // <-- true if SW 2 is closed
                PORTB |= 0x02;          // <-- turn LED 2 ON
                }
            else {
               PORTB &= 0xFD;          // <-- turn LED 2 OFF
                 }

            // SW 3 and LED 3
            if (!(PIND & (1<<PD2))) { // <-- true if SW 3 is closed
             PORTB |= 0x04;          // <-- turn LED 3 ON
             }
            else {
              PORTB &= 0xFB;          // <-- turn LED 3 OFF
             }
			
			}                      
      
    }
  }
  }

Now, when switch 5 is closed and then switch 4 is closed then LED 4 should light up till switched 5 is released and closed again. I added the following IF clause, but then it overrides the function of switch 5 

        if(!(PIND & (1<<PD4)))
          {while(!(PIND & (1<<PD3)))
		{if( !(PIND & (1<<PD3))) PORTB |= 0x08; } 
          }

Other than that how do I make the micro controller unreadable?

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

dhirenp03 wrote:
DDRD |= 0x0F;

Can you show us some sort of schematic of how you have the switches wired to the uC, Please?

David (aka frog_jr)

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

Now, when switch 5

This is the first time you've mentioned a fifth switch.

 

And I guess it wasn't obvious enough the 7 previous times you've been told this, but:

DDRD  |= 0x0F;  // <-- still wrong!

You are very frustrating.

 

Other than that how do I make the micro controller unreadable?

OK, now I've completely lost you.

 

I think I'm going to have to take a break from this thread for a while and let others have a go.

"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."

"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]