Go Down

Topic: maximum delay() is 32767ms ? (Read 8311 times) previous topic - next topic

TomP

From playing with it, it looks like the argument to the delay() function is treated as a signed int, making the maximum delay something like 32767 milliseconds.  Is that correct?  If so, it would be helpful to update the reference page for delay() to make this clear.  Currently there's no mention there of a maximum possible delay.  

I noticed this when I tried setting a delay of 60*1000 (one minute) and found that my sketch just hung at that point.  A delay of 30000 works, on the other hand.

Thanks.

TomP

My bad.  I see now (from wiring.h) that the delay() function takes an unsigned long, so it should work for much longer delays than I was trying to use.  My mistake was multiplying a signed integer variable and a signed integer constant to get the delay time.  Making the variable unsigned long fixed things.

Sorry for the distraction...

tkbyd

#2
Feb 13, 2008, 10:43 am Last Edit: Feb 13, 2008, 10:49 am by tkbyd Reason: 1
"Your bad".... NOT!

I'm puzzled why the 60 * 1000 didn't work but passed (I presume?) the compiler's validity tests, and would welcome claification on that point.

And even though it wasn't the answer to what you were seeing, I'd welcome what you suggested. If someone who knows the answer would modify the entry at....

http://www.arduino.cc/en/Reference/Delay

... to make explicit what the largest acceptable arguement is, I think it would be a good addition. The entries for miilis and pulseIn are very good in this respect. It would be great if the explicit statement of different functions needs were to be extended.

mellis

I just did a few quick tests, and I think the issue is not with the delay() function, but with the multiplication.  The compiler treats an "integer literal" (e.g. 60 or 1000) as an "int" (which can hold numbers from -32,768 to 32,767) not as a long (which goes from negative to positive 2 billion or so).  So when you try to multiply the two integers 60 and 1000, the result (60,000) is bigger than what can fit in an int, and so it overflows to a negative number.  Then delay() gets called with a negative number (which actually gets turned into a very large positive one, since delay() takes an unsigned long), which makes it hang.  You can get around this by putting an L after one or both of the numbers, e.g. delay(60L * 1000L); (as described, though not in much detail on the integer constants page).

I'll add something about not passing a negative number to delay(), and try to clarify what happens when arithmetic overflows on the arithmetic page.  And also the fact that numbers like 60 are treated as integers (not longs).

kg4wsv

Could some C++ strong typing, function overloading, and an extra version or three of delay() take care of this problem?  Maybe one delay() for each of the following input types: int, unsigned int, long, unsigned long.

Of course, there's not a lot of room on the ATmega for code bloat...

-j


TomP

Quote
Could some C++ strong typing, function overloading, and an extra version or three of delay() take care of this problem?  Maybe one delay() for each of the following input types: int, unsigned int, long, unsigned long.


As David explained, the problem is fundamentally that 60*1000 does not equal 60,000 here, because the result is an int (a signed 16 bit integer).  There's no way to catch that at the compiler level (that I can think of, anyway); you need to promote one or both of the constants to a long (32 bit integer) to get a result of 60000.  

The best you could do (I think) would be have a version of delay that accepted an int, and which would barf when given a negative delay.


mellis

I do want to look through the gcc documentation, though, because it's smart enough to treat 60000 (for example) as a long, even without an L at the end.  Maybe there's an option to tell it to treat all integer literals as longs (or something similar).

tkbyd

In the specific case we're discussing this probably is not an answer... it was not noticing the type issues that led to the problem, after all.

But when the programmer is aware of a type issue, maybe something along the following lines would help?.....

longint k60=60;
longint k1000=1000;

delay(k60*k1000);

(Although what's in k60, k1000 could be altered, of course, I use the "k" prefix to tell myself that the variable is to be used as a konstant.)

mem

I wonder if the function name delay was replaced by a macro that cast the parameter to an unsigned long would solve the problem.

#define delay(_val)  _delay((unsigned long) _val)

where _delay is the existing  function renamed


xSmurf

Wouldn't casting work fine? delay((uint16_t) 60 * 1000); ?

My 2 cents...
"Pilots believe in a clean living... they never drink wisky from a dirty glass."

mellis

Casting should work as does appending L to the integer literals (e.g. 60L).  But, of course, that only helps if you know you need to do it.  I'm not sure of the best way to help people that don't know they need to do that.

mem

#11
Feb 15, 2008, 08:45 pm Last Edit: Feb 15, 2008, 08:45 pm by mem Reason: 1
If the arduino distribution had delay declared as a macro (with the actual function renamed to _delay) then this would do the cast without the user knowing anything about it. It would look exactly the same as now but would operate as a long.

TomP

Quote
If the arduino distribution had delay declared as a macro (with the actual function renamed to _delay) then this would do the cast without the user knowing anything about it. It would look exactly the same as now but would operate as a long.

Again, the problem is with the arithmetic, not with the delay() function.  When you multiply the 16-bit signed integers 60 and 1000, you don't get 60000, but rather -5536.  (The biggest number you can represent as a 16-bit signed integer is 32767.)  Casting that result to an unsigned long doesn't get you 60000 back.  You need to do something to make sure the arithmetic is carried out using unsigned longs (32-bit integers) in the first place. It's not clear there's any convenient way to do that automatically, however.

A way of helping people avoid this mistake in the first place might be to provide special delay_*() functions for longer time units, and then document clearly what the maximum argument for each is.  If there were a delay_minutes() or delay_seconds() function, for instance, you wouldn't need to multiply 60*1000 to get the delay.  These special variants could easily be implemented as macros, e.g.,

#define delay_seconds (_val)   delay(1000L * (_val))

It doesn't fix the problem, of course, but ought to make it less likely that people get bitten by it.


mellis

That's not a bad idea.  I've also been thinking we should add things like seconds() and minutes() to go along with the millis() function.

mem

Quote
Quote
If the arduino distribution had delay declared as a macro (with the actual function renamed to _delay) then this would do the cast without the user knowing anything about it. It would look exactly the same as now but would operate as a long.

Again, the problem is with the arithmetic, not with the delay() function.  When you multiply the 16-bit signed integers 60 and 1000, you don't get 60000, but rather -5536.  (The biggest number you can represent as a 16-bit signed integer is 32767.)  Casting that result to an unsigned long doesn't get you 60000 back.  You need to do something to make sure the arithmetic is carried out using unsigned longs (32-bit integers) in the first place. It's not clear there's any convenient way to do that automatically, however.
Casting the parameter in a macro to an unsigned long forces the compiler to do the entire calculation as a long in other platforms I have used. Are you saying that the suggestion to use a macro with the cast wont work here?

Go Up