Debounce routine works once only

I'm writing a debounce routine using my timer class, this works once and once only until I reset then its the same, quite repeatable...

bool clsApp::debounceDI(uint8_t ud8DI, int intState, unsigned udMS) {
  bool blnResult = false;
  clsTimer* pTmr = NULL;
  int intCurrent;

  while( (intCurrent = digitalRead(ud8DI)) == intState ) {
    if ( pTmr == NULL ) {
// Install timer to count up to debounce time
      pTmr = createTimer( udMS );
    } else if ( pTmr->getExpired() == true ) {
// Timer has expired whilst state is still at specified value
      blnResult = true;
      break;
    }
  }
  if ( pTmr != NULL ) {
    delete pTmr;
  }
  return blnResult;
}

The timer is managed in an ISR, when the timer expires the interrupt sets a flag, the getExpired method checks the expired flag. I can't see anything wrong or explain why this only works once. The application loop does nothing except call this routine:

  if ( objApp.debounceDI(RESET_BUTTON, LOW, 200) == true ) {
    digitalWrite(13, HIGH);    
  } else {
    digitalWrite(13, LOW);    
  }

What does createTimer() do? Does it really return a pointer?

I was about to ask that. And as for:

 if ( pTmr != NULL ) {
    delete pTmr;
  }

The delete keyword is defined to work will NULL pointers, so you can just do:

    delete pTmr;

And what does pTmr->getExpired() do?

When it 'goes wrong', what does it do?

createTimer is a wrapper that creates an instance of clsTimer and returns a pointer to the object.

pTmr->getExpired() returns the internal member variable m_blnExpired, which is set by the ISR when the timers counter set point is reached.

The first time it works perfectly, after that it just doesn’t trigger the LED again.

When I asked what they did I was kinda hoping for code.

The full code for clsTimer is posted here: http://arduino.cc/forum/index.php/topic,88544.0.html

The code for createTimer just calls new clsTimer passing the parameters on.

Right. So if I have this straight the timer library relies on interrupts to call checkTimers. I don’t quite see the point of:

  TCNT2 = 0x06;

… but we’ll let that pass.

checkTimers sets a flag on expired timers.

I can’t see getExpired() on the page you quoted.

Did you download the zip clsTimers.zip? I stopped pasting the code and just updated the zip. It is there, it has to be, code compiled without any warnings.

TCNT2 = 0x06;

Is a oversight, I should have deleted it...will remove it, but I don't think that's anything to do with the problem.

Got the .zip now.

// Perform call-back  
    (*pTimer->m_pCallback)(pTimer);

Isn't m_pCallback NULL in this case?

That all seems needlessly complicated when you ultimately just need to know when a specified number of milliseconds has elapsed. Can you not imagine a simpler way to achieve that?

    if ( pTimer->m_pCallback != NULL ) {
// Perform call-back  
      (*pTimer->m_pCallback)(pTimer);
    }

The if condition before the callback ensures that the callback is only performed if a callback has been supplied. In this case a callback has not been supplied.

@PeterH, I am building a suite of classes and functions, of course it can be done other ways, however I want to create a re-entrant portal class library.

What "if"? I downloaded the .zip file, and this is the code I found:

/**
 * Should be called in main loop to service any expired timers
 */  
void clsTimer::serviceTimers() {
  for( clsTimer* pTimer=(clsTimer*)pAllTimers; pTimer!=NULL; 
       pTimer=pTimer->next() ) {
    if ( pTimer->m_blnExpire == false ) {    
      continue;
    }
// Suspend this timer whilst we process it        
    pTimer->suspend();      
// Timer has expired, clear flag
    pTimer->m_blnExpire = false;
    
    if ( blnDebugFlag == true ) {
      Serial.print(pTimer->interval());
      Serial.println( "ms timer expired!" );
    }    
// Set initial call flag so we know the next call will be using the repeat time
    pTimer->m_blnCalled = true;
// Perform call-back  
    (*pTimer->m_pCallback)(pTimer);
// Resume processing of this timer        
    pTimer->resume();
                
    if ( pTimer->m_uintRepeat > 0 ) {
      pTimer->m_uintCntDown = pTimer->m_uintRepeat;
    } else { 
// This timer is a one-shot timer, remove it!
      delete pTimer;
    }      
  }
}

Here:

void clsTimer::serviceTimers() {
  for( clsTimer* pTimer=(clsTimer*)pAllTimers; pTimer!=NULL; 
       pTimer=pTimer->next() ) {
...
// This timer is a one-shot timer, remove it!
      delete pTimer;
    }      
  }
}

You delete pTimer, and then access pTimer->next. That isn't allowed - well you can do it, if you don't mind undefined behaviour.

In that case I haven't uploaded the very latest version, very sorry...will see to that now.

I do see your point in serviceTimers and will see to that before I upload new release.

That could be your problem. You make a once-off timer, then when you delete it you potentially corrupt memory. So it would work the first time.

Ok, new version uploaded hopefully with a fix for the linked list indexing, however looking at the code, the delete is only performed for timers with a callback, this timer doesn't.

Isn't this taking encapsulation too far?

/**
 * Gets the next timer object in the linked list
 */        
clsTimer* clsTimer::next() {
  return m_pNext;
}

You have a private function call that just returns a private variable. Isn't that just adding overhead and obscurity?

I mean, reading your code I have to work out that:

pAllTimers = this->next();

... is the same as:

pAllTimers = this->m_pNext;

or indeed:

pAllTimers = m_pNext;

The qualifier "this" is implied in class variables. Spelling it out makes me wonder why you do that.