View previous topic :: View next topic |
Author |
Message |
rems
Joined: 02 Feb 2012 Posts: 56
|
Debounce routine not working |
Posted: Sat Mar 31, 2012 7:54 pm |
|
|
PIC16F887 //EDIT--The code I am trying is actually in the 3rd post. The following will not return a value as it is marked as "void" my mistake
I have a very bouncy switch in dire need of debouncing. I am trying the following to remedy it with out success.
Code: |
#define DEBOUNCE_PERIOD_IN_MS 10
#define DEBOUNCE_COUNT 2
void wait_for_keypress(void)
{
int count = 0;
while(1)
{
if(!input(PIN_B2) == 1)
count++;
else
count = 0;
if(count == DEBOUNCE_COUNT)
break; // I want to return to "scanfaces" and execute my "bitset"
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
int8 scanfaces(void)
{
int led_output=0;
int face;
if(!input(pin_B2)) { // Checking Face 1
wait_for_keypress();
//I want to debounce here and then return to execute below.
bit_set(led_output,0);
}
else{
bit_clear(led_output,0);
}
|
This doesn't seem to be working. It compiles fine but.... I've even tried to return a value to "wait_for_keypress" and then use that value as a signal to execute, as in the following
Code: |
void wait_for_keypress(void)
{
int key_confirmed=0;
int count = 0;
while(1)
{
if(!input(PIN_B2) == 1)
count++;
else
count = 0;
if(count == DEBOUNCE_COUNT){
Key_confirmed=1;
}
else{
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
return(key_confirmed);
}
int8 scanfaces(void)
{
int led_output=0;
int face;
if(!input(pin_B2))
wait_for_keypress();
if(wait_for_keypress()==1)
bit_set(led_output,0);
else{
bit_clear(led_output,0);
}
|
Does anyone have any advice or direction? I've been at it all day. I am not very experienced but am picking things up at a reasonable pace and am learning so much. I am very excited and truly have a passion for this.
Last edited by rems on Sat Mar 31, 2012 9:09 pm; edited 1 time in total |
|
|
PCM programmer
Joined: 06 Sep 2003 Posts: 21708
|
|
Posted: Sat Mar 31, 2012 8:40 pm |
|
|
Quote: |
void wait_for_keypress(void)
{
return(key_confirmed);
}
|
How do you return something when the function is declared as void ?
You are telling the compiler that it returns nothing.
I'm not convinced you even tested this code, because if I do a test
compile, I get this error message:
Quote: |
Error 127 "PCM_Test.c" Line 10(1,7): Return value not allowed in void function |
|
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Sat Mar 31, 2012 9:04 pm |
|
|
You are absolutely correct. I copied the wrong code out. The following is what I use and does in fact compile with no errors. I added a bitset to it.
Code: |
int8 wait_for_keypress_1(void)
{
char count=0;
int button_press;
int key_confirmed=0;
while(1)
{
if(input(pin_B2)==1)
count++;
if(count == DEBOUNCE_COUNT)
{
bit_set(key_confirmed,0);
}
else{
delay_ms(DEBOUNCE_PERIOD_IN_MS);
bit_clear(key_confirmed,0);
}
if (key_confirmed==0b00000001)
button_press=1;
return(button_press);
}
}
int8 scanfaces(void)
{
int led_output=0;
int face;
OUTPUT_low(pin_B1); // Configuring cage to sense faces
if(!input(pin_B2)) // Checking Face 1
wait_for_keypress_1();
if(wait_for_keypress_1()==1){
bit_set(led_output,0);
}
else{
bit_clear(led_output,0);
}
|
Of course this is followed by main which runs the "scanfaces" function along with other things not needed for the sake of my question. I apologize for the "fools mission" I sent you on due to my error in copying. |
|
|
PCM programmer
Joined: 06 Sep 2003 Posts: 21708
|
|
Posted: Sat Mar 31, 2012 10:29 pm |
|
|
My comments are placed above each section of code.
Code: |
int8 wait_for_keypress_1(void)
{
char count=0;
int button_press;
int key_confirmed=0;
while(1)
{
|
Below, you are incrementing a counter if the switch pin reads a
a high level. But in your schematic here, from a previous thread,
http://i1249.photobucket.com/albums/hh515/remsphotos/ccs_pic_schematic.jpg
you have pull-ups on the switch input pins to the PIC. You don't show it
in the schematic but I assume the bottom sides of the switches are tied
to ground. Are they ? If they are, then the switches are "active low".
So, when they're pressed, you should be doing the counting. So you
should be looking for a 0 level in the code below, not a 1.
Code: |
if(input(pin_B2)==1)
count++;
|
This section below is needlessly complex. Look at the original code
again. No need to add complexity and additional variables.
Code: |
if(count == DEBOUNCE_COUNT)
{
bit_set(key_confirmed,0);
}
else{
delay_ms(DEBOUNCE_PERIOD_IN_MS);
bit_clear(key_confirmed,0);
}
if (key_confirmed==0b00000001)
button_press=1;
|
Below, you have a return statement at the end of the while() loop.
That means the while() loop will return immediately after the first pass.
It will only do one pass. I'm sure you don't intend that. The original
code had a test, and used a 'break' statement to break out of the
while() loop if the test condition was satisfied.
Code: |
return(button_press);
}
} |
|
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Sun Apr 01, 2012 6:49 am |
|
|
I can't believe I forgot my exclamation mark
"if(!input(pin_B2)==1)"
I know the code is probably needlessly complex. I don't know enough to make it simple. I figured I didn't need the "bitset" but when it wasn't working, I thought I'd better try something that I have seen work before. Meanwhile, I had made a silly mistake the whole time.
Thank you for pointing out my blunders. I learned a lot. I will go back to the original and try again. I just wasn't sure if the "break" would go back to where I wanted it to. So I took it out and tried using the "return" Now, I realize it will contradict the "while".
I will keep trying. Thank you so much for your time and advice. |
|
|
ckielstra
Joined: 18 Mar 2004 Posts: 3680 Location: The Netherlands
|
|
Posted: Sun Apr 01, 2012 7:32 am |
|
|
rems wrote: | I can't believe I forgot my exclamation mark
"if(!input(pin_B2)==1)"
I know the code is probably needlessly complex. I don't know enough to make it simple. | Just a small suggestion to make things more simple is in the line referenced here. In my experience humans have troubles with reading logical NOT instructions, so your code would be easier to read when you change it to: Code: | if (input(PIN_B2) == 0) | Functionality of this code is exactly the same but now it easier to see you are testing for a zero value (instead of 'not 1').
Note also that I changed the capitalization of pin_B2 to PIN_B2. It is good coding practice to write constants in all upper case letters. In fact, that is how this value was defined in the header file. You code compiles because the CCS compiler is case insensitive by default, this is different from the C standard and can be changed with the #case precompiler directive. |
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Tue Apr 03, 2012 10:52 pm |
|
|
I appreciate the advice.
I have my debounce to where I think it really should be working but it isn't.
Does anyone have any thoughts or suggestions. The following is an excerpt from my code.
Code: |
#define DEBOUNCE_PERIOD_IN_MS 10
#define DEBOUNCE_COUNT 2
void wait_for_keypress_1(void)
{
int count=0;
while(1)
{
if(!input(PIN_B2)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
void wait_for_keypress_2(void)
{
int count=0;
while(1)
{
if(input(PIN_B3)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
int8 scanfaces(void)
{
int led_output=0;
int face;
// ************************************Configuring cage to sense faces
output_low(PIN_B1);
//************************************ Checking Face 1
if(input(PIN_B2)==0) {
wait_for_keypress_1();
bit_set(led_output,0); // will the "break;" come back to this line?
}
else {
bit_clear(led_output,0);
}
//************************************ Checking Face 5
if(input(PIN_B3)==0) {
wait_for_keypress_2();
bit_set(led_output,1); // will the "break;" come back to this line?
}
else{
bit_clear(led_output,1);
}
}
|
Will the "break;" automatically bring my program back to where it left? ie will the next instruction be my "bitset" ?
Is there something better that I should be looking at? |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1345
|
|
Posted: Wed Apr 04, 2012 6:17 am |
|
|
I don't expect wait_for_keypress_1() to work well the way you have it. Prior to calling it, you check:
Code: |
if(input(PIN_B2)==0)
|
Then in the function, you increment count if:
Code: |
if(!input(PIN_B2)==0)
|
The two if statements don't match, so I would expect a gnd'ed button to go into this and never fully increment the count in wait_for_keypress_1() |
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Wed Apr 04, 2012 6:32 am |
|
|
It still doesn't work. I am concerned it may be due to how I think the "break;" will perform. Will it break to where I think?
void wait_for_keypress_1(void)
{
char count=0;
while(1)
{
if(input(pin_B2)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
void wait_for_keypress_2(void)
{
char count=0;
while(1)
{
if(input(pin_B3)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
int8 scanfaces(void)
{
int led_output=0;
int face;
// ************************************Configuring cage to sense faces
output_low(PIN_B1);
//************************************ Checking Face 1
if(input(PIN_B2)==0) {
wait_for_keypress_1();
bit_set(led_output,0); // will it break to here?
}
else {
bit_clear(led_output,0);
}
//************************************ Checking Face 5
if(input(PIN_B3)==0) {
wait_for_keypress_2();
bit_set(led_output,1); // will it break to here?
}
else{
bit_clear(led_output,1);
}
Thank you for your help |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1345
|
|
Posted: Wed Apr 04, 2012 6:49 am |
|
|
All break does is take you out of the most current loop or switch statement. So it will simply jump to right outside your loop and continue from there. |
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Wed Apr 04, 2012 6:56 am |
|
|
Okay, Great...I get it...So no wonder it isn't working...So it would just start "scanfaces" again and go back to the "keypress" function. Just a big loop.
Maybe I should have the "wait_for_keypress" function return a value and then check for that value in my "scanfaces" function? But then I couldn't loop my _wait_for_keypress" function with the "while(1)". Would a "for" loop be better in my "wait_for_keypress" function to work with some sort of "return"? |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1345
|
|
Posted: Wed Apr 04, 2012 7:23 am |
|
|
The benefit of how you did it is that you skip the last delay_ms() call when you have a good number.
Just for clarity's sake, this is a visual of what break does:
Code: |
while(TRUE){
if(input(PIN_B2) == 0){
count++;
}else{
count = 0;
}
if(count == DEBOUNCE_COUNT){
break;
}
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
/* BREAK JUMPS YOU TO THIS SPOT */
|
Since you didn't provide any code other than your scanfaces(), wait_for_keypress_1(), and wait_for_keypress_2() methods, I can't really comment on the rest of your inquiry.
The way I understand how your code works is that you just loop infinitely until you get a debounced low. Is that the intention? |
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Wed Apr 04, 2012 7:46 am |
|
|
I have a very bouncy switch. It is homeade and literally has a ball within a cage that has 6 possible positions to be in. I need to debounce each of those positions. When the ball is in position, an i/p is sensed "LOW" and the debounce should start. In the debounce, I want to check and recheck that the i/p is still low. I may need to do this several times or just increment the delay between checks. Like I said it's a very bouncy switch.
Here is my complete code.
Code: |
#include <16F887.h>
#fuses INTRC_IO //use internal oscillator
#fuses NOWDT //No watch dog timer
#fuses PUT//
#fuses BROWNOUT//reset chip if voltage falls
#fuses MCLR//master clear enabled
#fuses NOLVP
#use delay(clock=8000000)
#define DEBOUNCE_PERIOD_IN_MS 100
#define DEBOUNCE_COUNT 2
#define resbutt pin_B5 //restart 'c' condition
//if condition is obtained turn on led
#define sleepbutt pin_D7
//**********************************Debounce Functions
void wait_for_keypress_1(void)
{
int count=0;
while(1)
{
if(input(pin_B2)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
void wait_for_keypress_2(void)
{
int count=0;
while(1)
{
if(input(pin_B3)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
void wait_for_keypress_3(void)
{
int count=0;
while(1)
{
if(input(pin_B4)==0)
{
count++;
}
else{
count=0;
}
if(count == DEBOUNCE_COUNT)
break;
delay_ms(DEBOUNCE_PERIOD_IN_MS);
}
}
/*****************************************************************Declare a function that will SCAN the faces of the puzzle box*******************/
int8 scanfaces(void)
{
int led_output=0;
int face;
// ************************************Configuring cage to sense faces
OUTPUT_low(PIN_B1);
//************************************ Checking Face 1
if(input(PIN_B2)==0) {
wait_for_keypress_1();
bit_set(led_output,0);
}
else {
bit_clear(led_output,0);
}
//************************************ Checking Face 5
if(input(PIN_B3)==0) {
wait_for_keypress_2();
bit_set(led_output,1);
}
else{
bit_clear(led_output,1);
}
//*************************************** Checking Face 4
if(input(PIN_B4)==0) {
wait_for_keypress_3();
bit_set(led_output,2);
}
else{
bit_clear(led_output,2);
}
// **************************************Configuring cage to sense second 3 i/p's
OUTPUT_low(pin_B2);
//*************************************** Checking Face 2
if(input(PIN_B3)==0) {
wait_for_keypress_2();
bit_set(led_output,3);
}
else{
bit_clear(led_output,3);
}
//**************************************** Checking Face 6
if(input(PIN_B4)==0) {
wait_for_keypress_3();
bit_set(led_output,4);
}
else{
bit_clear(led_output,4);
}
//****************************************** Configuring cage to sense last i/p
OUTPUT_LOW(PIN_B3);
//****************************************** Checking Face 3
if(input(PIN_B4)==0) {
wait_for_keypress_3();
bit_set(led_output,5);
}
else{
bit_clear(led_output,5);
}
//*********************************************Making 6 unique possibilities
if (led_output==0b00000001)
face=1;
if (led_output==0b00001001)
face=2;
if (led_output==0b00100000)
face=3;
if (led_output==0b00110100)
face=4;
if (led_output==0b00001010)
face=5;
if (led_output==0b00110000)
face=6;
return(face);
}
//*****************************************Password = 512346
void main()
{
int sh1=1,sh2,sh3,sh4,sh5,sh6; // sw1=1 for starting first condition.
while(1){
if(scanfaces()==5&&sh1==1) //if butt 1 = 1 AND sw1=1(which it does to start) then
sh2=1; //Allow second condition
if(scanfaces()==1&&sh2==1)//if butt 2=1 AND sw2=1(it only will if butt 1 was pressed) then
sh3=1; //Allow third condition
if(scanfaces()==2&&sh3==1) //if the three condition are obtained turn the led on.
sh4=1;
if(scanfaces()==3&&sh4==1) //if the three condition are obtained turn the led on.
sh5=1;
if(scanfaces()==4&&sh5==1) //if the three condition are obtained turn the led on.
sh6=1;
if(scanfaces()==6&&sh6==1) //if the three condition are obtained turn the led on.
{
output_high(pin_C5);
sh6=0;
}
else if(scanfaces()==6&&sh6!=1) //if the algorithm is not executed wait for restart. (if butt 3=1 and sh3 does NOT =1 then )
{
sh1=0; //set all to zero
sh2=0; //which ensures that only the proper combo(sequence) will activate
sh3=0;
sh4=0;
sh5=0;
sh6=0;
}
if(input(resbutt)==1) //restart entering sequence of buttons. if reset button pushed
{
output_low(pin_C5); //led will turn off
sh1=1; //allow condition start again
}
}
}
|
Because my "cage switch" shares common pins, I use my "scanfaces" to toggle i/p's and o/p's. I then give each a unique ID with the "bitset" and then return the value of each for use in the state machine, in my main program.. All works beautifully except of course for the debounce. |
|
|
RF_Developer
Joined: 07 Feb 2011 Posts: 839
|
|
Posted: Wed Apr 04, 2012 9:39 am |
|
|
The basic idea is that your main while loop just goes round and round. It checks if things need doing: it checks buttons, it checks for incoming data that needs to be processed, it measures, it outputs stuff and so on. None of these things should ever wait anything other than a few tens of us or so. Instead they check and if nothing to do they do nothing in that loop.
Most of the time your main loop does nothing much. Only if something needs servicing: display needs updating, someones pressed a button, a message had been receved, a timed event has been flagged up and so on; does your main loop actually do any processing. That means you'll check (or poll) everything on every loop.
So. you never "wait for a key press". You note a key has been pressed; if so do stuff and set a counter so that the keys will be ignored for say 50ms worth of loops before looking at the keys again. Longer for your caged ball switch. Using a timer, or clock tick based software timer will help there.
With debouncing there actually isn't much point in checking the contact is still where you first detected it after a time. The bounce happens when a contact is first made or broken and then for a while un-makes or un-breaks repeatedly. So only the first detection is relevant, then hold off from making further detections for at least the longest time you'll get bounce, which for some switches (and it varies from switch to switch of one type and from one type of switch to another) may be longer than 100ms.
Your caged ball will be tricky as normal switches work with some form of spring arrangement that biases them in to on position or another. The ball however relies on gravity alone, and it may get dirty and not make good contact. Have you designed the ball switch to be break-before-make? If so then you should be able to check it goes to a "none connected" state between one face and another and so, if it does go into the "none" state and back then nothing has really happened... or has it? How you interpret that is dependant on your application. You almost ceratainly can't do that if you are multiplexing the pins with another function.
This is the simplest form of "co-operative multitasking". Where each thing that has to be done: e.g. detect keys, update display, read serial, measure voltages, send formatted data, is regarded as a seperate task and your loop is the most simple form of round-robin scheduler. No "task" should ever wait if it can help it, and certainly not if it has nothing to do. Even ADC reads can, if required, be started on on loop and read back on the next, eliminating the need to wait for the ADC to convert. I have one simple app that does this. Though I confess on my others waiting for the ADC typically forms the greatest single time in the overall loop execution time.
RF Developer |
|
|
rems
Joined: 02 Feb 2012 Posts: 56
|
|
Posted: Thu Apr 05, 2012 6:50 am |
|
|
A lot to think about...That is some great stuff for me consider. With a lot of hard work and dedication to this stuff, I will eventually understand and appreciate everything you wrote.
I will keep at it.
Thank you for your insight. |
|
|
|