Go Down

Topic: help with millis() (Read 710 times) previous topic - next topic

leeamsi97

I am trying to make an rfid safe. I am using a finite state machine to have 3 states: locked, unlocked, and reprogramming. In the locked state, the arduino scans for either a button press, which changes the state to reprogramming, or a card read by the rfid reader, which changes the state to unlocked if it matches the authorized code stored in eeprom. In the unlocked state, the arduino waits 3 seconds, then relocks the safe and changes its state to locked. In the reprogramming state, the ardsuino stores the next card to be scanned in eeprom (this feature changes the card authorized to open the safe). If no card is scanned within 5 seconds, the state is changed back to locked. To execute the 5 second timeout, i am using millis(). In the code, i had the arduino print "reprograming..." to the serial monitor when the button is pressed, and "locked" when the 5 second timeout is over. The problem is, when the button is pressed, the serial monitor prints "reprogramming...", then "locked" in a cycle that runs 4-5 times in about a second. If the button is held down, it continues the cycle until the button is released.

Quote

#include <EEPROM.h>
#include "EEPROMAnything.h"
int  val = 0;
char code[11];                    //the code read by the RFID reader
char validCode [11];             //code read from EEPROM is stored here
int bytesread = 0;
#define buttonPin 10
int lockState = 0;
unsigned long time;





void setup(){
  Serial.begin (2400);
  pinMode(2,OUTPUT);
  pinMode (buttonPin, INPUT); 
  digitalWrite(2, LOW);
  EEPROM_readAnything (0, validCode);
}

bool readRFID(){
  if(Serial.available() > 0) {          // if data available from reader
    if((val = Serial.read()) == 10) {   // check for header
      bytesread = 0; 
      while(bytesread<10) {              // read 10 digit code
        if( Serial.available() > 0) {
          val = Serial.read();
          if((val == 10)||(val == 13)) { // if header or stop bytes before the 10 digit reading
            break;                       // stop reading
          } 
          code[bytesread] = val;         // add the digit           
          bytesread++;                   // ready to read next digit 
        } 
      } 
      if(bytesread == 10) {           // if 10 digit read is complete
        bytesread = 0; 
        digitalWrite(2, HIGH);                  // deactivate the RFID reader for a moment so it will not flood
        delay(1500);                       // wait for a bit
        digitalWrite(2, LOW);                  // Activate the RFID reader
        return true;
      }
    }
  } 
  return false;


void loop() {
  switch (lockState)
  {
  case 0:                                         //Locked State
    if (digitalRead (buttonPin) == LOW)           // check if the button is pressed
    {        
      time = millis();
      lockState = 2;
      Serial.println ("Reprogramming...");
      break;
    }
    else if(readRFID()== true)
    {
      if (strcmp (code, validCode) == 0)
      {
        //unlock...
        lockState = 1;
        break;   
      }
    }

    break;
  case 1:                                    //Unlocked State
    digitalWrite (2, HIGH);
    Serial.println ("Unlocked");
    delay (3000);
    Serial.println ("Locked");
    digitalWrite (2, LOW);
    //lock...
    lockState = 0;
    break;
  case 2:                                    //Reprogram
    if (time + 5000 >= millis())
    {
      Serial.println ("Locked1");
      lockState = 0;
      break;
    }

    else if (readRFID())
    {
      //EEPROM_writeAnything (0, code);
      Serial.println ("Locked");
      lockState = 0;
      break;
    }
    //break;
  }
}


the nature of the problem suggests (to me) that the code is performing the actions inside the

if (time + 5000 >= millis())

statement even when the condition is not true. How can I fox this problem? Is it my code, or is something wrong with my chip? please help...if you have any questions, please ask.


Canyonero

I would write it as:



if (time - buttonPressTime >= 5000) {
    ....


time should be updated every time through the loop, and buttunPressTime should be updated when the button is pressed.

CrossRoads

This
if (digitalRead (buttonPin) == LOW)           // check if the button is pressed
suggests your button connects to Gnd when pressed.

This
pinMode (buttonPin, INPUT);
says you are not utilizing the internal pullup.
Change to
pinMode (buttonPin, INPUT_PULLUP);
to ensure the pin is not floating and leading to what would appear as false low readings and the repeated actions you see.
Designing & building electrical circuits for over 25 years.  Screw Shield for Mega/Due/Uno,  Bobuino with ATMega1284P, & other '328P & '1284P creations & offerings at  my website.

leeamsi97

I already have a pull up resistor. And could someone please elaborate on Canyonero's response?

Delta_G

Canyonero's response is a better way to write time comparisons so that you won't have any trouble when millis hits the rollover point. 

But the failure in your logic is here.

if (time + 5000 >= millis())

This will be true until 5 seconds have passed.  I think you want it to be false until 5 seconds have passed.

Let's think about some real world numbers.

Let's say millis() returns 1000 when you set time.  So when you do this check, millis has gone up a little, let's say to 1001.  Now 1000 + 5000 is 6000.  That will be greater than what is returned by millis until millis finally get's over 6000 in about 5 seconds. 

I would say to rewrite this with a less than, but in reality you should do what canyonero posted and do it with subtraction. 


leeamsi97

So for Canyonero's fix, I know I set "buttonPressTime" when the button is pressed, but when do I set "time"?

Canyonero

#6
Aug 23, 2013, 07:30 pm Last Edit: Aug 23, 2013, 07:32 pm by Canyonero Reason: 1
do a time = millis(); that gets called every time you go through the loop, and stays (relatively) current, make it the first line in your loop.

Then you can do a buttonPressTime = time; when the button is pressed.

wildbill

Or just use millis() instead of time.

leeamsi97

Thanks everyone. I got it to work.

CrossRoads

Designing & building electrical circuits for over 25 years.  Screw Shield for Mega/Due/Uno,  Bobuino with ATMega1284P, & other '328P & '1284P creations & offerings at  my website.

leeamsi97

I used the easier method and changed the greater than to a less than

Go Up