Is there a better way to do this? Do A for X min, then B for Y minutes, then A for Z minutes

I’m wondering if there is a better way to create this timer. I want the device to be locked for X minutes, then unlocked for Y minutes, and after the Y minutes, will relock indefinitely.

There is also an interrupt loop that collects, and a screen to display it, but I removed them for the sake of keeping the code short for this post.


//setting up timer

long unlock = 5000;           // milliseconds to unlock device
long relock = 10000;  // milliseconds to relock the device 
int lockedpos = 120;  //servo position to lock
int openpos = 30; //servo position to unlock

Servo myservo; //setting up servo name


void setup()
{
 //servo setup and initial position
  myservo.attach(9);
 

}

void loop()
{ 
  unsigned long currentMillis = millis();  //this needed to be within the loop for some reason


  if ((currentMillis <= unlock)) {  //if time is less than the unlock time, keep the device locked

    myservo.write(lockedpos);
         
 } 

  else if ((currentMillis >= unlock) && (currentMillis <= relock)) { //if time is past the unlock time, but before the relock time, unlock the device

    myservo.write(openpos);
    }
     else if (currentMillis >= relock)  { //if we are past relock time, lock the device

    myservo.write(lockedpos);
    }

My immediate reaction to your question was to suggest using a state machine using a variable for the current state and switch/case on the value of that variable and millis() for timing of course.

That way you don’t even need to use interrupts for data collection or updating a screen

Since you are using ‘else’ (good!), some of your tests are redundant and can be removed.

void loop()
{
  unsigned long currentMillis = millis();  //this needed to be within the loop for some reason

  if (currentMillis <= unlock)    //if time is less than the unlock time, keep the device locked
  {
    myservo.write(lockedpos);
  }
  else if (currentMillis <= relock)   //if time is past the unlock time, but before the relock time, unlock the device
  {
    myservo.write(openpos);
  }
  else   //if we are past relock time, lock the device
  {
    myservo.write(lockedpos);
  }
}

The code would be a little easier to understand and change if you add some named constants:

const unsigned long SECONDS = 1000UL;
const unsigned long MINUTES = 60 * SECONDS;
unsigned long unlock = 5 * SECONDS;    // milliseconds to unlock device
unsigned long relock = 10 * SECONDS;  // milliseconds to relock the device 

I’m reading into the state machines and they are a little bit over my head.

Would it work so that within each state I would have my data collection repeated and therefore not need the interrupt?

Thank you!

I guess the double conditions are unnecessary. Out of curiosity, does running two conditions in the else if statement slow the loop down substantially?

No. The compiler is usually smart enough to not generate code for redundant conditions. It mostly just makes the code harder for humans to read.

No. The code for the current state runs but it does not hold up the free running of loop() so that you only need to put the input detection and output code in loop()

An example to illustrate the principle

enum states
{
  stateAX,
  stateBY,
  stateAZ
};

byte currentState;
unsigned long stateStartTime;
unsigned long statePeriod;
unsigned long currentTime;

void setup()
{
  Serial.begin(115200);
  while (!Serial);
  currentState = stateAX;
  stateStartTime = millis();
  statePeriod = 5000;
  reportState("stateAX");
}

void loop()
{
  currentTime = millis();
  switch (currentState)
  {
    case stateAX:
      if (currentTime - stateStartTime > statePeriod)
      {
        stateStartTime = currentTime;
        statePeriod = 2000;
        currentState = stateBY;
        reportState("stateBY");
      }
    case stateBY:
      if (currentTime - stateStartTime > statePeriod)
      {
        stateStartTime = currentTime;
        statePeriod = 10000;
        currentState = stateAZ;
        reportState("stateAZ");
      }
    case stateAZ:
      if (currentTime - stateStartTime > statePeriod)
      {
        stateStartTime = currentTime;
        statePeriod = 5000;
        currentState = stateAX;
        reportState("stateAX");
      }
  }
  //non blocking general code goes here
}

void reportState(char * newState)
{
  Serial.print("entering ");
  Serial.print(newState);
  Serial.print(" for ");
  Serial.println(statePeriod);
}

I’m going to give this a shot when I’m back in the lab on Monday. I’m reading up on the states and cases right now.

The rest of my code contains:

  • a hall sensor that counts revolutions
  • a screen that displays the revolutions and time
  • a servo that locks the device

Based on your other comment, the sensor and screen code can go outside of the cases where you have “non blocking general code goes here” and the servo locking code would go inside of whatever state I want to lock and unlock, like you did with the reportState loop. Am I getting that right?

Sounds correct, although I would point out that reportState() is not a loop but a function

Let us know how you get on

You could try it this way…

#include <Servo.h>
#include <timeObj.h>

float unlock = 5000;       // milliseconds to unlock device
float relock = 10000;      // milliseconds to relock the device 
int   lockedpos = 120;     // servo position to lock
int   openpos = 30;        // servo position to unlock


Servo    myservo; //setting up servo name
timeObj  unlockTimer(unlock);
timeObj  relockTimer(relock);

void setup() {
   
    myservo.attach(9);        // servo setup.
    myservo.write(lockedpos); // initial position.
}

void loop(void) {

   if (unlockTimer.ding()) {     // If the unlock timer has expired..  >ding<
      myservo.write(openpos);    // Open the lock.
      unlockTimer.reset();       // SHut down the unlock timer.
   }
   if (relockTimer.ding()) {     // If the relock timer has expired.. >ding<
      myservo.write(lockedpos);  // Reclock the lock.
      relockTimer.reset();       // Shut down the relock timer.
   }
}

You’ll need to grab LC_baseTools from the IDE library manager to compile this.

Good luck!

-jim lee

Okay, good news is the code works when I don’t have any of my garbage in it. Bad news is when I add the fun parts (screen, servo, hall effect sensor), shit hits the fan. I’m using a lot of pieces that I already have working from my if/else version.

I’m getting a lot of “not declared in this scope” starting at my void setup. Is this an error on how I’m formatting things? I tried to annotate it as best I could

I also need to wrap my head around how state names work. I think understand that code that is running during the state determines when to move to the next state - it just gets jumbled when the state for “lock” has an output that says “unlock”

#include <Servo.h>
#include <Arduino.h>
#include <U8g2lib.h>
#ifdef U8X8_HAVE_HW_SPI
#include <SPI.h>
#endif
#ifdef U8X8_HAVE_HW_I2C
#include <Wire.h>
#endif


int lockedpos = 120;  //servo position to lock
int openpos = 30; //servo position to unlock
Servo myservo; // servo
int hallsensor = 2; //hall sensor pin

volatile long count; //rotation count



enum states
{
  lock,
  unlock,
  relocked
};

byte currentState;
unsigned long stateStartTime;
unsigned long statePeriod;
unsigned long currentTime;

void setup()
{
  Serial.begin(115200);
  while (!Serial);
  currentState = lock;
  stateStartTime = millis();
  statePeriod = 5000;
  reportState("locked");
  count = 0;

  // this is where errors begin, lots of "not declared in this scope" this code works fine when paired with my if/else version
  u8g2.begin();
  u8g2.setFlipMode(1);
  u8g2.setFont(u8g2_font_t0_11_mf); // choose a suitable font
  u8g2.setCursor(8 , 8);
  u8g2.clearBuffer();
  u8g2.sendBuffer();
  attachInterrupt(digitalPinToInterrupt(hallsensor), sensor, FALLING); //attach interrupt
  pinMode(hallsensor, INPUT); //setup hall sensor
  myservo.attach(9); //setup pin for servo

}

void loop()
{
  currentTime = millis();
  switch (currentState)
  {
    case lock:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(openpos);
        stateStartTime = currentTime;
        statePeriod = 2000;
        currentState = unlock;
        reportState("unlocked");
        screen()
      }
    case unlock:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(lockedpos);
        stateStartTime = currentTime;
        statePeriod = 10000;
        currentState = relocked;
        reportState("relocked");
        screen()
      }
    case relocked:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(lockedpos);
        stateStartTime = currentTime;
        statePeriod = 5000;
        currentState = lock;
        reportState("lock");
        screen()
      }
  }
  //non blocking general code goes here
//empty because still using interrupt. Maybe put screen() here??

}

// serial monitor code 
void reportState(char * newState)
{
  Serial.print("entering ");
  Serial.print(newState);
  Serial.print(" for ");
  Serial.println(statePeriod);
}


//interrupt code
}
void sensor () {
  count++; //every time hall sensor goes off, add 1 to count
}


//OLED screen display
void screen () {
  u8g2.setFont(u8g2_font_t0_11_mf); // choose a suitable font
  u8g2.setCursor(8 , 8); //curser set
  u8g2.clearBuffer(); //clear screen
  u8g2.print(count); //print revolutions
  u8g2.setCursor(16 , 16); //curser set
  u8g2.print(millis() / 1000); //print time in seconds
  u8g2.sendBuffer(); //send that baby to the screen
}

Correct

Each state prints the name of the state that it is moving to as part of getting ready to move to the next state

As to the errors starting at

  u8g2.begin();

Where in the code is the u8g2 constructor in your sketch ? It should look something like this

U8G2_SSD1306_128X64_NONAME_F_4W_SW_SPI u8g2(U8G2_R0, /* clock=*/ 13, /* data=*/ 11, /* cs=*/ 10, /* dc=*/ 9, /* reset=*/ 8);

but needs to match your display

There is at least one other problem with your code. Here is a section of it

// serial monitor code
void reportState(char * newState)
{
  Serial.print("entering ");
  Serial.print(newState);
  Serial.print(" for ");
  Serial.println(statePeriod);
}


//interrupt code
}
void sensor ()
{
  count++; //every time hall sensor goes off, add 1 to count
}

There is an extra } in that section and there may be other errors

I must have rocks for brains, I didn’t copy over the constructor from my other sketch. Thanks for catching that.

I also missed a bunch of ;;;;;;;;'s. I had to move the screen function outside of the state case part because it wasn’t updating the screen every second. But now it works well!

Until it didn’t. I think I have a hardware problem. I’m attaching the diagram of what I’m working with. The screen and servo are still going strong, but the count function isn’t working anymore. The hall effect sensor is still lighting up when I put the magnet near it and I tried two different ones without any more success. Maybe it’s the double ground I have going on?

Also, what would be the metric I should use to determine which version of the code to go with (state/case vs if/else)

Go with the version that you are happiest with. Personally I favour switch/case combined with an enum to define the cases as to me it is easier to read and maintain than if/else but YMMV

If I get a chance I will look at your circuit later but please post the code that is causing the problem

Decisions, decisions. I like this version because it’s neat and tidy. The code below is just the fixed version. Everything works, but count++ stopped counting :confused: Same with the if/else version, which makes me think I broke the nano…

#include <Servo.h>
#include <Arduino.h>
#include <U8g2lib.h>
#ifdef U8X8_HAVE_HW_SPI
#include <SPI.h>
#endif
#ifdef U8X8_HAVE_HW_I2C
#include <Wire.h>
#endif


int lockedpos = 120;  //servo position to lock
int openpos = 30; //servo position to unlock
Servo myservo; // servo
int hallsensor = 2; //hall sensor pin

volatile long count; //rotation count

U8G2_SSD1306_128X64_NONAME_F_HW_I2C u8g2(U8G2_R0, /* clock=*/ A5, /* data=*/A4, /* reset=*/ U8X8_PIN_NONE);

enum states
{
  lock,
  unlock,
  relocked
};

byte currentState;
unsigned long stateStartTime;
unsigned long statePeriod;
unsigned long currentTime;

void setup()
{
  Serial.begin(115200);
  while (!Serial);
  currentState = lock;
  stateStartTime = millis();
  statePeriod = 5000;
  reportState("locked");
  count = 0;

  // this is where errors begin, lots of "not declared in this scope" this code works fine when paired with my if/else version
  u8g2.begin();
  u8g2.setFlipMode(1);
  u8g2.setFont(u8g2_font_t0_11_mf); // choose a suitable font
  u8g2.setCursor(8 , 8);
  u8g2.clearBuffer();
  u8g2.sendBuffer();
  attachInterrupt(digitalPinToInterrupt(hallsensor), sensor, FALLING); //attach interrupt
  pinMode(hallsensor, INPUT); //setup hall sensor
  myservo.attach(9); //setup pin for servo

}

void loop()
{
  currentTime = millis();
  switch (currentState)
  {
    case lock:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(openpos);
        stateStartTime = currentTime;
        statePeriod = 2000;
        currentState = unlock;
        reportState("unlocked");
      
      }
    case unlock:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(lockedpos);
        stateStartTime = currentTime;
        statePeriod = 10000;
        currentState = relocked;
        reportState("relocked");
       
      }
    case relocked:
      if (currentTime - stateStartTime > statePeriod)
      {
        myservo.write(lockedpos);
        stateStartTime = currentTime;
        statePeriod = 5000;
        currentState = lock;
        reportState("lock");
        
      }
  }
  //non blocking general code goes here
screen();

}

void reportState(char * newState)
{
  Serial.print("entering ");
  Serial.print(newState);
  Serial.print(" for ");
  Serial.println(statePeriod);
}


void sensor () {
  count++; //every time hall sensor goes off, add 1 to count
}

void screen () {
    Serial.println(count);
  u8g2.setFont(u8g2_font_t0_11_mf); // choose a suitable font
  u8g2.setCursor(8 , 8); //curser set
  u8g2.clearBuffer(); //clear screen
  u8g2.print(count); //print revolutions
  u8g2.setCursor(16 , 16); //curser set
  u8g2.print(millis() / 1000); //print time in seconds
  u8g2.sendBuffer(); //send that baby to the screen
}

Which Arduino board are you using ?

An arduino nano v3 clone

When you compile your sketch do you get a warning about low memory availability and saying that stability problems may occur ?

One version of the sketch has:
42% of storage space used
86% of dynamic memory used

another version has
44% of storage space used
88% of dynamic memory used

Both have the stability problem error. I'm guessing 12-14% is not enough?

EDIT: I think I can cut down the memory usage by changing the u8g2 code. Let me give it a quick shot.

EDIT2: I switched over to U8x8 and it runs muuuuch faster, but still no counting. Looking at 28% and 29% usage of storage and memory. Is it possible I blew out a pin?

EDIT 3: moved to a different pin, still no dice

EDIT 4: hall effect sensor needed 5V input, works now