Jump to content
43oh

Exercise: Robust Digital Edge Detection


Recommended Posts

Concrete is good, but then people tend to focus on details that make the solution useful only in limited cases (like the ability to change the hardware). I'm aiming for a problem description that captures only the most general requirements.

 

Unless I'm mistaken, the problem with my original solution is also present in your code.

Hmmm... all I can think up at this point is potential state divergence between the various statements of code (e.g. P2IFG differing from what the state was when P2IN was read, and likewise later on in my code P2IES is hinging on the previously-read value of P2IN rather than allowing for interpretation of transient events that may have emerged during the graycode interpretation portion).

 

That is a problem, and probably something I could do better; for this application I was creating a lamp remote control for changing colors on a water-jug lamp, but for e.g. an encoder detecting gasoline motor position I would think harder on it.  Probably switch around the if(PxIFG) code to use a ... while (PxIFG & (BIT4 | BIT)) { p2in = P2IN; .... blahblah, PxIES setting only the bits that changed, PxIFG clearing only the bits that were initially set } I guess.

Link to post
Share on other sites

We have to worry about an edge happening in our critical section initialization. So change initialization as follows.

__disable_interrupt();
uint8_t back = PxIES;
PxIFG = 0;
PxIES = PxIN;

/* According to slau144j 8.2.7.2 It's not possible to trigger a fake transition
 if PxIES is getting set to the same values as PxIN. So if there is a bit
on PxIFG we did get an edge just after we set PxIES. */
uint8_t snap = PxIFG;
PxIFG = 0;
if(snap & BITN){
  if(PxIES & BITN)
    /* emit falling */
  else
    /* emit rising */
}

PxIES &= BITN;
back &= ~BITN;
PxIES |= back;
PxIE |= BITN;
/* Assume PxIFG is still zero because we don't worry about transients.*/
__enable_interrupt();

And our interrupt handler:

uint8_t snap = PxIFG;
PxIFG = 0;
if(snap & BITN){
 PxIES ^= BITN;
 if(PxIES & BITN)
  /* emit rising */
 else
  /* emit falling */
}
Link to post
Share on other sites

@@spirilis That's the idea, though the lost transitions aren't necessarily transient.

 

@@David Bender That's a better identification of the problem, but your solution needs some tuning. That you're affecting the interrupt status of other pins on the same port could be fixed, but fundamentally there are still gaps. You might simplify the code by using PxIN instead of trying to sample the current state via PxIES+PxIFG, which could make the gaps more obvious.

 

addendum: there is no need to emit events during initial configuration; we just need to know the state.

Edited by pabigot
Link to post
Share on other sites

@@spirilis That's the idea, though the lost transitions aren't necessarily transient.

 

@@David Bender That's a better identification of the problem, but your solution needs some tuning. That you're affecting the interrupt status of other pins on the same port could be fixed, but fundamentally there are still gaps. You might simplify the code by using PxIN instead of trying to sample the current state via PxIES+PxIFG, which could make them more obvious.

Ok, so going back to your original example, the ISR code...

  while ( (PxIN & BITn) != state ) {
    PxIFG &= ~BITn;  // 1 : Clear handled interrupt
    PxIES ^= BITn;   // 2 : Configure to detect change in state
    state = ! state; // 3 : Record the state change indicated by the interrupt
    if (state) {
      // 2a : emit rising edge event
    } else {
      // 2b : emit falling edge event
    }
    // PxIFG may have re-triggered in the meantime
  }
Link to post
Share on other sites

Edited my last post to use while ( (PxIN & BITn) != state ) because:

 

slau144j page 332

NOTE: Writing to PxIESx

Writing to P1IES, or P2IES can result in setting the corresponding interrupt flags.

 

"can" being the operative word...

 

... Or maybe I should say:

while ( (PxIFG & BITn) || ((PxIN & BITn) != state) )

?

Then I have to handle the fact that I may have missed two transitions within my code segment. (one that set PxIFG, and the other that ended with PxIN & BITn == state).

Link to post
Share on other sites

Alright, the problem's been identified and we're now down in the weeds. Rather than point out issues in solutions-in-progress, below is my explanation of the flaw and how I'm solving it. Please point out anything that seems wrong or doesn't satisfy the requirements as stated. Those who want to keep trying should not read this post yet.

 

The key phrase, as is so often the case, is: "race condition"

 

The MSP430 features that impact it are (1) the PxIFG register records only edge transitions, not signal level, and (2) disabling interrupts only delays notification of a detected event, it does not delay recording the event.

 

Configure:

  __disable_interrupt();
  state = PxIN & BITn; // 1 : record state
  if (state) {
    PxIES |= BITn; // 2a : High, detect falling edge
  } else {
    PxIES &= ~BITn; // 2b : Low, detect rising edge
  }
  PxIE |= BITn; // 3 : Register to receive interrupts on edge changes
  PxIFG &= ~BITn; // 4 : Reset interrupts from past changes
  __enable_interrupt();

 

What's wrong here is: At any point after (1) the pin state may change. If it changes an odd number of times before (3), we'll be configured to detect the wrong edge, and the state will always be opposite the real value.

 

Monitor (within PORTx interrupt handler, assume PxIV is NOT used):

  state = ! state; // 1 : Record the state change indicated by the interrupt
  if (state) {
    // 2a : emit rising edge event 
  } else {
    // 2b : emit falling edge event
  }
  PxIFG &= ~BITn;  // 3 : Clear handled interrupt
  PxIES ^= BITn;   // 4 : Configure to detect change in state

 

This has two problems.

 

First, if the transition occurred during a long period with interrupts disabled, the state may have changed back prior to the interrupt handler being invoked, so again we get out of sync. One might argue that such a change should be characterized as "transient" even if it exceeded the arbitrary 20-cycle limit. From the perspective that leaving interrupts disabled that long is poor design I'd probably concede the point, so this quibble is weak.

 

But second, as with configure the state may change between (1) and (4), again desynchronizing the state and the edge being detected.

 

The following approach solves both problems. First, we need to be absolutely sure that the edge we're looking for will detect a change from the state we think we're in. Use the following function:

 

static __inline__ void
configure_state_detection () {
  do {
    state = PxIN & BITn; // 1 : record state
    if (state) {
      PxIES |= BITn; // 2a : High, detect falling edge
    } else {
      PxIES &= ~BITn; // 2b : Low, detect rising edge
    }
    PxIFG &= ~BITn; // 3 : Reset interrupts from past changes
  } while ((PxIN & BITn) != state); // 4 : loop if state changed
}
This is much like the original naive configuration, but skips the PxIE setting and makes sure that the state after configuring the edge matches the state for which the edge was configured. So we know we're looking for the right edge, even if it changed while we were setting things up.

 

Then use that in these contexts:

 

Configure:

  __disable_interrupt();
  configure_state_detection();  // 1 : safe set state and edge detection
  PxIE |= BITn;  // 2 : Enable interrupts on state change, may fire right away
  __enable_interrupt();
Nothing special for configure. Monitor changes a lot:

 

Monitor:

  int in_state = state;
  configure_state_detection();  // 1 : safe set state and edge detection
  if (in_state != state) {
    if (state) {
      // 2a : emit rising edge event 
    } else {
      // 2b : emit falling edge event
    }
  }
This makes sure that state has actually changed within the resolution of our ability to detect it. (As with the rejected quibble above, we'll assume that any even number of changes between the original edge and the execution of the handler are ignorable within the bounds of "transient". If they had to be reported, interrupts were disabled too long and there is no viable solution.)

 

Eagle-eyed folks will notice that there is a race condition in configure_state_detection: If the state changes a positive even number of times between (3) and (4) we're going to get a spurious interrupt as soon as Configure re-enables interrupts. However, this doesn't matter because the Monitor implementation will detect that no effective change occurred and will not generate an event.

 

Because this safely resynchronizes the state and the PxIES configurations each time the state is inspected, and we only generate events when the state actually changed, it's also safe against the subtlety that setting PxIES can produce spurious interrupts.

 

Addendum 2014-09-29T11:50: I should make clear that the state variable must be marked volatile as it's a global that is mutated within an ISR and read outside the ISR. Technically some hyper-optimizing compiler might otherwise allow the application to process an event while looking at an out-of-date value of state.

Edited by pabigot
Link to post
Share on other sites

Nice!  This one's getting bookmarked.  That's some thorough checking.

Four years ago I had a wireless network that would lock up nodes randomly, usually after a day or two (or three) running. That was isolated to a similar race condition due to a change in state that occurred between machine-level instructions. That experience is part of why I'm so anal about checking things, and why I don't like coding to special cases that depend on assumptions that might change during evolution of the product.

 

At any rate, after folks have had a chance to review this and point out any flaws I've missed, I'll put a final version of it in Code Vault.

 

(Robustly detecting edge transitions in the timer module has some similar issues; somewhere on E2E there's a thread where I posted that, and the solution is also present in BSP430's pulse capture infrastructure, with relevant code in timer.c. I haven't reviewed that code recently to make sure I still think it's correct. In it you can see some of the effect of using PxIV which clears IFG before the handler code gets a chance to inspect the pre-existing state. Sometimes it'll turn out that's not such a good idea.)

Link to post
Share on other sites

Good work on the robust edge detection. It now kind of looks like a state machine method where you save the last state and compare the present and last states before transitioning.

 

 

Addendum 2014-09-29T11:50: I should make clear that the state variable must be marked volatile as it's a global that is mutated within an ISR and read outside the ISR. Technically some hyper-optimizing compiler might otherwise allow the application to process an event while looking at an out-of-date value of state.

 

 

This is a golden nugget of information for me right here.  Thank you for the clear and concise use case for the volatile declaration.

 

I came across this situation while working on a client project. Once I declared my variables as volatile then all my problems cleared up.

Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...