logic error help?

hi,
i was programming a program for arduino. the program works correctly but there is a logic problem in it

void semaforo(){

  if((time1+8000)<millis())//Se sono trascorsi 8 secondi
  {                  
    colori(0, 255, 0);  // verde
    time2 = millis(); // azzera il conteggio del tempo
  }  // chiusura if rosso   
  
  if((time2+4000)<millis())//Se sono trascorsi 500 millisecondi
  {                   //
    colori(255, 0, 0);  // rosso
    time3 = millis(); // azzera il conteggio del tempo
  }//chiusura if giallo

  if((time3+10000)<millis()) //Se sono trascorsi 500 millisecondi
  {                   //
    colori(255, 255, 0);  // giallo
    time1 = millis(); // azzera il conteggio del tempo
  }// chiusura if verde
}// chiusura void semaforo

in according to this code every if should be elaborated, but when i start the program it works like the program looks the first if and if its true, every if after the first arent elaborated…

Im using a multicolor led. there shouldn’t be any delay() cause it would block the remaining program(not posted).

the program should manage a traffic light. the first if make it green(10 seconds), the second yellow (its written 8seconds but lasts in 2 seconds)and the last one red(its written 2seconds but it lasts in 8seconds).

Thanks for your help

Hi taran95

Could you post your complete program? We need to see (for example) how you have declared the global variables you use in the function you posted.

Thanks

Ray

  if((time1+8000)<millis())//Se sono trascorsi 8 secondi
  {                  
    colori(0, 255, 0);  // verde
    time2 = millis(); // azzera il conteggio del tempo
  }  // chiusura if rosso

Each time round loop(), this if statement executes IF millis() > time1 + 8000. But you don’t change time1 until you get to the third if statement. So this first if will still be true even when you have turned the light verde, and therefore it will immediately set it back to rosso next time round loop().

//Librerie impiegate per il progetto
#include <SPI.h>
#include <Ethernet.h>

//Creao un array di byte per specificare il mac address
byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
//creo un array di byte per specificare l'indirizzo ip
byte ip[] = {192, 168, 0, 153};
int valore = 0;


//creo una variabile char per memorizzare i byte letti dal client
char Data_RX;
EthernetClient client;
IPAddress server(192,168,0,151);


int sensor = 2;    // porta analogica del sensore
int ledPin1 = 7;     //porta digitale per i led
int ledPin2 = 6;
int ledPin3 = 5;
int redPin = 4;
int greenPin = 9;
int yellowPin = 8;
int lightPin =0;    //porta analogica per la fotoresistenza
int valori[10];    // array composto dai valori rilevati dal sensore di distanza
int val = 0;       // variabile finale del sensore

unsigned long time1 = millis();
unsigned long time2 = millis();
unsigned long time3 = millis();
unsigned long timeS = millis();



void setup() {
  //inizializza lo shield con il mac e l'ip
  Ethernet.begin(mac, ip);
  pinMode(ledPin1, OUTPUT);  // dichiariamo ledPin come un OUTPUT
  pinMode(ledPin2, OUTPUT);  
  pinMode(ledPin3, OUTPUT);  
  Serial.begin(9600);
  pinMode(redPin, OUTPUT);
  pinMode(greenPin, OUTPUT);
  pinMode(yellowPin, OUTPUT);  
} // chiusura setup
void loop() {
  int livelloNotte = 600; // indica il valore dell'oscurità presente, se superiore alla luminosità allora vuol dire che è notte
  int livelloGiorno = analogRead(lightPin);
  val = 0;
  Serial.println("sono nel loop");
  for(int i=0; i<10; i++)
  {
    valori[i] = analogRead(sensor);
    val = val + (valori[i] / 10 );
  }
  
  if((timeS+1000)<millis()){
    
    Serial.print("valore ");         // stampa i valori rilevati dal sensore e dalla fotoresistenza
    Serial.println(val);
    Serial.print("luce ");
    Serial.println(livelloGiorno);
    timeS = (millis());
    
  }// chiusura stampe
  
  if(livelloGiorno>livelloNotte){  // se è giorno
    
    semaforo();
    digitalWrite(ledPin3, LOW);
    digitalWrite(ledPin1, LOW);
    digitalWrite(ledPin2, LOW);
    
  }// chiusura luce
  
  if(livelloGiorno<livelloNotte){
    
    if(val > 350 && val < 500){
      semaforo();
      digitalWrite(ledPin3, LOW);
      digitalWrite(ledPin1, HIGH);
      digitalWrite(ledPin2, LOW);
    }// chiusura if distanza
  
    if(val > 250 && val < 350){
      semaforo();
      digitalWrite(ledPin3, LOW);
      digitalWrite(ledPin1, LOW);
      digitalWrite(ledPin2, HIGH);
    }// chiusura if distanza
    
    if(val > 150 && val < 250){
      digitalWrite(redPin, LOW);
      digitalWrite(greenPin, LOW);
      digitalWrite(yellowPin, LOW);
      digitalWrite(ledPin3, HIGH);
      digitalWrite(ledPin1, LOW);
      digitalWrite(ledPin2, LOW);
    }// chiusura if distanza
    
    if(val>500 || val<150){
      digitalWrite(redPin, LOW);
      digitalWrite(greenPin, LOW);
      digitalWrite(yellowPin, LOW);
      digitalWrite(ledPin3, LOW);
      digitalWrite(ledPin1, LOW);
      digitalWrite(ledPin2, LOW);
    }// chiusura if distanza
  }// chiusura if luce
   controllo();
   sendData(valore,6797);
   delay(500);

   
} // chiusura Loop

void colori(int red, int green, int yellow){  // funzione che imposta i colori
  analogWrite(redPin, red);
  analogWrite(greenPin, green);
  analogWrite(yellowPin, yellow);  
}

void semaforo(){

  if((time1+10000)<millis())//Se sono trascorsi 8 secondi
  {                  
    valore = 1;
    colori(0, 255, 0);  // verde
    time2 = millis(); // azzera il conteggio del tempo
  }  // chiusura if rosso   
  
  if((time2+2000)<millis())//Se sono trascorsi 500 millisecondi
  {              
    valore = 3;
    colori(255, 0, 0);  // rosso
    time3 = millis(); // azzera il conteggio del tempo
  }//chiusura if giallo

  if((time3+8000)<millis()) //Se sono trascorsi 500 millisecondi
  {           
    valore = 2;
    colori(255, 255, 0);  // giallo
    time1 = millis(); // azzera il conteggio del tempo
  }// chiusura if verde
  
}// chiusura void semaforo

void controllo(){
      if (client.available()) {
      char c = client.read();
      Serial.print(c);
    }
    
    if (!client.connected()) {
      Serial.println();
     // Serial.println("disconnecting.");
      client.stop();
    }
  
}

void sendData(int vall, int porta) {

  if (client.connect(server, porta)) {
   // Serial.println("connecting...");
 
    client.println(valore);
   
  }  else{
    // if you couldn't make a connection:
   // Serial.println("connection failed");
    //Serial.println();
    //Serial.println("disconnecting.");
    client.stop();
  }
  

}

this is the complete code.

Hackscribble:

  if((time1+8000)<millis())//Se sono trascorsi 8 secondi

{                  
   colori(0, 255, 0);  // verde
   time2 = millis(); // azzera il conteggio del tempo
 }  // chiusura if rosso




Each time round loop(), this if statement executes IF millis() > time1 + 8000. But you don't change time1 until you get to the third if statement. So this first if will still be true even when you have turned the light verde, and therefore it will immediately set it back to rosso next time round loop().
  if((time1+8000)<millis())//Se sono trascorsi 8 secondi
  {                  
    colori(0, 255, 0);  // verde
    time2 = millis(); // azzera il conteggio del tempo
  }  // chiusura if rosso   
  
  if((time2+4000)<millis())//Se sono trascorsi 500 millisecondi
  {                   //
    colori(255, 0, 0);  // rosso
    time3 = millis(); // azzera il conteggio del tempo
  }//chiusura if giallo

but when the first if is true the 2nd is true too, so my traffic light should change every time, but it works like a normal traffic light

but when the first if is true the 2nd is true too

Not sure it is …

  if((time1+8000)<millis())//Se sono trascorsi 8 secondi
  {                  
    colori(0, 255, 0);  // verde
    time2 = millis(); // azzera il conteggio del tempo
  }  // chiusura if rosso   
  
  if((time2+4000)<millis())//Se sono trascorsi 500 millisecondi
  {                   //
    colori(255, 0, 0);  // rosso
    time3 = millis(); // azzera il conteggio del tempo
  }//chiusura if giallo

If ((time1+8000) < millis()) is true, the program goes into first if statement block. This sets the light to verde and sets time2 to the current value of millis().

A few microseconds later, the program moves on to evaluate ((time2+4000) < millis()). This can’t be true yet, because millis() has not had time to increase by 4000. So the light is left at verde.

However, next time semaforo is called (looking at the rest of your code, this might be 500 - 1000ms later?), the first if statement is evaluated again. Nothing has updated time1 since the last call to semaforo, so ((time1+8000) < millis()) is still true. The light is set to verde again. BUT time2 is also reset to the current value of millis() (which is more recent than the previous value it was set to).

So, unless your main program is taking much longer to process loop(), the expression ((time2+4000) < millis()) will never be true.

Which sounds like the problem you described in your first post.

it works like the program looks the first if and if its true, every if after the first arent elaborated…

Regards

Ray

thanks very much, thanks to you now i understand the problem :slight_smile: