It is possible that the clock folds around.
previousclock = millis() might get stuck at a high value, just before folding around. You could extend the test to include the case that (currentmilis < previousmillis) plus/minus some fudge factor.
BTW It used ignas's source code (the OP sourcecode was not readble without registration, and I don't want to registrate)
EDIT: I copied the fragment below from wakkerbot, and edited it a bit. It is just to
demonstrate how wraparound can get your last_action timestamps stuck at the top of an int interval (if the bump value is not a divisor for int_max)
You could probably simplify the above/below logic a bit, since you are only interested in inside/outside interval tests. The typedef for Stamp should of course be adapted to the type of millis() (unsigned long ?) and fakemillis() removed and references to it replaced by millis().
#include <stdio.h>
#define STAMP_INSIDE 0
#define STAMP_BELOW -1
#define STAMP_ABOVE 1
#define STAMP_BEYONDO -1
/* Intentionally very small, for fast wraparound
** Intentionally signed to stress test the logig.
*/
typedef signed char Stamp;
/* fake clock, returns incrementing value, but folds around
*/
Stamp fakemillis(void)
{
static Stamp ticker =0;
return ticker++;
}
/* Check if "test" is inside or above/below the interval {low,high}
** low and high may have been wrapped around zero (--> low > high)
** return
** 0 := "test" inside interval
** 1 := "test" below interval
** -1 := "test" above interval (but wrapped)
** The two impossible cases return -2.
*/
static int check_interval(Stamp low, Stamp high, Stamp test)
{
switch (4 *(high >= low)
+2 *(test >= low)
+1 *(test > high)
) {
case 0: return STAMP_INSIDE; /* inside (wrapped) */
case 1: /* outside (wrapped) */
return ((Stamp)(low - test) < (Stamp)(test - high)) ? STAMP_BELOW : STAMP_ABOVE;
case 2: break; /* impossible */
case 3: return STAMP_INSIDE; /* inside (wrapped) */
case 4: /* all below */
return ((Stamp)(low - test) < (Stamp)(test - high)) ? STAMP_BELOW : STAMP_ABOVE;
case 5: break; /* impossible */
case 6: return STAMP_INSIDE; /* inside normal case */
case 7: /* all above) */
return ((Stamp)(low - test) < (Stamp)(test - high)) ? STAMP_BELOW : STAMP_ABOVE;
}
return STAMP_BEYONDO;
}
/* Get new clock value, test if it is inside interval {*old, *old+width)
** iff inside: return STAMP_INSIDE;
** iff above (or below) return STAMP_ABOVE or STAMP_BELOW
** and UPDATE *old
*/
static int test_or_set(Stamp *old, Stamp width)
{
Stamp tick;
int diff;
tick = fakemillis();
diff = check_interval( *old, *old+width, tick);
if (!diff) return 0;
*old = tick;
return diff;
}
int main(void) {
Stamp goodlast=0;
Stamp tick=0;
Stamp badlast=0;
int goodtest;
int badtest;
unsigned uu;
for (uu = 0; uu < 260; uu++) {
tick= fakemillis();
if (tick > badlast+10) { badlast=tick; badtest=1; } else {badtest =0;}
goodtest = test_or_set ( &goodlast, 10);
printf("%x:Tick=%x bad=%x, badtest=%d good=%x goodtest=%d\n"
, uu, (unsigned) tick
, (unsigned) badlast, badtest
, (unsigned) goodlast, goodtest
);
}
return 0;
}
If you compile and run the above program on a "normal" computer, you can see the badlast and badtest getting stuck. That is what happens on your arduino, too, IMHO.
Update: definitely overflow/rollover. (GIYF)
http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1200662708
Update2: unrelated, but bad coding practice:
#define CMD_SET_SPEED "S"
...
/* Internal configuration */
if(buffer.substring(0,1)==CMD_SET_SPEED) {
updateSpeed(buffer.substring(1));
}
You are comparing two strings here. (this might be handled as intended by c++, but in C it is just plain wrong. I'd also suggest replacing the repeating if(...) {...} by a giant switch statement, that would at least avoid calling the substr() function repeatedly. (or is it inlined?)
UPDATE 20111211: here is a wrap-oblivious compare&set function it needs a pointer to the value to compare and set, and the width of the intended interval:
int test_and_set_if_beyond( unsigned long *pprev, unsigned long width )
{
unsigned long tick, low,high;
low = *pprev;
high = low+width;
tick = millis();
if (low < high) {
if (tick >= low && tick < high ) return 0; /* normal case */
}
else { /* interval is wrapped , clock could have wrapped */
if (tick >= low || tick < high) return 0;
}
*pprev = tick;
return 1;
}
This function is used in the loop() section as follows:
if (test_and_set_if_beyond ( &lightTimer, lightnessCheckPeriod)) {
int newLightness = analogRead(brightnessPin);
if(newLightness-lightness > LIGHT_TRESHOLD) {
say(RESPONSE_FLASH);
}
lightness = newLightness;
}
if (test_and_set_if_beyond ( &pingTimer, pingTimerPeriod)) {
say(RESPONSE_PING);
}
if (test_and_set_if_beyond ( &pingLEDTimer, pingTimerPeriod*2)) {
digitalWrite(failPin, HIGH);
}
feed();
Finally: IMHO the reason why RESET does not work, is that not all global variables are initialised in the setup() function. Also: I think you should get rid of the String thingies (is there GC in the runtime ?) and use ordinary character buffers instead.
Serialin the recently released version 1.0)? – Emmons