Hi
I made flow chart and program for following Exercise. Please Look at flow chart and program. Does my flow chart match with program ?
Exercise #1
Flash an LED as follows, turn on LED for n seconds, turn off LED for t seconds, repeat forever

 

It looks like the same mistakes as before in your delay function.

void Delay (unsigned int i)
{
    for(i = 0; i < 40000; i++);
    {

    }
}

(1) You overwrite the input parameter, so the delay will always be the same.
(2) You have a stray semicolon after the for loop. The pair of brackets ("{" and "}") that follow are therefore not "attached" to the loop (they essentially amount to an "anonymous scope" within the function). Empty statements should be avoided anyway...they just look like accidentally-deleted code; use the continue keyword instead for clarity.

So really the take-away here is to pay closer attention to what has already been pointed out before, otherwise you're just going to end up going around and around in circles without learning much.

 

Looks good, with some corrections to be made. See below

Why don't you post the diagram and the code directly to the thread so that readers do not have to click on the file?

#include <REG51.h>
#define LED_ON 1  /* LED ON */
#define LED_OFF 0 /* LED OFF */
/* LED connected to port P1 of pin 7 */
sbit LED = P1^7;

/* This is delay function */
void Delay (unsigned int i)
{
  for  (i = 0; i <40000; i++);
  {
  }
}

void main(void)
{
  LED = LED_OFF; /* LED OFF */
  while (1)
  {
    LED = LED_ON; /* Turn ON LED */
   Delay(37000);  /* Wait 40 ms */
   LED = LED_OFF; /* turn off LED */
   Delay(18500);  /* wait 20ms */
  }
}

#1
LED = LED_ON; /* Turn ON LED */
LED = LED_OFF; /* turn off LED */
These comments are redundant. (i.e. useless - they add no new information)

#2
You have an error in the for statement. Semicolon ends the statement.

#3
Use unsigned long in your Delay() function. This will you a larger range of delays.

#4
xox points out error.

 

Your usage of Delay() implies strongly that it takes an argument that controls how long the delay is. But your code completely ignores the value that is passed in.

 

Also, just a recommendation, but a much more intuitive delay function might specify a floating-point parameter defined in terms of seconds (which would then get converted into loop increments within the function). Not only would that be way more readable, but it would obviously save on having to manually calculate accurate durations by hand.

void Delay(double seconds);
// ...
Delay(1.0);  // one second
Delay(1.0/10.0); // one-tenth of a second
Delay(0.003); // three milliseconds

 

I seen, errors are related to delay function. I have corrected error in delay function. Is there anything wrong here?

/* This is delay function */

void Delay (unsigned long n)
{
      unsigned int i;

      for (i = 0; i < n;  i++);         
}

 

Yes. If you are going to input n as unsigned long then you better make i the same, unsigned long.

Just that you understand,

for (i = 0; i < n;  i++);

the semicolon at the end terminates the for.

Make sure you remove the semicolon the next time you want to do something like this:

for (i = 0; i < n;  i++)
  {
  }

 

To make your code more meaningful at a future date, why not use

Delay_ms(40);

to mean delay by 40ms.

 

Exercise #2
If Switch is closed, Turn ON LED. else Turn OFF LED

Flow Chart



Program code

#include<REGX51.h>

/*set bit P2^0 to Switch*/
sbit  Switch = P2^0;

/*set bit P1^7 to LED*/
sbit LED = P1^7;

#define LED_ON             1
#define LED_OFF            0

#define Switch_Closed      1
#define Switch_Open        0

void main (void)
{
        Switch  =  Switch_Open;
        LED  =  LED_OFF;
  
   while (1)
   {
      if (Switch  ==  Switch_Closed)
      {
            LED  =  LED_ON;
      }
      else
      {
           LED  =  LED_OFF;
      }
   }
}

 

Exercise #3
When Switch is closed, Turn ON LED. When Switch is open , Turn OFF LED repeat forever



Program code

#include<REGX51.h>

/*set bit P2^0 to Switch*/
sbit  Switch = P2^0;

/*set bit P1^7 to LED*/
sbit LED = P1^7;

#define LED_ON   1
#define LED_OFF  0

#define Switch_Closed  1
#define Switch_Open    0

void main (void)
{
     Switch  =  Switch_Open;
     LED  =  LED_OFF;

  while (1)
     {
        while (Switch  ==  Switch_Closed)
        {
              LED  =  LED_ON;
        }

        while (Switch  ==  Switch_Open)
        {
              LED  =  LED_OFF;
        }
     }
}

 

@Parth786 Before you go any further, go back to post #9 and find another way to do this.

 

@Parth786 Before you go any further, go back to post #9 and find another way to do this.

Have you seen Post#10. It check both conditions of switch. My program check all possible condition of switch. So I don't see another way.

 

Have you seen Post#10. It check both conditions of switch. So I don't see another way. My program check all possible condition of switch

Do you think post #10 is different from post #9?

 

Do you think post #10 is different from post #9?

I have checked both program's in simulator. Both do the same things.

 

How can you make the LED reflect the state of the switch?

 

How can you make the LED reflect the state of the switch?

Sorry But I didn't understand your question. I copied your line and search on google. are you talking about denouncing.
if this is case then

void main ()
{
    Switch  =  Switch_unpressed;
     LED = LED_OFF;

  while (1)
  {
         if(Switch   ==   Switch_presed)                /* Switch is pressed */
         {
              Debounce_time(22000);                      /* wait 20 msec */
               if(Switch  ==  Switch_pressed)             /* check switch again */
               LED = LED_ON;
         }
   }
}

Note : This is not complete Program. I have just posted the part of program for switch push button

 

At this stage you don't need to debounce the switch. Why? Think about it.

Think about this one:

LED = Switch;

 

At this stage you don't need to debounce the switch. Why? Think about it.

Think about this one:

LED = Switch;

Sorry but really I don't understand what do you mean by your statement. There are the two state of switch open or closed ( 0 or 1).
also LED has two state LED ON or LED OFF (1 or 0 ).

 

Exercise #2
If Switch is closed, Turn ON LED. else Turn OFF LED

Flow Chart

View attachment 137099

Program code

#include<REGX51.h>

/*set bit P2^0 to Switch*/
sbit  Switch = P2^0;

/*set bit P1^7 to LED*/
sbit LED = P1^7;

#define LED_ON             1
#define LED_OFF            0

#define Switch_Closed      1
#define Switch_Open        0

void main (void)
{
        Switch  =  Switch_Open;
        LED  =  LED_OFF;
 
   while (1)
   {
      if (Switch  ==  Switch_Closed)
      {
            LED  =  LED_ON;
      }
      else
      {
           LED  =  LED_OFF;
      }
   }
}

Your flowchart matches your code. Good job.

Note that you have not dealt with switch debouncing, but in this case you don't really need to. If you flip the switch and it bounces (say it changes from OPEN to CLOSED and back a few dozen times over the course of a handful of milliseconds) then your LED might switch on and off several times over that interval as well (depending on how fast your loop runs and exactly when you sample the switch). But so what -- the human eye won't be able to detect it. Now, if you had a sensor that was viewing the LED, or you were doing something else based on reading the condition of that switch, not dealing with the switch bounce might be a critical error. As with most things, it all depends on what is and what isn't important in a particular situation.

While your flowchart and code are correct and clearly convey the purpose of the program, they are very inefficient.

If the Switch is closed, then it has a value of 1 and, when that is the case, you want to set LED to a value of 1.

If the Switch is open, then it has a value of 0 and, when that is the case, you want to set LED to a value of 0.

Notice that, regardless of what the value of Switch happens to be, you want to set LED to that same value. This can be accomplished with a single statement:

LED = Switch;

That's the only statement that needs to be within your while(1) loop. But it is a bit of a hack and so a comment is probably in order to link the logic to the implementing code.

Also, it is customary to make all #define symbolic constants all uppercase (there are times when it makes sense to violated this, but not very often) so that readers know they are dealing with a constant. Similarly, variables should be lowercase to prevent them from being mistaken for constants.

It is also customary to avoid numeric constants in the code unless the reason they are there is specifically because of the constant value itself. For instance, in your while(1) the '1' isn't there because your logic uses the value 1 (as opposed to 45 or 3.14), it is there because you need something that is interpreted as a logic TRUE. So use a symbolic constant for that.

Consider the following code:

#include<REGX51.h>

#define TRUE (1)
#define FALSE (0)
#define FOREVER (TRUE)

#define LED_ON        (1)
SWITCH_CLOSED    (1)

#define LED_OFF       (!LED_OFF)
#define SWITCH_OPEN (!SWITCH_CLOSED)

/*set bit P2^0 to Switch*/
sbit  switch = P2^0;

/*set bit P1^7 to LED*/
sbit led = P1^7;

void main (void)
{
   LED  =  LED_OFF;
 
   while (FOREVER)
   {
      /* led tracks the state of the switch */
     /* may need to be inverted if polarity of switch or LED changes */
      led = switch;
   }
}

Notice that while that single statement is the most efficient code, it introduces a maintainability issue because if the switch or LED polarity gets flipped, the code will need to be tweaked to keep the same behavior. If you NEED the code to run this fast, then you live with this and document it well. But if you can live with slightly less performance, you can easily make the code self-maintaining while sacrificing only a little bit of performance.

#include<REGX51.h>

#define TRUE (1)
#define FALSE (0)
#define FOREVER (TRUE)

#define LED_ON        (1)
SWITCH_CLOSED    (1)

#define LED_OFF       (!LED_OFF)
#define SWITCH_OPEN (!SWITCH_CLOSED)

/*set bit P2^0 to Switch*/
sbit  switch = P2^0;

/*set bit P1^7 to LED*/
sbit led = P1^7;

void main (void)
{
   LED  =  LED_OFF;
 
   while (FOREVER)
   {
      if (SWITCH_CLOSED == switch)
         led = LED_ON;
      else
         led = LED_OFF;
   }
}

This can be tightened up a bit, and some compilers can squeeze a bit more performance, from the following tweak.

#include<REGX51.h>

#define TRUE (1)
#define FALSE (0)
#define FOREVER (TRUE)

#define LED_ON        (1)
SWITCH_CLOSED    (1)

#define LED_OFF       (!LED_OFF)
#define SWITCH_OPEN (!SWITCH_CLOSED)

/*set bit P2^0 to Switch*/
sbit  switch = P2^0;

/*set bit P1^7 to LED*/
sbit led = P1^7;

void main (void)
{
   LED  =  LED_OFF;
 
   while (FOREVER)
   {
      LED = (SWITCH_CLOSED == switch)? LED_ON : LED_OFF;
   }
}

If you haven't played with the ternary operator (also known as the conditional operator), then now might be a good time to look it up. But it can also wait until you are ready.

 

Exercise #3
When Switch is closed, Turn ON LED. When Switch is open , Turn OFF LED repeat forever

View attachment 137101

Program code

#include<REGX51.h>

/*set bit P2^0 to Switch*/
sbit  Switch = P2^0;

/*set bit P1^7 to LED*/
sbit LED = P1^7;

#define LED_ON   1
#define LED_OFF  0

#define Switch_Closed  1
#define Switch_Open    0

void main (void)
{
     Switch  =  Switch_Open;
     LED  =  LED_OFF;

  while (1)
     {
        while (Switch  ==  Switch_Closed)
        {
              LED  =  LED_ON;
        }

        while (Switch  ==  Switch_Open)
        {
              LED  =  LED_OFF;
        }
     }
}

This is a big step back. Your flowchart doesn't match your code and neither is what you want.