Go Down

Topic: Creating a function (Read 1 time) previous topic - next topic

vceklic

Jun 10, 2015, 07:22 am Last Edit: Jun 10, 2015, 07:32 am by Coding Badly
Hi all,

I am trying to create a function that would be used in my program as a generic timer. I have this code so far:

Code: [Select]
bool idleTime(int batteryNumber, unsigned long waitTime){
   bool retVal = false;
   long unsigned timePast = millis() - batPrevMillis[batteryNumber]; //keeping prev millis in array
   
   if( (long)( millis() - waitTime ) >= 0){ //check if time defined has passed
     retVal = true;
     batWaitTime[batteryNumber] = 0UL;
     batPrevMillis[batteryNumber] = 0;
   }
   else
   {        
     if(timePast > 0){
        batWaitTime[batteryNumber] += timePast;  
     }
     else{
        batWaitTime[batteryNumber] += millis();
     }
     batPrevMillis[batteryNumber] = millis();      
   }
   return retVal;
 }


What I am trying to achieve here is get true/false from function if time passed into function has finished. Looks like I am not resetting something  as numbers are off and increase in every loop pass. Any suggestions?

Moderator edit: [code] [/code] tags added.

arduinodlb

#1
Jun 10, 2015, 08:26 am Last Edit: Jun 10, 2015, 08:28 am by arduinodlb
The logic looks all wrong to me.

Don't you just want:

Code: [Select]

bool idleTime(int batteryNumber, unsigned long waitTime){
   if( millis() >= waitTime + batPrevMillis[batteryNumber]){ //check if time defined has passed
     batPrevMillis[batteryNumber] = millis();
     return true;
   }
   return false;
 }
Do not IM me. I will not respond. Ask questions in the forum.

Robin2

#2
Jun 10, 2015, 09:39 am Last Edit: Jun 10, 2015, 09:40 am by Robin2
Code: [Select]
if( millis() >= waitTime + batPrevMillis[batteryNumber]){
does not allow for millis() rollover. You need to use subtraction as in
Code: [Select]
bool idleTime(int batteryNumber, unsigned long waitTime){
   if( millis()- batPrevMillis[batteryNumber] >= waitTime){ //check if time defined has passed
     batPrevMillis[batteryNumber] += waitTime; // prevents timing errors accumulating
     return true;
   }
   return false;
 }


...R
Two or three hours spent thinking and reading documentation solves most programming problems.

arduinodlb

#3
Jun 10, 2015, 09:48 am Last Edit: Jun 10, 2015, 09:51 am by arduinodlb
Code: [Select]
if( millis() >= waitTime + batPrevMillis[batteryNumber]){
does not allow for millis() rollover. You need to use subtraction as in
Code: [Select]

bool idleTime(int batteryNumber, unsigned long waitTime){
   if( millis()- batPrevMillis[batteryNumber] >= waitTime){ //check if time defined has passed
     batPrevMillis[batteryNumber] += waitTime; // prevents timing errors accumulating
     return true;
   }
   return false;
 }

I agree with this line for more accurate timing:
Code: [Select]

     batPrevMillis[batteryNumber] += waitTime; // prevents timing errors accumulating


but why do you say you need you need to use subtraction for this line?:

Code: [Select]

     if( millis() >= waitTime + batPrevMillis[batteryNumber])
Do not IM me. I will not respond. Ask questions in the forum.

sonyhome

when you implement integer logic, you need to account for a value to overflow aka roll over.

millis will jump from maxint to zero suddenly all that while using integers.

arduinodlb

#5
Jun 10, 2015, 11:14 am Last Edit: Jun 10, 2015, 11:18 am by arduinodlb
when you implement integer logic, you need to account for a value to overflow aka roll over.

millis will jump from maxint to zero suddenly all that while using integers.
yes, but that doesn't explain why you need to use subtraction instead of addition.

I'm asking why he changed this line to a subtraction, since this handles the rollover as well:

Code: [Select]

if( millis() >= waitTime + batPrevMillis[batteryNumber])

As far as I can tell, the only necessary change was to stop the minor timing drift.
Do not IM me. I will not respond. Ask questions in the forum.

sonyhome

you use unsigned, hence you generate 0-n > w, not n > w-0,  that is negative but cant be stored. compiler might flag the warning...

Robin2

but why do you say you need you need to use subtraction for this line?:
Because it still gives the correct answer if the interval spans the moment when the value of millis() changes from 232-1 back to 0 . It is one of the features of integer maths.

Try it manually with a byte that rollsover from (say) 253 to 007

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

arduinodlb

#8
Jun 10, 2015, 11:46 am Last Edit: Jun 10, 2015, 03:04 pm by arduinodlb
Because it still gives the correct answer if the interval spans the moment when the value of millis() changes from 232-1 back to 0 . It is one of the features of integer maths.

Try it manually with a byte that rollsover from (say) 253 to 007

...R
Mathematically, it also gives the correct answer with addition. It does not need to use subtraction. Try using addition, it's fine, no?

I do agree that the change to add the waitTime to the last stored time makes timing more accurate, but your claim that subtraction is necessary is inaccurate, so I'm trying to clear that up for everyone.

if millis is now rolled over to say 8,

if 8 > (2^32 - 4) + 6 is true, same as if 8 - (2^32 - 4) > 6
Do not IM me. I will not respond. Ask questions in the forum.

aarg

You're wrong. Subtraction is necessary. This subject comes up from time to time, and the conclusion (after arduous explanations) is always the same. Your theory look great on paper, but you need to test it, then you will see that it will fail on a roll over.
  ... with a transistor and a large sum of money to spend ...
Please don't PM me with technical questions. Post them in the forum.

arduinodlb

You're wrong. Subtraction is necessary. This subject comes up from time to time, and the conclusion (after arduous explanations) is always the same. Your theory look great on paper, but you need to test it, then you will see that it will fail on a roll over.
Well, I can't test it as my arduino is not here, so please explain why it is wrong.
Do not IM me. I will not respond. Ask questions in the forum.

Delta_G

Well, I can't test it as my arduino is not here, so please explain why it is wrong.
You don't need your arduino.  You need a piece of paper.

What happens when the batPrevMillis number is 4,294,967,285 and waitTime is 20 and millis is at 4,294,967,286. 

4,294,967,285 + 20 is going to be 9.  So now you have if 4,294,967,286 > 9.  And yes, it is definitely greater.  But the whole interval hasn't passed yet has it?  You've only gone 1 millisecond.
|| | ||| | || | ||  ~Woodstock

Please do not PM with technical questions or comments.  Keep Arduino stuff out on the boards where it belongs.

arduinodlb

You don't need your arduino.  You need a piece of paper.

What happens when the batPrevMillis number is 4,294,967,285 and waitTime is 20 and millis is at 4,294,967,286. 

4,294,967,285 + 20 is going to be 9.  So now you have if 4,294,967,286 > 9.  And yes, it is definitely greater.  But the whole interval hasn't passed yet has it?  You've only gone 1 millisecond.

Ahhh. OK. That makes sense now. Thanks for explaining it.
Do not IM me. I will not respond. Ask questions in the forum.

vceklic

Thank you all. I understand part with subtraction, but just to clarify, waitTime is time in milliseconds that I pass into function, why do I need to add it to pastBatteryTime array value? Thanks for your help.

arduinodlb

Thank you all. I understand part with subtraction, but just to clarify, waitTime is time in milliseconds that I pass into function, why do I need to add it to pastBatteryTime array value? Thanks for your help.
It only gets added when you have passed the previous waitTime, so it forms the basis of the new starting time to check against.
Do not IM me. I will not respond. Ask questions in the forum.

Go Up