Beating my head against the wall! Why doesn't this work?

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

ATMega48. Having some issues with boolean comparisons, I guess. This will work for the first state change, either going into programming mode or into calibration mode - but will never exit either mode. Can anyone tell me what I'm doing wrong?

Thanks!

#include 
#include 
#include 
#include 
#include 
#include 
#include 

uint16_t volatile PD0downcounter;
uint16_t volatile PD1downcounter;
bool volatile programming_mode = false;
bool volatile calibration_mode = false;
bool volatile PD0down = false;
bool volatile PD1down = false;


ISR(__vector_default)
{

}

ISR(PCINT2_vect)
{
	if (~PIND & (1 << PD0))
	{
		PD0down = true;
	} else {
		if (PD0downcounter > 5000)	// If switch is depressed for more than 5000 cycles, toggle programming mode
		{
			if (calibration_mode == false)
			{
				if (programming_mode == false)
				{
					programming_mode = true;
					PORTD = (1 << PORTD5);
				} else {
					programming_mode = false;
					PORTD = 0;
				}
			}
		} else if (PD0downcounter > 200) {
			// Other operation
		}
		PD0down = false;
		PD0downcounter = 0;
	}

	if (~PIND & (1 << PD1))  		// If switch is pressed
	{
		PD1down = true;
	} else {
		if (PD1downcounter > 5000)	// If switch is depressed for more than 5000 cycles, toggle calibration mode
		{
			if (programming_mode == false)
			{
				if (calibration_mode == false)
				{
					calibration_mode = true;
					PORTD = (1 << PORTD6);
				} else {
					calibration_mode = false;
					PORTD = 0;
				}
			}
		} else if (PD1downcounter > 200) {
			// Other operation
		}
		PD1down = false;
		PD1downcounter = 0;
	}


}

/*-----------------------------------------------------------------------------------------------
	Main program
-----------------------------------------------------------------------------------------------*/

int main(void) {
	
	DDRD = (1 << DDD7)|(1 << DDD6)|(1 << DDD5);		//Set PIND7:5 to output (status LEDs)
	PORTD |= (1 << PORTD0)|(1 << PORTD1);			//Enable pull-up resistors for PORTD1:0 (input switches)
	
	PCMSK2 |= (1 << PCINT16)|(1 << PCINT17);		//Enable interrupts on PD0 and PD1 (input switches)
	PCICR |= (1 << PCIE2);							//Enable pin change interrupt 2

	
	
	for (;;)										//Main program loop
	{										
		cli();	// Disale interrupts while processing volatile 16-bit integers

		if (PD0down == true)
		{
			PD0downcounter++;
		}

		if (PD1down == true)
		{
			PD1downcounter++;
		}

		sei();	// Re-enable interrupts
	}
}

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

I think, you should place the code

 PD0downcounter = 0; 

into the corresponding "if" statement. As you have it now, this line will always be executed, regardless whether you have exceeded the 5000 limit or not.
Also, why don't you put your

if (PD0down == true)
      {
         PD0downcounter++;
      } 
if (PD1down == true)
      {
         PD1downcounter++;
      } 

into the interrupt routine? You could save two if statements...
Zoltan

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

Quote:
Also, why don't you put your ... into the interrupt routine?

Because he is counting how long it is down, not how many times it goes down.

Regards,
Steve A.

The Board helps those that help themselves.

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

Quote:
Quote:
Also, why don't you put your ... into the interrupt routine?

Because he is counting how long it is down, not how many times it goes down.


Sorry, that's my mistake. I have overlooked it. :oops:
Zoltan

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

zoltanvoros wrote:
I think, you should place the code

 PD0downcounter = 0; 

into the corresponding "if" statement. As you have it now, this line will always be executed, regardless whether you have exceeded the 5000 limit or not.

What I'm trying to do is put it under the "else" of whether or not the pin is down, which resets the counter to 0 whenever the pin change interrupt is fired but the switch is open (should be when the pin goes from low to high). This action (theoretically) should reset the counter after taking the appropriate action (based on how long the switch was held closed).

Thanks for the help so far...

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

Anyone else have any ideas??

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

Not sure what it will help, but I would put a delay of some sort after the sei. Otherwise the global interrupt flag is enabled only for the duration of the presumed relative jump to the cli instruction.

C: i = "told you so";

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

Psychlow wrote:
Anyone else have any ideas??

Drink beer or tea according to preference.

Use a timer for the IRQ. Increment a counter for the down period of your switch, and perform simple debouncing. This will give meaningful times e.g. a 10mS IRQ with a count of 500 will give you a 5 sec push.

Put all your decision logic in your foreground routines.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
               PORTD = (1 << PORTD5);

Turns off your pullups. Do you have external pullups on your switches?

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

PORTD5:7 Are status LEDs - the inputs are on PORTD0:PORTD1 only.

I am doing as David has suggested and am counting the switch press time with a timer IRQ. So far, this is working well - I'll post revised code once I get it tested thoroughly. I'm still stumped, though, as to why the code above just stops working after one mode change.

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

The line that mckenney quoted will turn off the pullups on your switches. Use an OR to preserve the other bits in PORTD.

PORTD |= (1 << PORTD5); 

There are a few other similar lines in the code that also write the entire PORTD when you probably intended to change only one bit.

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

Aha! Thank you both, I didn't understand why at first. :mrgreen::mrgreen:

That helps quite a bit - I'll check for other logic errors such as this one. I think that is exactly what the problem has been!