Turn LED on for 250ms and off for 750ms

Good evening. I am trying to make an LED turn on for 250ms and then off again for 750ms repeatedly. I have tried various methods, but with little success. The code below is my closest yet: however, after an initial blink, the LED settles into a dim glow, but I cannot see why. Any help would be appreciated.

Thank you.

James.

  const int Built_in_LED =  13;      // The number of the LED pin
  unsigned long Currently_It_Is = millis(); // Present time
  unsigned long Previously_It_Was = 0; // Time an event occurred
  
   
  const long Time_to_Light_LED = 250;  // Duration LED remains on
  const long Time_to_Extinguish_LED = 750; // Duration LED remains off
  
void setup() {

  pinMode(Built_in_LED, OUTPUT); // Set the digital pin as output:
  digitalWrite(Built_in_LED, LOW);  // Start LED turned off

  Serial.begin (9600); //Initiate serial printer
}

void loop() {

if (Currently_It_Is - Previously_It_Was >= Time_to_Extinguish_LED); // Wait for 750 and then turn LED on
 {
  Previously_It_Was = Currently_It_Is;
  digitalWrite(Built_in_LED, HIGH);
 }

if (Currently_It_Is - Previously_It_Was >= Time_to_Light_LED); // Wait for 250 and then turn LED off
 {
  Previously_It_Was = Currently_It_Is;
  digitalWrite(Built_in_LED, LOW);
 }
}

Where in the code are you updating the value of Currently_It_Is ?

Time to start selecting better names for you variables. :wink:

.

UKHeliBob:
Where in the code are you updating the value of Currently_It_Is ?

After I posted the query, I checked and realised that I had this in the wrong place. I have now put
unsigned long Currently_It_Is = millis(); as the first line in the loop.

However, the code still does not function as anticipated.

James.

However, the code still does not function as anticipated.

So, you changed the code, but didn't post it.

It does something that you didn't describe.

You expect it to do something that you didn't describe.

And yet you want us to help you fix the code. Doesn't seem likely, does it?

Here is the amended code. I ran out of time to post it this morning.

const int Built_in_LED =  13;      // The number of the LED pin
unsigned long Previously_It_Was = 0; // Time an event occurred

unsigned long Period_Of_Time = 0; // Duration between now and then
 
const long Time_to_Light_LED = 250;  // Duration LED remains on
const long Time_to_Extinguish_LED = 750; // Duration LED remains off

void setup() {

pinMode(Built_in_LED, OUTPUT); // Set the digital pin as output:
// digitalWrite(Built_in_LED, LOW);  // Start LED turned off

Serial.begin (9600); //Initiate serial printer
}

void loop() {
unsigned long Currently_It_Is = millis(); // Present time
Period_Of_Time = (Currently_It_Is - Previously_It_Was);

switch (Period_Of_Time) {
 case 1:
  Period_Of_Time <= Time_to_Light_LED;
//   Previously_It_Was = Currently_It_Is;
  digitalWrite(Built_in_LED, HIGH);
  Serial.print(Period_Of_Time);
  Serial.println();    
  break;

case 2:
  Period_Of_Time >= Time_to_Light_LED && Period_Of_Time <= Time_to_Extinguish_LED;
//  Previously_It_Was = Currently_It_Is;
  digitalWrite(Built_in_LED, LOW);
  Serial.print(Period_Of_Time);
  Serial.println();
  break;

 

 //Currently_It_Is - Previously_It_Was >= Time_to_Light_LED); // Wait for 250 and then turn LED off
  //Previously_It_Was = Currently_It_Is;
  //digitalWrite(Built_in_LED, LOW);
  //Serial.print(Currently_It_Is);
  //Serial.println();
  
// default: 
// digitalWrite(Built_in_LED, HIGH);
//break;
}
}

Kind regards,

James

More than one way to skin a cat

unsigned long startTime;
unsigned long currentTime;
unsigned long periods[] = {250, 750};
byte periodIndex = 0;
const byte ledPin = 13;

void setup()
{
  pinMode(ledPin, OUTPUT);
}

void loop()
{
  currentTime = millis();
  if (currentTime - startTime >= periods[periodIndex])
  {
    digitalWrite(ledPin, !digitalRead(ledPin));
    periodIndex++;
    periodIndex = periodIndex % 2;
    startTime = currentTime;
  }
}

Thank you, Bob.

I shall study this tonight.

Regards,

James

Look at it and maybe learn something. It uses an array and the % operator which may be new to you. You may need to swap the order of the periods in the array to get the effect that you want.

switch (Period_Of_Time) {
  case 1:
   Period_Of_Time <= Time_to_Light_LED;

Care to explain what you think that third line is doing?

PaulS:

switch (Period_Of_Time) {

case 1:
  Period_Of_Time <= Time_to_Light_LED;



Care to explain what you think that third line is doing?

The aim of the line is to calculate how many millis have elapsed since the start of the process and store it in the variable "Period_Of_Time".

James.

Have a look at the demo Several Things at a Time - three flashing LEDs

...R

Success.

I now have the code working. Thank you for the help and links.

I have reverted to my original use of "if" statements as I wanted the satisfaction of getting them to work.

Bob: I like the elegance of your suggestion and shall look at using the idea in another part of the project that I am doing where "if" statements will be too cumbersome.

Here is the revised code, which I shall annotate later.

const int Built_in_LED =  13;      // The number of the LED pin
unsigned long Previous_Millis = 0; // Time an event occurred
unsigned long Elapsed_Time = 0; // Time an event occurred 
const long Period_On = 150;
const long Period_Off = 850;
int State_of_LED = LOW;             // ledState used to set the LED

void setup() {
pinMode(Built_in_LED, OUTPUT); // Set the digital pin as output:
}

void loop() {
unsigned long Present_Time = millis(); // Present time

Elapsed_Time = Present_Time - Previous_Millis;

 if (Elapsed_Time <= Period_On){State_of_LED = HIGH;} 
 if (Elapsed_Time >= Period_On){State_of_LED = LOW;} 
 if (Elapsed_Time >= Period_On + Period_Off){Previous_Millis = Present_Time;} 
   // set the LED with the ledState of the variable:
   digitalWrite(Built_in_LED, State_of_LED);
  }

catceefer:
Here is the revised code, which I shall annotate later.

For the uninitiated, LATER is a technical term. It means "the grandchildren will do it after I'm gone"

Do it while you are writing the program. If you use meaningful names for variables and functions a program will be largely self-documenting

...R

Robin2:
For the uninitiated, LATER is a technical term. It means "the grandchildren will do it after I'm gone"

Do it while you are writing the program. If you use meaningful names for variables and functions a program will be largely self-documenting

...R

It is done already: I did it as I integrated it into the rest of my code for the project.

Well done for persisting and getting the program working but embrace the use of arrays now. Don't put it off. They may look scary but they really aren't

Another thing to tackle is the type of variables used by your programs. In general you should use a variable type that allows values no larger than needed to save space. It may not matter in this program but one day it might so get used to doing it.

Examples in your program are the pin number of the LED, but see below, and the LED state, both of which could be byte instead of int.

The IDE has the pin number of the built in LED defined for the selected board. You can use the name LED_BUILTIN instead of creating a variable or using your own #define. Using this name would make your code portable to other Arduinos without changing the actual pin number in the code. As it happens, most Arduinos have the built in LED on pin 13 but not all of them.

Another suggestion. You have code in this format

 if (Elapsed_Time <= Period_On){State_of_LED = HIGH;} 
 if (Elapsed_Time >= Period_On){State_of_LED = LOW;} 
 if (Elapsed_Time >= Period_On + Period_Off){Previous_Millis = Present_Time;}

and it works, but it can sometimes be difficult to read and maintain. Easier to read and maintain, at the expense of more screen space used in the editor, would be

  if (Elapsed_Time <= Period_On)
  {
    State_of_LED = HIGH;
  }
  if (Elapsed_Time >= Period_On)
  {
    State_of_LED = LOW;
  }
  if (Elapsed_Time >= Period_On + Period_Off)
  {
    Previous_Millis = Present_Time;
  }

You should also consider the use of if/else if/else where multiple tests are mutually exclusive so that multiple tests are not performed when one condition is found to be true. For instance, the elapsed time can only be greater, equal or less than the on period so you don't need to check that twice. Incidentally, there is a minor flaw in the logic of your tests as they stand. What will happen if the elapsed time equals the on period ? Which test(s) will return true and what will the LED state be set to ? It may not matter in this program but one day it might.

As I said earlier, well done for getting it working but always seek to improve your program layout, logic and memory use. Keep your working version when you improve it so that you can refer back and even revert to the original if you need to.

Bob,

Thank you for the constructive suggestions. I shall have a proper look at arrays: I have always shied away from them in the past, but I can see how they can be useful.

I did try writing my "if"s as you suggest, but kept getting into a muddle. I have been so used to writing nested "if" formulae in Excel in a single line that I find it easier to follow in that format, where it can be used.

The silly logic error is annoying, but now amended.

I shall also review my variables and change them to be more efficient.

Regards,

James.

With regard to your series of if statements:

  if (Elapsed_Time <= Period_On)
  {
    State_of_LED = HIGH;
  }
  if (Elapsed_Time >= Period_On)
  {
    State_of_LED = LOW;
  }
  if (Elapsed_Time >= Period_On + Period_Off)
  {
    Previous_Millis = Present_Time;
  }

How many of these blocks can be true for any given value of Elapsed_Time? If only one of them can be true, if/else if/else statements are preferred.

I can't see why you would want to turn the LED on if Elapsed_Time is equal to Period_on and then turn around and turn it off for exactly the same reason. So, I suspect that there is a flaw in your logic.

PaulS:
I can't see why you would want to turn the LED on if Elapsed_Time is equal to Period_on and then turn around and turn it off for exactly the same reason. So, I suspect that there is a flaw in your logic.

Paul,

Thank you for the feedback. I intend to look at using "else" to tidy the code up a bit. My logic was flawed, as you suggest, and has been altered to correct it.

Regards,

James.