Bug report: Ice Tube

[mkwired]

The expression "wait+ms" can overflow and this case isn't being handled. The function "delayms" can return without waiting long enough.

Code: Select all

volatile uint16_t milliseconds = 0;
void delayms(uint16_t ms) {
  uint16_t wait;
  sei();
  wait = milliseconds;
  while (milliseconds < (wait+ms));
}

// called @ (F_CPU/256) = ~30khz (31.25 khz)
SIGNAL (SIG_OUTPUT_COMPARE0A) {
// ...
  milliseconds++;
// ...
}


[adafruit]

oh right. yeah luckily delayms is not a precision function -at all- and its not so bad if it fails but i'll fix it

change it to this:

Code: Select all

// We have a non-blocking delay function, milliseconds is updated by
// an interrupt
volatile uint16_t milliseconds = 0;
void delayms(uint16_t ms) {
  sei();

  milliseconds = 0;
  while (milliseconds < ms);
}

[madworm_de]

I'll just hijack this thread, as the title applies to my cause as well.

I've poked around inside the clock's firmware available for download on the 'make' site a bit.

a)

It seems to me the "wakeup()" function is never called at all and the going back to normal stuff is handled inside main() already. If the comparator bit indicates a loss of power it calls "gosleep()", if not it activates or re-activates all normal functions there.

This doesn't break the clock, but the wakeup() function should be marked as "unused" if you keep it in there for reference or educational purpose.

b)

it would be nice to add a comment to the hold-a-button checking routine to go and look at the other ISR that ticks every 1s.

there's some code like

Code: Select all

while(buttonhold) {
...
}

and that variable is decreased inside the other ISR. At first glance that code would never exit the while loop if you keep the button pressed.

[adafruit]

oh yah, actually i made a few new mods to the firmware since last uploading
but yes wakeup() was taken out when the WDT was put in
the code needs a little more commenting as well. but theres a lot of it and i did my best for now
im still stalking one more annoying rare bug that causes the clock to not restore the proper time on power loss. its not a terrible bug but its annoying and happens once in a while. grr!

[mkwired]

I've started to modify the code to make it easier for me to follow the interactions between the "logical" blocks of the firmware. I might totally refactor the code to use a state machine so there's no need to sleep inside an ISR. I might have to buy one of these clocks to see if my modifications work.