1

Trying to do my own timer function. I was wondering if there is any other ways to make my code more efficient without the use of a library. I noticed if I only use one timer function, I wont have multiple instances of the timer. My current code works though.

unsigned long previousMillis1 = 0;
unsigned long currentMillis1 = millis();
unsigned long previousMillis2 = 0;
unsigned long currentMillis2 = millis();
unsigned long previousMillis3 = 0;
unsigned long currentMillis3 = millis();

bool timer1(int preset){
  unsigned long currentMillis1 = millis();

  if(currentMillis1 - previousMillis1 >= preset ){
    previousMillis1 = currentMillis1;
    return true;
  }
  else{
    return false;
  }
}

bool timer2(int preset){
  unsigned long currentMillis2 = millis();

  if(currentMillis2 - previousMillis2 >= preset ){
    previousMillis2 = currentMillis2;
    return true;
  }
  else{
    return false;
  }
}

bool timer3(int preset){
  unsigned long currentMillis3 = millis();

  if(currentMillis3 - previousMillis3 >= preset ){
    previousMillis3 = currentMillis3;
    return true;
  }
  else{
    return false;
  }
}

void setup() {
 Serial.begin(9600);
}

void loop() {
  if(timer1(1000)){Serial.println(1);}
  if(timer2(2000)){Serial.println(2);}
  if(timer3(3000)){Serial.println(3);}
}
4
  • What is a "timer function"? Jun 26 '15 at 5:37
  • Do not store times in signed numbers! Your computation of 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. Jun 26 '15 at 13:27
  • edited for the unsigned. Thanks for that.
    – TL140
    Jun 26 '15 at 23:10
  • Actually, signed longs are just as fine, as long as you compare T2 - T1 > duration. What is actually prone to integer overflows is T2 > T1 + duration. That is, assuming T2 is always "later" than T1 and the difference between them fits in the type.
    – Mishony
    Jun 30 '16 at 19:30
2

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);
}
4
  • Thank you for the input. It is very helpful. I understand classes and objects, but not so much pointers. The main reason I have 3 separate timer functions is because I tried with one single function, however, it would not work correctly, as it only printed the one second timer. Can you at least explain how pointers would work to get around this and allow all 3 timers to function properly? Also, if you don't mind, I would like to see how you wrote the class
    – TL140
    Jun 26 '15 at 23:08
  • If your single function version did not work, that was a bug. You may want to post that buggy code here if you want people to explain what was wrong. If you do not understand pointers, well, this is a big topic that requires some good book or tutorial, some patience, and careful study. Not the kind of thing that can be easily explained here. I edited my answer to include a version with pointers and a class. Jun 27 '15 at 12:41
  • I have experience using industrial controllers. Can i ask, is it similar to "indirect addressing" or "indexed addressing"?
    – TL140
    Jun 28 '15 at 11:02
  • I have no experience with industrial controllers, but pointers are related to “indirect addressing”, in the assembly sense of the term. Jun 28 '15 at 13:40
2

You can simplify a lot by using arrays. The example code below just uses a single variable (array) to store all the time intervals you are interested in.

const int NUMBER_OF_TIMERS = 5;

unsigned long intervalStarted [NUMBER_OF_TIMERS];

void resetTimer (const int whichTimer)
  {
  if (whichTimer < 0 || whichTimer >= NUMBER_OF_TIMERS)
    return;   // sanity clause
  intervalStarted [whichTimer] = millis ();
  }  // end of resetTimer

// interval in milliseconds
bool timeUp (const int whichTimer, const unsigned long whatInterval)
  {
  if (whichTimer < 0 || whichTimer >= NUMBER_OF_TIMERS)
    return false;   // sanity clause
  return  millis () - intervalStarted [whichTimer] >= whatInterval;
  }  // end of timeUp

void setup ()
  {
  Serial.begin (115200);
  Serial.println ();
  Serial.println (F("Starting ..."));
  }  // end of setup

void loop ()
  {

  if(timeUp(0, 1000))
    {
    Serial.println(1);
    resetTimer (0);
    }  // end of if timer 0 up

  if(timeUp(1, 2000))
    {
    Serial.println(2);
    resetTimer (1);
    }  // end of if timer 1 up

  if(timeUp(2, 3000))
    {
    Serial.println(3);
    resetTimer (2);
    } // end of if timer 2 up

  }  // end of loop

I have allowed for 5 "timers" in the code but only used 3 to be consistent with the original post. You can see that you can have as many as you want (until you run out of RAM) with only an overhead of 4 bytes per "timer".

Two functions are provided. One checks if a nominated interval is up, the other one "resets" the timer by setting its start time to now. The checking function should work correctly even during millis() wrap-around, providing you are not trying to time more than 4294967296 milliseconds in one hit (49.71 days).

Note that the timer numbers are zero-relative, so the first timer is timer 0. You could make them one-relative if you prefer that with a slight adjustment to the two functions.

You could also, if you chose to, put the resetTimer function call inside the timeUp function (if the time is up) so that the timer automatically resets. I didn't do that because I think starting timing, and detecting the end of a period could be two different things. For example, when you boil an egg, you start timing from the time the egg goes onto the boil, not from when you last cooked an egg.

1
  • I do not think an array significantly simplifies the code unless you are going to loop through it... which you could do if you attach a callback to each “timer”. Anyway, +1 for providing a rollover-aware example! Jun 27 '15 at 12:51
0

It depends on what kind of efficiency you want. In coding we can often between better execution speed and less memory required. And in the case of Harvard-architected machines such as the AVR devices, which have separate memories for code and data, there may be a further balance between those, too.

But it looks like you're trying to reduce your code space requirement. So notice how alike your three timer functions are. I'd combine those into one, and pass another argument that indicates which timer value to operate on.

Secondly, (and this is probably a bug more than an inefficiency), you have three globals that are not being used (in your example, anyway): long currentMillis1, ..2, & ..3. You have redeclared each of those names as a local variable in the respective timer function, which, within that function over-rides (hides) the global declaration. There is no code that uses those globals.

0

Happened across this page which explains Arduino timer interrupts with out using any libraries. The Atmel timer hardware is discussed and the sketches not only control the hardware directly but also include the actual interrupt service routine. So addresses the OP's question very well.

-1
  1. Those lines like

    currentMillis1;

    are completely useless

  2. You do not really have "timers", especially if your goal is to be more efficient. What you have done is to create some convoluted way to perform a test that could be done as:

    int PERIOD = something; for (int target = millis() + PERIOD; millis() < target;);

  3. If you really care about efficiency and timed actions, check some library that deals with timer interrupts.

  4. Start reading the code of existing libraries and try to understand it. Chances are that who wrote them knew a great deal more than you know. You will learn much more from first reading the code than trying to accomplish something that is clearly too much beyond your current knowledge (the fact you think what you did is an optimization proves it).

4
  • First of all, yes, I took the lines that are useless out. Second of all, I am highly offended by your last comment. You obviously did not read my statement and question above ASKING if there was a way to optimize it. I never stated that I optimized anything. Lastly, isn't this what stack exchange is for? To ask questions? Not everyone is a professional at programming.
    – TL140
    Jun 26 '15 at 10:33
  • I really didn't mean to offend you.But to me, your question shows that you are lacking some concepts. Interrupts, for example. Writing your own optimised ISR would be a way to optimize for real. But is it worth? What you did is likely to be used in a busy loop, doing polling on millis(). Not very optimized, generally speaking. Is it good enough for you? You are the only one who knows, until you say more. So you get the same answer: check if any of the existing libraries works for you, understanding what it does. If it doesn't work for you, come back explaining why it doesn't work for you. Jun 26 '15 at 11:22
  • Your single line example not only does something completely different from the OP's code, its also broken: think about what happens when the first call to millis() yields something bigger than INT_MAX. Jun 26 '15 at 13:33
  • I agree with @EdgarBonet here - your answer is incorrect and even offensive, for no good reason.
    – Mishony
    Jun 30 '16 at 19:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.