Millis Timer Review

I have a project I am working on that is using Millis. The program below works but it seems like a lot of code for 1 sensor and a LED. The operation is for a sensor on a conveyer belt. The sensor will allow for an object to pass through the sensor. if there is a jam and the sensor is triggered for longer than 1500ms, the program will turn on the led for a jam alert.

static const int SenPin = 2;             // Sensor
const int led = 12;                      // OUT
int SenStatePrevious = LOW;              // Previous state of Sensor
unsigned long MilDelay = 1500;           // Delay before activation
unsigned long SenMillis;                 // Time in ms when we the button was pressed
bool SenState = false;                   // True if Sensor is held on for longer than MilDelay Setting
unsigned long PreviousMillis;            // Timestamp of the latest reading
unsigned long SenON;                     // Sensor triggered

//// GENERAL ////

unsigned long currentMillis;          

void setup() {

  pinMode(SenPin, INPUT_PULLUP);      
  pinMode(led, OUTPUT);


void readSenState() {
   if(currentMillis - PreviousMillis)  {
    int senState = digitalRead(SenPin);
// When sensor is activated by object 
    if (senState == LOW && SenStatePrevious == HIGH) {
      SenMillis = currentMillis;
      SenStatePrevious = LOW;
      Serial.println("Sensor ON");
SenON = currentMillis - SenMillis;  
// Sensor is blocked by object   
    if (senState == LOW && SenON >= MilDelay) {
      SenState = true;
      digitalWrite (led, HIGH);
      Serial.println("Sensor Time Expired");
 //This resets millis if the sensor is open prior to millis time triggering 
    if (senState == HIGH && SenStatePrevious == LOW) {
      SenStatePrevious = HIGH;
      //SenState = false;
      digitalWrite (led,LOW);
      Serial.println("Sensor OPEN");
    // store the current timestamp in PreviousMillis
    PreviousMillis = currentMillis;

void loop() {

  currentMillis = millis();    // store the current time
  readSenState();           // read the button state

What's your question? I don't see any errors and it compiles for me. I'd just behoove you to rename readSenState() since that function does much more than just read the sensor's state.

if ( end_time - start_time >= wait_time ) {

Using unsigned integers, end - start = time difference. This works through rollovers with no change, no if(), same code always works BECAUSE unsigned math makes it so.

Using time on Arduino to do multiple things at once completely explained.

This includes an intro to State Machines which are indispensible code power tools.

The code can be better.

  1. I would do the structure with the 'if' in a different way. If you check if something has changed, and then check for HIGH and LOW, then you have detected the rising and falling edge. I would also make locally used variables local.
int currentJamState = digitalRead( sensorPin);
if( currentJamState != previousJamState)  // the signal has changed
  if( currentJamState == LOW)  // signal turned from HIGH to LOW
    // This is the falling edge of the input signal
  else // signal turned from LOW to HIGH
    // This is the rising edge of the input signal
  previousJamState = currentJamState;     // update the stored value.
  1. There is a variable 'senState' and a variable 'SenState'. That makes the code hard to read. For me, it is almost impossible to read.
  2. You may use names for the variables that say something about the real world. For example bool jamState and unsigned long maximumJamTime = 1500.
  3. The variable 'SenState' is a 'bool' and the variable 'SenStatePrevious' is a 'int', that is inconsistant.
  4. The variable 'SenState' is a 'bool' and can therefore be 'true' or 'false', yet you make it 'HIGH' and 'LOW'.
  5. You already call the pin for the sensor 'SenPin', then you could call the other pin 'ledPin' instead of 'led'.
  6. Please make the text look better, use the indents correctly.
  7. Why do you use: if(currentMillis - PreviousMillis). Why not if(currentMillis - PreviousMillis > 0) or if(currentMillis != PreviousMillis). Why use it at all ? Is there a reason why the sensor input should not be read faster than 1000 times per second ?
  8. You shorting the word 'sensor' to 'sen' for the variables. However, that does not make it better when someone else reads the code. I prefer 'sensorState' and 'sensorValue' and 'sensorPin'. That is just my personal preference. Sometimes I give example code with ridiculous long variables names and let someone else think of a better name.
if (senActive)     // the name might be too short
if (isTheSensorOfTheConveyorBeltActiveAtThisMoment) // let's overdo it
if (jammed)        // good
if (sensorActive)  // good
  1. I prefer to use the variable names that are used in the Blink Without Delay and the State Change Detection.

Is the sensor a mechanical switch ? then it bounces and you need more code to filter the bouncing out.
Is the sensor a detection sensor without bounce ? Then you can read the input as often as you want without additional code.
Is the sensor active low ? Please write that clearly in the sketch as comment.

It's not a great idea to have a variable called senState and another called SenState. Yes, C/C++ variable names are case sensitive, so it will work, but it is confusing and will lead to mistakes, either by you or others reading your code.

By convention, variable names, and function names, in C/C++ begin with a lower case letter and use camelCase. Class names begin with an upper case letter.

it's often better to separate the logic for separate actions.


#undef MyHW
#ifdef MyHW
const int SenPin = A1;
const int led    = LED_BUILTIN;

static const int SenPin = 2;             // Sensor
const int led = 12;                      // OUT

enum { Off = HIGH, On = LOW };

bool senState;
bool senBlocked;

unsigned long senMillis;        // Time in ms when we the button was pressed
unsigned long MilDelay = 1500;  // Delay before activation

// -----------------------------------------------------------------------------
void loop () {
    unsigned long msec = millis ();    // store the current time

    bool sensor = digitalRead (SenPin);
    if (senState != sensor)  {
        senState = sensor;

        if (LOW == sensor)  {
            senMillis        = msec;
            Serial.println ("Sensor ON");
        else  {
            senBlocked = false;
            digitalWrite (led, Off);
            Serial.println ("Sensor OFF");

    if (LOW == sensor && (msec - senMillis) > MilDelay)  {
        if (! senBlocked)  {
            senBlocked = true;
            digitalWrite (led, On);
            Serial.println ("Sensor Blocked");

// -----------------------------------------------------------------------------
void setup () {
    Serial.begin (9600);

    pinMode (SenPin, INPUT_PULLUP);
    senState = digitalRead (SenPin);

    pinMode (led, OUTPUT);
    digitalWrite (led, On);

    Serial.println ("Sensor");

common practice to only Capitalize Constants (and keep variable names short)

Thank you for the input. Here are answers to some of the questions.

The sensor is of an NPN Rectro Reflective type solid state, goes HIGH when beam is interrupted. I do not have any denouncing issues as far as I can tell.

The SenState and senState was starting to confuse me as well, that will be changed.

I do not want slow down the reading, I would like to make the count as fast as possible. I will delete that area.

@gcjr, thank you for the example. I will look at it for ideas.

Imagine that the Arduino clock is a round clock with only a second hand and the numbers starting at top are 0 to 59.

My previous start-interval read was 50 and my current read is 20.
Adding is counting to the right, subtraction counts left.

Starting at 20, I count left 50 and end up on 30 which is elapsed time from 50 to 20.

So what will ( current - previous ) always tell you? Only if they're different or not.