lcd_disp( QUESTION_MARK_CHARACTER );

If you are considering another message than a question mark, then you obviously should avoid a symbol name that says QUESTION_MARK_CHARACTER. Why not a:

lcd_disp(INPUT_PROMPT);

So loop termination value for 'j' isn't important, as long as limit is large? So why large and not instead infinite? If not infinite, then you should be able to figure out a better value than just "large".

if(state_var==ENTER_VAL || state_var==ERROR_VAL || state_var==MISMATCH_VAL)

Is the "var" part intended to mean "variable"? How does it help the user to know that it is a variable? Isn't "j", "temp1", "count" also variables? Don't add extra info that isn't really extra info. Symbol names should explain important things to the reader.

Next part is "state". Most things in this world have a state. So what does a reader get out of your name? You sure it isn't an "input_result"?

You have a variable "right_count". Is there a variable "left_count" too? It is important to write code that isn't ambiguous. If you use the word "correct" insted of "right", you make it clearer that you consider number of accepted characters for the password and isn't thinking about an alignment problem for the display output. If you instead use "num_accepted_chars" you would be even better off.

You have a function keypad(). But what does it do? Does it read keys from a keypad? Or does it initialize a keypad? Or detect if a keypad is connected? Would I be able to ask these questions if the function had instead been named read_keypad()?

void delay_small(unsigned char u)
void delay_large(unsigned char u)

5 minutes is a short delay if waiting for an airplane.
1 ms is a very long delay when reading a byte from RAM.

Obviously, you should never use "small" and "large" in a symbol name since "small" and "large" can mean just about anything. Is the Eiffel Tower large? Is the moon small? Is our sun large? Is a bull small? Why didn't you make a single delay function supporting delays from 1ms to 65535ms? Even if an 8051 is an 8-bit processor, you can still use 16-bit integers. Sometimes, you take a performance hit. But in your case, the goal with the function is to consume time. Or why didn't you implement one 1ms delay function and one 100ms delay function? Or a one-second delay function?
If I do delay_large(50), I need to check through your source code to figure out what 100 times "large" is. If I call delay_100ms(50), I can probably figure out that 50*100ms = 5 seconds.

You have obviously already noticed this problem, since you felt you had to "explain" the resulting delay with comments:

if (a==1) {
    delay_large(80);// Four second delay is to be expected
} else {
    delay_large(10);// 0.5 second delay is to be expected
}

By the way - what reader will now what "a==1" means? Do you know without getting the full source for the function and a bit of time to think? A is the first character in the alphabet - was that the selection criteria for the naming of this parameter?

And shouldn't you split lcd_disp1() into one function responsible for printing text and immediately return, and a different function who calls the first function and then adds a delay before the displayed text gets overwritten?

for(i=0; i<1; i++) {
    lcd_disp1(lcd_door,0);
    temp=keypad();
    if(temp != ERROR_VAL)  break;

    lcd_disp1(lcd_change,0);
    temp=keypad();
    if(temp != ERROR_VAL)  break;

    lcd_disp1(lcd_exit,0);
    temp=keypad();
    if(temp != ERROR_VAL)  break;
}

The loop just iterates a single time. So it isn't really a loop. You have added this construct just to be able to use "break" within a linear sequence of instructions. There are times when this is a great approach, simplifying the code. Are you sure that your specific code really gains from your fake loop? And what does the last break do?

lcd_disp1(lcd_door,0);
response = read_keypad();
if (response != ERROR_VAL) {
    lcd_disp1(lcd_change,0);
    response = read_keypad();
    if (response != ERROR_VAL) {
        lcd_disp1(lcd_exit,0);
        response = read_keypad();
    }
}

The nested if statements with the constantly growing indent will at least have the advantage that a reader will instantly understand that a number of individual criteria must be fulfilled one-by-one to reach the innermost code block.

A big question then is if it is expected to be hard to reach the innermost section, or if your code better describes the flow. Maybe most runs are expected to do everything and only once in a blue moon does you get an unexpected response where you should end the sequence early.

void change_key()

For most part of your code, key or keypad is used in relations to key presses on a keypad. But this function - with key in the name - is suddenly used to change a password.
If "Right Key" means a key on a keypad, then you should not have a function with this name or a prompt: "Key Changed..." when the password has been changed.


There is a term "locality of reference". It is applicable in a lot of computer-science situations. But if we look at your source code - for example a single page of source code lines. You should then try to write your code so that I get as good picture as possible about what happens in that page of code, without me needing to scroll up or down and constantly try to find explanations in comments or among the #defined symbols. When possible, a symbol name should give a good indication of what the symbol is used for - or what it does.

So whenever you add one more variable - look at the name, and try to figure out what information you get from the name of the variable. The more meaning you can get into the symbol name, the less risk that you use the symbol in the wrong way because you happened to forget your original plan for the variable.