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 
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 ?
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
