Your code is not bad in terms of efficiency. You have, however,
duplicated code, which is bad in terms of program size, but also makes
the program harder to read and to maintain. A good rule for programmers
is “do not repeat yourself”. Then, when you find yourself writing almost
identical functions, you should try to find a way to merge them into a
single function.
Here it is easy: the only difference between your timer functions is
that they use a different previousMillis
variable. Also, the name of
the currentMillis
variable is different, but it being a local
variable, this difference is irrelevant. Then, to merge them into a
single function, you only have to give to this function a reference to
the relevant PreviousMillis
.
bool timer(unsigned long &last_time, unsigned long period)
{
unsigned long now = millis();
if (now - last_time >= period) {
last_time = now;
return true;
}
return false;
}
And you use it like this:
void loop() {
static unsigned long previousMillis1;
static unsigned long previousMillis2;
static unsigned long previousMillis3;
if (timer(previousMillis1, 1000)) Serial.println(1);
if (timer(previousMillis2, 2000)) Serial.println(2);
if (timer(previousMillis3, 3000)) Serial.println(3);
}
I took the liberty of making all the previousMillis
local to loop()
,
since they are only used there. Making them static
means their values
are remembered between call to loop()
, much like global variables.
In plain C you would use pointers instead of references. It is basically
the same, except that with pointers you are explicitly manipulating
addresses, while this is implicit with references:
bool timer(unsigned long *last_time, unsigned long period)
{
unsigned long now = millis();
if (now - *last_time >= period) {
*last_time = now;
return true;
}
return false;
}
void loop() {
/* [...] */
if (timer(&previousMillis1, 1000)) Serial.println(1);
/* [...] */
}
The next step in code modularity would be to wrap this timer function,
together with the associated data (last_time
and period
), inside a
class:
class Timer
{
public:
Timer(unsigned long _period)
: period(_period), last_time(millis()) {}
bool shouldRun() {
unsigned long now = millis();
if (now - last_time >= period) {
last_time = now;
return true;
}
return false;
}
private:
unsigned long period;
unsigned long last_time;
};
Then, you would use it like this:
Timer timer1(1000); // period = 1000 ms
Timer timer2(2000); // period = 2000 ms
Timer timer3(3000); // period = 3000 ms
void loop() {
if (timer1.shouldRun()) Serial.println(1);
if (timer2.shouldRun()) Serial.println(2);
if (timer3.shouldRun()) Serial.println(3);
}
currentMillis1-previousMillis1
is prone to signed integer overflow, which in C and C++ yields undefined behavior. Meaning it's always incorrect, even if it luckily works with your current compiler.millis()
returns an unsigned long.