I want to know what's wrong with my coding

Using Arduino Programming Questions
1

Hi! I am a beginner Arduino user. Recently I write a code by using Millis function. But it is not working. can you help me find out my fault in coding. I am sharing my code below:

unsigned long currentTime = 0;
unsigned long previousTime1 = 0;
unsigned long previousTime2 = 0;

void setup()
{
pinMode(12, INPUT); //
pinMode(2, OUTPUT);
pinMode(4, OUTPUT);
}

void loop() {
unsigned long currentTime = millis();
if (digitalRead(12) == HIGH && currentTime-previousTime1>=2000)
{
digitalWrite(2, HIGH);
previousTime1 = currentTime;
if (digitalRead(12) == HIGH && currentTime-previousTime2>=6000)
{
digitalWrite(2, LOW);
previousTime2 = currentTime;
}

else {
digitalWrite(2, LOW);
}

}
}

2

That's not helpful.

Maybe if you used the IDE auto format tool, it would help you visualise your code better

Please remember to use code tags when posting code.

3

Of course it is working! If it does not work as you expect then please tell us what it should do.

You also may have to read more about state machines, if the code should do what I guess.

4

there are some problems with your logic

in the code below, the led pin is set HIGH but then immediately set LOW by both conditions of the if statement

    digitalWrite(2, HIGH);
    previousTime1 = currentTime;
    if (digitalRead(12) == HIGH && currentTime-previousTime2>=6000)
    {
        digitalWrite(2, LOW);
        previousTime2 = currentTime;
    }

    else {
        digitalWrite(2, LOW);
    }

other issues are the button switch is typically connected between the pin and ground and the pin is configure as INPUT_PULLUP. the makes it HIGH until it is depressed. the condition should check for it being LOW.

consider

#define MyHW
#ifdef MyHW
const byte pinBut  = A1;
const byte pinLed1 = 12;
#else
const byte pinBut  = 12;
const byte pinLed1 = 2;
#endif

unsigned long previousTime1 = 0;

enum { Off = HIGH, On = LOW };

void setup ()
{
    pinMode (pinBut, INPUT_PULLUP); //
    pinMode (pinLed1, OUTPUT);

    digitalWrite (pinLed1, Off);
}

void loop () {
    unsigned long currentTime = millis ();

    if (digitalRead (pinBut) == On)  {
        if (currentTime-previousTime1 >=2000) {
            previousTime1 = currentTime;
            digitalWrite (pinLed1, ! digitalRead (pinLed1));
        }
    }
}
5

well, i want like below:

unsigned long currentTime = 0;
unsigned long previousTime1 = 0;
unsigned long previousTime2 = 0;

void setup()
{
pinMode(12, INPUT); // pressure switch low input
pinMode(2, OUTPUT); // compressor-01 start command
}

void loop() {
unsigned long currentTime = millis();
if (digitalRead(12) == HIGH && currentTime-previousTime1>=2000) // when pressure switch is low which detected by arduino as high input & 2 sec delay
{
digitalWrite(2, HIGH); // compressor-01 start command
previousTime1 = currentTime;
if (digitalRead(12) == HIGH && currentTime-previousTime2>=6000) // compressor-01 remain running for from 2 sec to 6 sec
{
digitalWrite(2, LOW); // after 6 sec compressor-01 stop command
previousTime2 = currentTime;
}

else {
digitalWrite(2, LOW); // if pressure switch is high which detected by arduino as low input for compressor-01 stop command.
}

}
}

6

learn to use "</>" to post code

that's not much different from what you originally posted. and as already explained, the logic is flawed.

it would be more helpful if you explained in words what you want the code to do. seems like you want it to flash an LED on/off every 6 seconds when a button is pressed

7

And I want you to Auto Format the code in the IDE and post it here in code tags so that it looks like this

unsigned long currentTime = 0;
unsigned long previousTime1 = 0;
unsigned long previousTime2 = 0;

void setup()
{
  pinMode(12, INPUT); // pressure switch low input
  pinMode(2, OUTPUT); // compressor-01 start command
}

void loop()
{
  unsigned long currentTime = millis();
  if (digitalRead(12) == HIGH && currentTime - previousTime1 >= 2000) // when pressure switch is low which detected by arduino as high input & 2 sec delay
  {
    digitalWrite(2, HIGH); // compressor-01 start command
    previousTime1 = currentTime;
    if (digitalRead(12) == HIGH && currentTime - previousTime2 >= 6000) // compressor-01 remain running for from 2 sec to 6 sec
    {
      digitalWrite(2, LOW); // after 6 sec compressor-01 stop command
      previousTime2 = currentTime;
    }
    else
    {
      digitalWrite(2, LOW); // if pressure switch is high which detected by arduino as low input for compressor-01 stop command.
    }
  }
}
8

If that is what you want, do you have further questions?

Please read about formatting and presenting code in the sticky topics on top of the forum. Your code is unreadable right now, little chance for help.

9

thanks all for your help. I am new here. let me explain what i want to execute through code:

  1. there will be one digital input signal
  2. two digital output signal

condition_1: **when digital input is high ** : digital output_1 will be high from 0 to 10 seconds & that time digital output_2 will be low. Then from 11 to 20 seconds digital output_2 will be high and that time digital output_1 will be low. This loop will continue as long as digital input is high.

condition_2: when digital input is low : both digital output_1 and digital output_2 will be low.

** I am trying to learn "Auto Format the code in the IDE". I will post the code in that format after a while.

10

  1. Have you tested JUST the digital input to make sure it is stable? What is the source of the digital input?

  2. Values you are testing against millis() against and number of outputs do not correspond with the following description:

condition_1: **when digital input is high ** : digital output_1 will be high from 0 to 10 seconds & that time digital output_2 will be low. Then from 11 to 20 seconds digital output_2 will be high and that time digital output_1 will be low. This loop will continue as long as digital input is high.

condition_2: when digital input is low : both digital output_1 and digital output_2 will be low.

  1. If the input goes LOW and then HIGH again should condition_1 timings start all over again or resume?
11

This might work for you but I haven't tested it yet. You will have to adjust the output2Pin assignment:

const int output1Pin = 2;
const int output2Pin = 3;
const int inputPin = 12;
const unsigned long outputTime = 10000;

unsigned long previousTime = 0;

int outputState = 0;

void setup()
{
  pinMode(inputPin, INPUT); // pressure switch low input
  pinMode(output1Pin, OUTPUT); // compressor-01 start command
  pinMode(output2Pin, OUTPUT);
}

void loop()
{
  static int lastInputState = digitalRead(inputPin);
  int currInputState = digitalRead(inputPin);
  unsigned long currentTime = millis();
  
  // If input went from LOW to HIGH reset output sequence
  if (lastInputState ==LOW && currInputState == HIGH)
  {
    previousTime = currentTime;
    outputState = 0;
    digitalWrite(output1Pin, HIGH);
    digitalWrite(output2Pin, LOW);
  }
  
  // If input is HIGH execue output sequence
  if (currInputState == HIGH)
  {
    if (outputState == 0)
    {
      if (currentTime - previousTime > outputTime)
      {
        previousTime = currentTime;
        digitalWrite(output1Pin, LOW);
        digitalWrite(output2Pin, HIGH);
        outputState = 1;
      }
    }
    else
    {
      if (currentTime - previousTime > outputTime)
      {
        previousTime = currentTime;
        digitalWrite(output1Pin, HIGH);
        digitalWrite(output2Pin, LOW);
        outputState = 0;
      }
    }
  }
  else
  {
    digitalWrite(output1Pin, LOW);
    digitalWrite(output2Pin, LOW);
  }
  
  lastInputState = currInputState;
}
12

Then a single start time is sufficient. During interval 1 (up to 10s) output_1 will be high, else during interval 2 (up to 20s) output_2 will be high, and afterwards the loop continues with the new start time.

For the change of the input see the StateChangeDetection example. If the button goes on the loop is started and going off ends the loop.

13
  1. digital input source is a switch.
  2. if the input goes low and then high again: it will start from condition_1 again.
14

thanks for your help. code is working. Only one flow, (that because may be i couldn't make it clear to you. ) .

when input goes from LOW to HIGH, output_1 take a 10 second delay (outputTime = 10000) to give HIGH . Instead i wanted output_1 will be high immediately after input goes from LOW to HIGH.

16

In my code output 1 goes to HIGH immediately after input goes to HIGH.

17

well, now it is okay. but another problem is like that: after switch on Arduino power and when input goes high then immediately output_1 goes to High. But after that if i press the Arduino reset button then it wait one cycle Output_1 as low although input is HIGH. other than that code is working perfectly. thanks for your help.

18

OP, now, as a lesson, see if you can comment the code, so we know that you understand why it’s written that way…!

19

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.