Black Lives Matter - Action and Equality. ... Adafruit is open and shipping.
0

Bug report: Ice Tube
Moderators: adafruit_support_bill, adafruit

Please be positive and constructive with your questions and comments.

Bug report: Ice Tube

by mkwired on Tue Aug 25, 2009 2:47 pm

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 | TOGGLE FULL SIZE
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++;
// ...
}

mkwired
 
Posts: 7
Joined: Tue Aug 25, 2009 2:40 pm

Re: Bug report: Ice Tube

by adafruit on Wed Aug 26, 2009 11:31 am

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 | TOGGLE FULL SIZE
// 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);
}

adafruit
 
Posts: 12151
Joined: Thu Apr 06, 2006 4:21 pm
Location: nyc

Re: Bug report: Ice Tube

by madworm_de on Wed Aug 26, 2009 5:49 pm

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 | TOGGLE FULL SIZE
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.
NO LARDO CHIPS
madworm_de
 
Posts: 99
Joined: Mon Jun 09, 2008 6:56 am

Re: Bug report: Ice Tube

by adafruit on Wed Aug 26, 2009 9:46 pm

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!

adafruit
 
Posts: 12151
Joined: Thu Apr 06, 2006 4:21 pm
Location: nyc

Re: Bug report: Ice Tube

by mkwired on Wed Aug 26, 2009 10:35 pm

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. :D

mkwired
 
Posts: 7
Joined: Tue Aug 25, 2009 2:40 pm

Please be positive and constructive with your questions and comments.