problem with mills()

#define pb1 2
#define pb2 3
#define pb3 4
#define pb4 5

#define op1 8
#define op2 9
#define op3 10
#define op4 11

#define pot A0

unsigned long currentMillis;
unsigned long previousMillis;
unsigned long duration;
int potVal;

unsigned long multiplier = 10; // 10mS to 10.3 seconds
byte op1State = 0;
byte op2State = 0;
byte op3State = 0;
byte op4State = 0;
void setup() {
  // put your setup code here, to run once:

  pinMode(pb1, INPUT_PULLUP);
  pinMode(pb2, INPUT_PULLUP);
  pinMode(pb3, INPUT_PULLUP);
  pinMode(pb4, INPUT_PULLUP);

  pinMode(op1, OUTPUT);
  pinMode(op2, OUTPUT);
  pinMode(op3, OUTPUT);
  pinMode(op4, OUTPUT);

  pinMode(pot, INPUT);



void loop() {
  // put your main code here, to run repeatedly:
  int value1 = digitalRead(pb1);
    int value2 = digitalRead(pb2);

    int value3 = digitalRead(pb3);

    int value4 = digitalRead(pb4);

    int value5 = analogRead(pot);

 duration = analogRead(pot) * multiplier;
    currentMillis = millis();
    if ((currentMillis - previousMillis) >= duration ) {
      previousMillis = previousMillis + duration;
      op1State = 1 - op1State;
      digitalWrite (op1, op1State);


i'm try to use push button to control outputs to be high or low by change the time being high or low by using the potentiometer but i can't . any one can tell me what's the problem ??

#define pb1 2
. . .

Look at these two lines of code, do you see any problems?


BTW, for documentation purposes, it’s time you add comments to your code.

I am not entirely sure what you want to achieve but here is are some pointers.

previousMillis = previousMillis + duration;

The variable is called previousMillis, but logicaly that is not true.

previousMillis = currentMillis;

would make it true. It might be what you want, but then the name would be wrong.

Additionaly, the button press can be missed. Maybe that is intented. Only you know.

And if(pb1==HIGH) // pb1 is defined a 2. Do you know what HIGH is? So this is ether always false or always true.

I would try to give your variables good names and then follow the logic. Right now its a bit hard to read.

Additionally go to File -> Preferences ->Compiler Warnings and set it to ALL. This should tell you that you have set value1 but never used it. So you read the button but it does not make a difference.

i’m try to use push button to control outputs to be high or low

How is the push button supposed to function? Can you explain more about what you want the code to do?

change the time being high or low by using the potentiometer but i can’t

duration = analogRead(pot) * multiplier;

Can you see the value of duration changing with the potentiometer?

if ((currentMillis - previousMillis) >= duration ) {
      previousMillis = previousMillis + duration;
      op1State = 1 - op1State;
      digitalWrite (op1, op1State);
#define pb1 2

It looks like you are comparing a pin number to a state and pb1 will always == HIGH
but the code should be toggleing the digitalWrite() state of op1 after the duration has elapsed.

the pb1/etc defines seem to represent pin numbers. But though you read values from them, in at least one place, you're just comparing the pin numbers to HIGH (which happens to be #defined as 1), instead of the value you read from the pin.

This style can be useful if it is very important to have precise timing over many successive iterations

previousMillis = previousMillis + duration;

however it can bite you in the bum if (at the start of the program run) the value of previousMillis is a lot smaller than the value of millis() and the value of duration is small. It can take several iterations before previousMillis exceeds millis()

If an occasional error of 1 millisec does not matter then this style avoids the problem

previousMillis = currentMillis;


previousMillis = millis();