Bitwise operations in IF statement not working properly

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

Hi everyone.

I'm new here and I'm not sure if this is the correct forum to ask these kind of questions, but here I go:

 

I'm working with an Arduino MEGA2560 and Atmel Studio, trying to make a digital stopwatch. At the moment I'm just trying to debounce via software the 3 buttons that I need to use (one for start/stop, one for increasing the time, and one for decreasing it). This is my code so far:

 

#include <avr/io.h>
#include <avr/interrupt.h>

void TC0_setup();
void main_setup();
void debounce();

uint8_t last_State = 1, current_State, flag_s1;
volatile uint32_t millisec = 0, last_s1, deb_Delay = 50;

int main(void)
{
main_setup();
    while (1)
    {
		debounce();
		if(flag_s1 & (1<<0)){
			PORTA ^= 0x1;
			flag_s1 ^=  (1<<0);
		}

		if((flag_s1 & (1<<1))){
			PORTA ^= 0x2;
			flag_s1 ^= (1<<1);
		}

    }
}

void TC0_setup(){
	//  T = (1 + OCR0A) * N * T_CLK => OCR0A = T * f_CLK / N - 1 Trabaja a 1 kHz
	TCCR0A = (1<<COM0A1)|(1<<COM0A0)|(1<<WGM01); // CTC
	TCCR0B = 0x03; // Prescaler  64
	TIFR0 = 0x00;
	OCR0A = 249;
        TIMSK0 = (1<<OCIE0A);
}

void main_setup(){
	DDRA = 0xFF;
	PORTA = 0x00;
	DDRC = 0x00;
	PORTC = 0xFF;
	TC0_setup();
	sei();
}

ISR(TIMER0_COMPA_vect){
	millisec++;
}

void debounce(){
	current_State = PINC;
	if(current_State != last_State){
		if(millisec - last_s1 > deb_Delay ){
			if(current_State > last_State){
				flag_s1 = current_State;
			}
			last_State = current_State;
			last_s1 = millisec;
		}
		}else{
		last_s1 = millisec;
	}
}

I'm using the Timer 0 to count milliseconds, just like the "millis()" function from Arduino. 

 

The idea is:

-When a button is pressed and released (the transition is "when current_State is greater than last_State) , the current state of PINC is saved into last_s1, then I test for 1's in last_s1 and turn on a LED connected to PORTA, and then erase the bit in last_s1 so it won't execute the same if more than once. 

 

The problem is that the code is not working. Any button in PINC turns on and off the two LED connected to PORTA at the same time, not independently like it's intended, and since I'm new to programming for an AVR microcontroller outside Arduino IDE, I'm not sure what I'm doing wrong.

 

Is there something wrong with my code? 

 

 

 

 

 

 

This topic has a solution.
Last Edited: Mon. Oct 7, 2019 - 04:05 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I didn't look in detail, but it seems you are not initializing flag_s1.

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

I tried initializing it, but it still doesn't work.

 

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

For debounce to be effective, you need to keep a count. One simple method is to use a downcounter. If the button is not pressed, the downcounter = 50. If the button is pressed and the downcounter is not zero, decrement the downcounter. If downcount is equal to 0, the button is pressed and debounced. Do this in your timer isr.

Another issue is you do not read the variable millisec atomically. If you look at the Aeduino core code for millis() you’ll see they disable interrupts, copy the value,re-enable the interrupts and return the value. This is done to ensure the four bytes of the millisecond count are read as one. If you don’t do this, the timer isr can interrupt whilst the four bytes are being read anf later them. What you end up with is some new bytes and some old bytes.

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

Thanks for the tip about millisec not being read atomically, I've changed my code to fix that.

As for the debouncing, when I'm using only one button it works, so I'm thinking that the issue is not the debouncing itself, but maybe a problem in my logic when I evaluate the content of the flag variable, or something about bitwise operations I'm not understanding correctly. I'm going to try the downcounter anyway, just to be sure.

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

I'd have to get pen and paper to work through your debounce logic. Using > seems suspect - I can't see how this would work for more than one button. I don't think the issue is with bit wise operations.

 

Write simple code - the defects tend to be simple to fix. 

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

I've changed the code to this:

 

 

#include <avr/io.h>
#include <avr/interrupt.h>


void TC0_setup();
void TC1_setup();
void main_setup();
void debounce();


volatile uint8_t first_D, second_D, fors;
uint8_t last_State = 1, current_State, flag_s1 = 0x0;
volatile uint32_t millisec = 0, last_s1, last_s2, last_s3, deb_Delay = 50;

int main(void)
{
	main_setup();
    while (1) 
    {

		debounce();
		
		if(flag_s1 & (1<<0)){
			PORTA ^= 0x1;
			flag_s1 ^=  (1<<0);
		}
		
		if((flag_s1 & (1<<1)) ){
			PORTA ^= 0x2;
			flag_s1 ^= (1<<1);
		}
		
			
		if((flag_s1 & (1<<2)) ){
			PORTA ^= 0x4;
			flag_s1 ^= (1<<2);
		}
    }
}


void TC0_setup(){
	//  T = (1 + OCR0A) * N * T_CLK => OCR0A = T * f_CLK / N - 1
	TCCR0A = (1<<COM0A1)|(1<<COM0A0)|(1<<WGM01); //  CTC
	TCCR0B = 0x03; // Prescaler 64
	TIFR0 = 0x00;
	OCR0A = 249; 
	TIMSK0 = (1<<OCIE0A);
	
}

void TC1_setup(){
	//  OCR1A = T * f_CLK / N - 1  Trabaja a 1 Hz
	TCCR1A = 0;
	TCCR1B = (1<<CS12)|(1<<CS10)|(1<<WGM12); 
	TCCR1C = 0x0;
	TIFR1 = 0x0;
	OCR1A = 15624; 
	TIMSK1 = (1<<OCIE1A);
}


void main_setup(){
	DDRA = 0xFF;
	PORTA = 0x00;	
	DDRC = 0x00;
	PORTC = 0xFF;
	TC0_setup();
	TC1_setup();
	sei(); 
}




ISR(TIMER0_COMPA_vect){
	millisec++;
}


void debounce(){
	current_State = PINC;  
	
	if((current_State&1) != (last_State&1)){
		if(ms() - last_s1 > deb_Delay ){
			if((current_State&1) != 0){
				flag_s1 |= (1<<0);
			}
			last_State = current_State;
			last_s1 = ms();
		}
		}else{
		last_s1 = ms();
	}
	
	if((current_State&(1<<1)) != (last_State&(1<<1))){
		if(ms() - last_s2 > deb_Delay ){
			if((current_State&(1<<1)) != 0){
				flag_s1 |= (1<<1);
			}
			last_State = current_State;
			last_s2 = ms();
		}
		}else{
		last_s2 = ms();
	}
	

	if((current_State&(1<<2)) != (last_State&(1<<2))){
		if(ms() - last_s3 > deb_Delay ){
			if((current_State&(1<<2)) != 0){
				flag_s1 |= (1<<2);
			}
			last_State = current_State;
			last_s3 = ms();
		}
		}else{
		last_s3 = ms();
	}

}




And now the debouncing works for all the buttons, but for some reason the LED that should turn on and off with the second button always starts on. I tried in the simulator of Atmel Studio and at the start the program jumps at the line "PORTA ^= 0x2;" of the second if statement even when flag_s1 is all 0, turning on just that LED. I don't understand why would the program do that. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
last_State = current_State;

you have this repeated three times. The first time it works, the other two, not so. This should be the last line in the function. Your debounce still sucks.

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

While I haven't looked into what you are seeing I do have some observations about your code.

 

  1. millisec is a 32-bit variable that is shared between the main thread of execution and an ISR. Interactions with this variable are not atomic which could lead to problems. Take a look at util/atomic.h.
  2. The first_D, second_D, and fors variables are unused. You should probably increase your warning levels (-Wall, -Wextra, and -Wpointer-arith are good starting points), and treat warnings as errors (-Werror).
  3. The last_s1, last_s2, and last_s3 variables are only used by the debounce() function. These variables do not need to be volatile. Also, declare them as static variables inside the debounce function instead of as globals.
  4. The current_State variable is only used by the debounce() function. It should not be a global.
  5. The ms() function seems to have not been declared/defined.
  6. The usage pattern of flag_s1 is suspect. It probably makes more sense to pass information to debounce() and have debouce() return information instead of using a global.

github.com/apcountryman/build-avr-gcc: a script for building avr-gcc

github.com/apcountryman/toolchain-avr-gcc: a CMake toolchain for cross compiling for the Atmel AVR family of microcontrollers

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

Thanks for the reply. About 1 and 5, I didn't copy all the code apparently, that's muy fault.

I'm going to change the way I debounce the buttons, to see if I could make it work.

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

Apparently it was the initialization of last_State what was making the LED to be on at start. I've change that and some variables to local as apcountryman suggested and now everything works. Thanks everyone!