INFINITE LOOP PROBLEM

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

I am trying to make a simple password lock system. I have checked keypad and LCD codes individually they work fine. but now in this code, I want to enter 4 digits so I simply put a for loop but it is not running 4 time but infinite.

#ifndef F_CPU
#define F_CPU 1000000UL
#endif

#include <avr/io.h>
#include <util/delay.h>
#include <string.h>
#include "lcd.h"

char password[]="8051";
volatile char *enter;
int check;
///////////////// keypad////////////////
#define RowColDirection DDRD //Data Direction Configuration for keypad
#define ROW PORTD            //Lower four bits of PORTC are used as ROWs
#define COL PIND           //Higher four bits of PORTC are used as COLs

void KEYPAD_Init()
{
	RowColDirection=0xf0;   // Configure Row lines as O/P and Column lines as I/P
}

void KEYPAD_WaitForKeyRelease()
{
	unsigned char key;
	do
	{
		ROW=0x0f;         // Pull the ROW lines to low and Column high low.
		key=COL & 0x0f;   // Read the Columns, to check the key press
	}while(key!=0x0f);   // Wait till the Key is released,
	// If no Key is pressed, Column lines will remain high (0x0f)
}

void KEYPAD_WaitForKeyPress()
{
	unsigned char key;
	do
	{
		do
		{
			ROW=0x0f;	   // Pull the ROW lines to low and Column lines high.
			key=COL & 0x0F;   // Read the Columns, to check the key press
		}while(key==0x0f); // Wait till the Key is pressed,
		// if a Key is pressed the corresponding Column line go low

		_delay_ms(1);		  // Wait for some time(debounce Time);

		ROW=0x0f;		  // After debounce time, perform the above operation
		key=COL & 0x0F;	  // to ensure the Key press.

	}while(key==0x0f);

}

unsigned char KEYPAD_ScanKey()
{

	unsigned char ScanKey = 0xe0,i, key;

	for(i=0;i<0x04;i++)           // Scan All the 4-Rows for key press
	{
		ROW=ScanKey + 0x0F;         // Select 1-Row at a time for Scanning the Key
		key=COL & 0x0F;             // Read the Column, for key press

		if(key!= 0x0F)             // If the KEY press is detected for the selected
		break;                   // ROW then stop Scanning,

		ScanKey=(ScanKey<<1)+ 0x10; // Rotate the ScanKey to SCAN the remaining Rows
	}

	key = key + (ScanKey & 0xf0);  // Return the row and COL status to decode the key

	return(key);
}

unsigned char KEYPAD_GetKey()
{
	unsigned char key;

	KEYPAD_WaitForKeyRelease();    // Wait for the previous key release
	_delay_ms(1);

	KEYPAD_WaitForKeyPress();      // Wait for the new key press
	key = KEYPAD_ScanKey();        // Scan for the key pressed.

	switch(key)                       // Decode the key
	{
		case 0xe7: key='1'; break;
		case 0xeb: key='2'; break;
		case 0xed: key='3'; break;
		case 0xee: key='A'; break;
		case 0xd7: key='4'; break;
		case 0xdb: key='5'; break;
		case 0xdd: key='6'; break;
		case 0xde: key='B'; break;
		case 0xb7: key='7'; break;
		case 0xbb: key='8'; break;
		case 0xbd: key='9'; break;
		case 0xbe: key='C'; break;
		case 0x77: key='*'; break;
		case 0x7b: key='0'; break;
		case 0x7d: key='#'; break;
		case 0x7e: key='D'; break;
		default: key='z';
	}
	return(key);                      // Return the key
}

//////////////////// keypad ///////////////////////

int match_pass()
{
	int x=0;
	for(int i=0 ; i<4 ; i++)
	{
		if ( (*(password+i)=*(enter+i)) )
		x=1;
		else
		x=0;
	}
	return x;
}
void pass_enter()
{
	unsigned int i=0;

	for (i=0 ; i<4 ; i++)
	{
		*(enter+i) = KEYPAD_GetKey();
		LCDData(*(enter+i));
		_delay_ms(20);
	}
	LCDWriteString("hi");
}}

int main(void)
{

	DDRC =(1<<5);
	KEYPAD_Init();
	LCDInit(LS_BLINK|LS_ULINE);
    while (1)
    {
		LCDWriteString("password");
		_delay_ms(500);
		LCDClear();
		pass_enter();
		LCDClear();
		check=match_pass();
		if (check==1)
		PORTC |= (1<<5);
		else
		PORTC &= ~(1<<5);
    }
}

 it enters only 2 digits and then if I press any key on keypad I got 'z' which default switch value in KEYPAD_GetKey().

unsigned char KEYPAD_GetKey()
{
	unsigned char key;

	KEYPAD_WaitForKeyRelease();    // Wait for the previous key release
	_delay_ms(1);

	KEYPAD_WaitForKeyPress();      // Wait for the new key press
	key = KEYPAD_ScanKey();        // Scan for the key pressed.

	switch(key)                       // Decode the key
	{
		case 0xe7: key='1'; break;
		case 0xeb: key='2'; break;
		case 0xed: key='3'; break;
		case 0xee: key='A'; break;
		case 0xd7: key='4'; break;
		case 0xdb: key='5'; break;
		case 0xdd: key='6'; break;
		case 0xde: key='B'; break;
		case 0xb7: key='7'; break;
		case 0xbb: key='8'; break;
		case 0xbd: key='9'; break;
		case 0xbe: key='C'; break;
		case 0x77: key='*'; break;
		case 0x7b: key='0'; break;
		case 0x7d: key='#'; break;
		case 0x7e: key='D'; break;
		default: key='z';
	}
	return(key);                      // Return the key
}

here is proteus pic what I am getting. I googled it cannot find anything.

This topic has a solution.
Last Edited: Tue. Jun 12, 2018 - 02:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As it's in Proteus, have you stepped through the code to see what's happening as it runs ... ?

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

How big is the buffer holding the entered keypad data?

 

David

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

DAFlippers wrote:
How big is the buffer holding the entered keypad data?

Zero (according to the code) ;-)

Stefan Ernst

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

if I press any key on keypad I got 'z'

Then I'd use your LCD as a debug terminal and print the "key" value returned from the call to KEYPAD_ScanKey() within KEYPAD_GetKey() and find out what the value really is if it is none of the other values in the switch().

 

I was trying to "dry walk" the code in:

unsigned char KEYPAD_ScanKey()
{

	unsigned char ScanKey = 0xe0,i, key;

	for(i=0;i<0x04;i++)           // Scan All the 4-Rows for key press
	{
		ROW=ScanKey + 0x0F;         // Select 1-Row at a time for Scanning the Key
		key=COL & 0x0F;             // Read the Column, for key press

		if(key!= 0x0F)             // If the KEY press is detected for the selected
		break;                   // ROW then stop Scanning,

		ScanKey=(ScanKey<<1)+ 0x10; // Rotate the ScanKey to SCAN the remaining Rows
	}

	key = key + (ScanKey & 0xf0);  // Return the row and COL status to decode the key

	return(key);
}

to work out what the return value is when NO key is pressed. IOW it's gone all 4 times around the for() loop with no break. Clearly on the last iteration if it did not take the break then key must be 0x0F. The "ScanKey" starts at 0xE0 so 1110 in the top nibble. After one iteration it is changed to 1101 in the top nibble, after the second it is 1011, after the third it is 0111 and after the fourth it is 1111. So presumably this means the whole function returns 0xFF (0xF0 from ScanKey and 0x0F from key) when no key is pressed. So presumably the "default:" case in the switch (which sets 'z') is taken because the return is 0xFF which is not otherwise handled by the switch.

 

I suppose one question is "why would you choose to return 'z' to mean "no key"?" and if you are doing that shouldn't the code that uses KEYPAD_GetKey() be watching out for 'z' as a marker of "no key pressed" ?

 

In use you are actually doing:

		*(enter+i) = KEYPAD_GetKey();

so you are storing 'z' rather than taking some kind of "keep waiting until a key IS pressed" action. By the way:

		*(enter+i) =

is very curious C syntax. Most C programmers would have written that as:

		enter[i] =

which will break down to the same syntax in the end but is more obvious for the reader!

 

Oh and you have made the classic C error:

volatile char *enter;

So, yes, that creates a pointer to char but where are you actually assigning any storage to hold the characters that are entered? Because "enter" is a global and has no initial value C will set it to 0. So your enter[] array is actually based at RAM address 0x0000 which is where the AVR just happens to map registers R0..R31 to appear. So as you write to enter[0], enter[1], enter[2] and enter[3] you are actually writing to AVR registers R0, R1, R2 and R3. This is VERY bad indeed. The C compiler for example always relies on R1=0x00 so if that changes because of your over-write some very odd behaviour may occur.

 

It seems to me you are trying to be too clever here. If "enter" is really going to be an array of entered characters and the length is known (and it is, you read 4 characters) then why on earth didn't you simply create an array:

volatile char enter[4];

At which point I would then ask "why volatile?". I can see no reason why that would need to be volatile unless this is simply for debugging as it prevents optimisation?

 

Once you have decided that enter is going to be an array then sure you can continue with the *(enter + i) syntax if you like but as I say, most programmers would treat an array as an array and use the more obvious enter[i] syntax.

 

EDIT: oh and two more things - you probably have things global when their data locality can be more isolated and I notice that in match_pass() you have basically reinvented strncmp() - might be an idea to simply use strncmp() !!

Last Edited: Tue. Jun 12, 2018 - 12:37 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Exactly why I asked the question...

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
char password[]="8051";
volatile char *enter; // I bet that, when enter overflows (see  posts  [#3-#6] above)
int check;            // check will be corrupted
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dbrion0606 wrote:
int check; // check will be corrupted
Err no - see what I wrote. "enter" is a pointer that he writes through and because it's in .BSS it will default to 0x0000 so the corrupting writes will be to memory locations 0x0000 onwards - that is registers R0 onwards. It's true if he'd done something like:

char password[]="8051";
volatile char enter[2]; // I bet that, when enter overflows (see  posts  [#3-#6] above)
int check;            // check will be corrupted

then written to 4 elements of a [2] dimensioned array it *might* have hit check but note also that (for alignment on larger architectures) the GNU linker has a habit of grouping items of similar types so (without a stuct being used to create a group) it's probably unlikely that "check" would be positioned after "enter" in RAM anyway. So the corruption might actually hit something that is not close. In fact the linker has another worrying habit (since some recent release) of ordering things "backwards" in RAM so in a strange quirk of fate if it grouped char arrays and ordered them "backwards" the thing that would be hit after enter[] writes off the end could well be password[] !!

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

Thanx Clawson. As I was accustomed, when arrays got corrupted, to look for overflows in the arrays "before", I will  now look on both sides (from your last sentence ) ... and try to  use interpreted languages (RAM allocation is reliably done) when possible.

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

dbrion0606 wrote:
and try to  use interpreted languages (RAM allocation is reliably done) when possible.
Also, compiled computer languages.

Memory safe computer languages

https://www.avrfreaks.net/forum/memory-safe-computer-languages

 

"Dare to be naïve." - Buckminster Fuller