problem with millis

Hi,

Can someone help with a problem I'm having with millis? Basically I'm trying to get a couple of piezo elements to trigger their respective digital pin high for a set duration (about 1 sec), without using delay() as this prevents other things from going on. I've got it working to an extent, but after about 30 seconds, it seems to crash and the output pin stays high permanently. Here's my code:

byte val1 = 0;        // variable to store the value read from the sensor pin
byte val2 = 0;        // variable to store the value read from the sensor pin
int timer1 = 0;       // time sensor was activated
int timer2 = 0;       // time sensor was activated
int THRESHOLD = 100;  // threshold value to decide when the detected sound is a knock or not

void setup() {
  pinMode(13, OUTPUT); // declare the ledPin as as OUTPUT
  pinMode(12, OUTPUT); // declare the ledPin as as OUTPUT
}

void loop() {
  val1 = analogRead(5);       // read the sensor and store it in the variable "val1"
  if (val1 >= THRESHOLD) {    // if it's over threshold
    timer1=millis();          // store time
    digitalWrite(13, HIGH);   // turn on LED
  }
  if (millis() > timer1+1000) {  //  if time since activation is longer than one second
    digitalWrite(13, LOW);       // turn off LED
  }
  val2 = analogRead(4);       // read the sensor and store it in the variable "val2"
  if (val2 >= THRESHOLD) {    // if it's over threshold
    timer2=millis();          // store time
    digitalWrite(12, HIGH);   // turn on LED
  }
  if (millis() > timer2+1000) {  //  if time since activation is longer than one second
    digitalWrite(12, LOW);       // turn off LED
  }
}

Code looks mostly good here.

Minor issue: analogRead() returns numbers in range 0 - 1023; your byte variable truncates them.

Likewise, millis() returns long values; the int variable truncates those. 2^15 milliseconds is just over 32 seconds. ;)

Thanks for the tip! I've added another integer variable called timer1test so that when storing the new millis variable it will automatically truncate it and then compare the two variables properly. This seems to work ok.

void loop() {
  val1 = analogRead(5);       // read the sensor and store it in the variable "val1"
  if (val1 >= THRESHOLD) {    // if it's over threshold
    timer1=millis();          // store time
    digitalWrite(13, HIGH);   // turn on LED
  }
  timer1test = millis();
  if (timer1test > timer1+1000) {  //  if time since activation is longer than one second
    digitalWrite(13, LOW);         // turn off LED
  }

Er, why not just change timer1 and timer2 to a long data type?

duh! yeah that's a better way :) (the right way even)