Hello, I have beginners problem with getting to work volatile variable. Trying to get reusable function for debouncing and detecting key_up state for all declared keys but can't get it to work as expected. Situation is as follows:
declaration:
sbit key1=P3^7; sbit key2=P3^6; bit key_up(volatile bit key);
usage:
if (!key_up(key2)) { green=~green; } if (!key_up(key1)) { ... do something... {
function:
bit key_up(volatile bit key) { if (key==0) // check if key is pressed { delay1ms(20); // debounce if (key==0) // really pressed { while(key==0) // hold until pressed { ; } // key is released delay1ms(20); // debounce when key released if (key!=0) // check if still released { key=1; // reset key return 0; // return key is up } } } return 1; // key was not pressed at all }
Program compiles with no errors but problem is that variable 'key' is not rechecked inside a function and I think it should be if it is declared 'volatile'. Please help to get my function working.
I find it counter-productive trying to get the code to work without trying to understand how every bit of code works. Obviously, you lack understanding of basic concepts of the programming language. Yet you want us to provide you with a working example. Sorry, I'm not interested in that.
@Mike, really no need for that. I can leave here well tested and fully workable example of multipurpose, multikey checking key_release routine with advanced double debouncing written in the best manner which C51 with 8051 can do! For users which may have similar situation in the future.
sbit key1=P3^7; sbit key2=P3^6; bit debounce(unsigned char AdditionalSemaphore);
if (!debounce('2')) { green=~green; } if (!debounce('1')) { ... on_key1_up do something...
bit debounce(unsigned char AdditionalSemaphore) { bit MagicTunnelToAlreadyDefinedVariable; if (AdditionalSemaphore=='1') MagicTunnelToAlreadyDefinedVariable=key1; if (AdditionalSemaphore=='2') MagicTunnelToAlreadyDefinedVariable=key2; if (MagicTunnelToAlreadyDefinedVariable==0) { delay1ms(20); if (MagicTunnelToAlreadyDefinedVariable==0) { while(MagicTunnelToAlreadyDefinedVariable==0) { if (AdditionalSemaphore=='1') MagicTunnelToAlreadyDefinedVariable=key1; if (AdditionalSemaphore=='2') MagicTunnelToAlreadyDefinedVariable=key2; } delay1ms(20); if (MagicTunnelToAlreadyDefinedVariable!=0) return 0; } } return 1; }
Not nice, really, but works.
Works?
How would your "magic tunnel" change value between the two times you use it?
if (MagicTunnelToAlreadyDefinedVariable==0) { delay1ms(20); if (MagicTunnelToAlreadyDefinedVariable==0) <= still same variable with zero interaction with hardware ... }
Next thing - your loop locks up the main code while one single button is pressed. That while loop will stay until released. Let's say one button gums up - then your program will permanently lock up.
Next thing - your code only manages one single button at a time. But if you have dedicated processor pins for each button, it's likely that you want to be able to enter input by pressing more than one button. Let's say A+B should force a reset of something. Quite common for example that you press multiple buttons on a watch to enter clock-setting mode. Or resets a stop watch.
But your code there will not leave processing of button A until you release it - how can it then detect that buttton B is pressed at the same time?
In the end, keyboard scan code normally always look at full set of signal lines. Doesn't matter if you have 8 buttons connected to 8 processor pins, or you have 16 buttons in a matrix of 4 rows+4 columns. For individual lines, the code would normally allow random combinations of keys pressed at the same time. For multiplexed keyboards, the code would normally reject as invalid all combinations where two row or two column signals gets activated at the same time.
With your code, you have multiple sections that looks at an input parameter to figure out which hardware signal to look at - this code will quickly grow if you have more buttons. Why do you think you got a suggestion to move this code to a separate helper function?
In the end, it would be much better if you read in all your button bits into a unsigned char and check if any key is pressed. If none is pressed, you are done. If one or more are pressed, you can then make a later check to see if you get the same pattern. If you get a stable pattern, you can report it. If you need multi-key patterns, then you need to have long enough delays in reporting that the user have time to press all buttons before you start to return the first key as a single-press. And you should normally consider always requiring all buttons released before accepting a new press of a single key or a pattern - else you need individual state info up/down for every button so that A+B reported as a pattern isn't later seen as a B press just because the user released A a bit earlier.
There is nothing magical about the 8051. When you take a copy of a hardware state, that copy is frozen. Volatile can't help. And you can't use pointers. So if you want to operate on a state that can change inside your function, then you need to go back to the actual hardware state every time.
Always program using a layered approach. So one function handling actions based on what was pressed. One function looking at time, to decide the debounce. One function looking at current hardware state to find current press state.
It's nicer to have a main loop looking like:
buttons_acceptable = TRUE; for (;;) { buttons = button_info(); switch (buttons) { case 0: // No button pressed, so rearm acceptance of button presses. buttons_acceptable = TRUE; case BUTTON_A: handle_button_a(); break; case BUTTON_B: handle_button_b(); break; case BUTTON_C: handle_button_c(); break; case BUTTON_A+BUTTON_B: handle_button_ab(); break; ... case STILL_DEBOUNCING: // seeing button presses, but haven't decided yet. break; default: handle_invalid_button_combination(); } }
And when you write the layer that does the debouncing - let it count time. But let it return instantly. Don't lock up the main loop waiting for the debounce. Or waiting for the release of a button. Your main may have lots of other fun things to do, while waiting.
Seem's you are right. But Ok, here is enchanced version with shortened variable names :) of my tunnel which becomes uglier with every new version :) Thanks for a revision.
bit debounce(unsigned char pKey) { bit localKey; if (pKey=='1') localKey=key1; if (pKey=='2') localKey=key2; if (localKey==0) { delay1ms(20); if (pKey=='1') localKey=key1; if (pKey=='2') localKey=key2; while(localKey==0) { if (pKey=='1') localKey=key1; if (pKey=='2') localKey=key2; } delay1ms(20); if (localKey!=0) return 0; } return 1; }
As a beginner in first steps with microcontroller I have never need to check multiple buttons at once but I mostly understand those bunch of useful tips on how to set a problem in such situation.
Thanks for that!