Binary Logic - Brain Game?

Hi guys wanting to check my "logic" with an ISR pin change interrupt where i record the status of PIND before the ISR and then during the ISR followed by a bit of binary logic gives me a binary result of which pin went high or 0 if it went low, now i am assuming that only a single pin can change when the interrupt is called i.e the pin that triggered the interrupt in which case all is good.

My project uses 5 infrared sensors on pins 3-7 i am using this particular part to turn off interrupts on the other 4 sensors so that i can reliably receive the rest of the data and now to business.

// The port status of PIND called before the ISR (Initially set during setup) is as so: 

Pins: 7, 6, 5, 4, 3, 2, 1, 0
        0, 0, 0, 0, 0, 0, 1, 1

//Now when the interrupt is called it will look like this: 

Pins: 7, 6, 5, 4, 3, 2, 1, 0
        0, 0, 0, 0, 1, 0, 1, 1

//If we name them as follows we get this:

Pins: 7, 6, 5, 4, 3, 2, 1, 0
        0, 0, 0, 0, 0, 0, 1, 1
        0, 0, 0, 0, 1, 0, 1, 1

/*A simple way to detect which pin changed would be to use an XOR function however i'm not interested *in when one changed only in when a pin goes high, so i use an OR gate to detect all pins that are high *then an XOR to detect which pins changed from low to high between our original register and the ones *that are high now as so.
*/


Pins:      7, 6, 5, 4, 3, 2, 1, 0
Past:      0, 0, 0, 0, 0, 0, 1, 1
Current: 0, 0, 0, 0, 1, 0, 1, 1
OR:        0, 0, 0, 0, 1, 0, 1, 1
XOR:      0, 0, 0, 0, 1, 0, 0, 0

/*The above returns a binary value that can be compared using the if(Value & 0b00001000) statement *to tell you that pin 3 went high.
*/

I believe the logic is sound providing only a single pin changes per ISR the code i have wrote is as follows.

ISR (PCINT2_vect){
  pdPast = pdState; //Remember our previous register.
  pdState = PIND; //Get register state now.
  pdOut = (pdPast | pdState) ^ pdPast; //Bitmath to get on bits then xor to find toggled bits.
  currMicro = micros(); //Get current time.
  iFlag = 1; //ISR indicator.
}

To detect which pin went high i use the above function in loop.

uint8_t getSensor(uint8_t pRead){
  if(pRead & 0b00001000){ //Pin 3.
    return 1; //Sensor 1.
  }else if(pRead & 0b00010000){ //Pin 4.
    return 2; //Sensor 2.
  }else if(pRead & 0b00100000){ //Pin 5.
    return 3; //Sensor 3.
  }else if(pRead & 0b01000000){ //Pin 6.
    return 4; //Sensor 4.
  }else if(pRead & 0b10000000){ //Pin 7.
    return 5; //Sensor 5.
  }else{
    return 0; //No inputs from sensor or no pulse being recieved.
  }
}

Now i simply use an if function to only process the ISR results when a value greater than 0 is recorded does anyone see any problems with my code or logic above i believe this is correct however my serial monitor seems to output a few odd value.

Reciever Started...
Past: 11 - State: 11 - Output: 0 - Sensor: 0
Past: 10011 - State: 11001 - Output: 0 - Sensor: 2
Past: 1 - State: 11 - Output: 10000 - Sensor: 2
Past: 10001 - State: 10001 - Output: 10000 - Sensor: 0
Past: 10001 - State: 11001 - Output: 10000 - Sensor: 1

Any variables that you share between an ISR and the main program flow need to be declared with the volatile keyword so that the compiler knows how to handle them correctly. If you haven't done this, then do it and re-test. Otherwise you need to post your complete code with a corresponding example output. Snippets are generally unhelpful.

Below is my complete code, what i am trying to achieve is to detect which pin went high and then disable interrupts for all other pins however as you can see from the serial monitor above and re-posted below i have it returning outputs from sensor 1, 2 and then 0 for the low states.

I'm confused as to some of these errors as the 2nd output should be returning sensor 1 from what i can make of it and there should be something on the output.

#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit)) //Clear bit in byte at sfr address.
#define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit)) //Set bit in byte at sfr address.

volatile long unsigned prevMicro, currMicro, Length; //Time before, current and time passed.
volatile uint8_t pdState, pdPast, pdOut; //Register state current, past and present.
volatile bool iFlag; //Interrupt flag.

void setup() {
  Serial.begin(115200);
  DDRD = DDRD | B11100100; //Setup 3-4 as inputs.
  PORTD = PORTD | B00011000; //Pull 3-4 high.
  Serial.println("Reciever Started..."); //Ready notification.
  pdState = PIND; //Store the current register status.
  EnableISR(); //Enable the ISR.
}

void EnableISR(){
  sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
  sbi (PCMSK2, PCINT23); //Pin 7.
  sbi (PCMSK2, PCINT22); //Pin 6.
  sbi (PCMSK2, PCINT21); //Pin 5.
  sbi (PCMSK2, PCINT20); //Pin 4.
  sbi (PCMSK2, PCINT19); //Pin 3.
}

void DisableISR(int sNum){
  cbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
  cbi (PCMSK2, PCINT23); //Pin 7.
  cbi (PCMSK2, PCINT22); //Pin 6.
  cbi (PCMSK2, PCINT21); //Pin 5.
  cbi (PCMSK2, PCINT20); //Pin 4.
  cbi (PCMSK2, PCINT19); //Pin 3.
  switch (sNum){
    case 1: //Disable all but pin 3 (Sensor 1).
      sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
      sbi (PCMSK2, PCINT19); //Pin 3.
      break;
    case 2: //Disable all but pin 4 (Sensor 2).
      sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
      sbi (PCMSK2, PCINT20); //Pin 4.
      break;
    case 3: //Disable all but pin 5 (Sensor 3).
      sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
      sbi (PCMSK2, PCINT21); //Pin 5.
      break;
    case 4: //Disable all but pin 6 (Sensor 4).
      sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
      sbi (PCMSK2, PCINT22); //Pin 6.
      break;
    case 5: //Disable all but pin 7 (Sensor 5).
      sbi (PCICR, PCIE2); //enable interrupt PCI2 on pins 3-7 (19-23).
      sbi (PCMSK2, PCINT23); //Pin 7.
      break;
  }
}

void loop() {
  if(iFlag){
    int sS = getSensor(pdOut);
    Serial.print("Past: ");
    Serial.print(pdPast, BIN);
    Serial.print(" - State: ");
    Serial.print(pdState, BIN);
    Serial.print(" - Output: ");
    Serial.print(pdOut, BIN);
    Serial.print(" - Sensor: ");
    Serial.println(sS);
    if (sS){
    DisableISR(sS);
    }
    iFlag = 0;
  }
}

uint8_t getSensor(uint8_t pRead){
  if(pRead & 0b00001000){ //Pin 3.
    return 1; //Sensor 1.
  }else if(pRead & 0b00010000){ //Pin 4.
    return 2; //Sensor 2.
  }else if(pRead & 0b00100000){ //Pin 5.
    return 3; //Sensor 3.
  }else if(pRead & 0b01000000){ //Pin 6.
    return 4; //Sensor 4.
  }else if(pRead & 0b10000000){ //Pin 7.
    return 5; //Sensor 5.
  }else{
    return 0; //No inputs from sensor or no pulse being recieved.
  }
}

ISR (PCINT2_vect){
  pdPast = pdState; //Remember our previous register.
  pdState = PIND; //Get register state now.
  pdOut = (pdPast | pdState) ^ pdPast; //Bitmath to get on bits then xor to find toggled bits.
  currMicro = micros(); //Get current time.
  iFlag = 1; //ISR indicator.
}

Re-posted serial monitor below.

Reciever Started...
Past: 11 - State: 11 - Output: 0 - Sensor: 0
Past: 10011 - State: 11001 - Output: 0 - Sensor: 2
Past: 1 - State: 11 - Output: 10000 - Sensor: 2
Past: 10001 - State: 10001 - Output: 10000 - Sensor: 0
Past: 10001 - State: 11001 - Output: 10000 - Sensor: 1

You could be getting multiple interrupts between between the time your loop() tests iFlag and you print out all the variables which means they can be changing

Shandy:
i am assuming that only a single pin can change when the interrupt is called i.e the pin that triggered the interrupt in which case all is good.

I don't think that is a correct assumption.

Shandy:

  DDRD = DDRD | B11100100; //Setup 3-4 as inputs.

PORTD = PORTD | B00011000; //Pull 3-4 high.

This don't look correct and what about the other 3 pins?

Shandy:
Below is my complete code, what i am trying to achieve is to detect which pin went high and then disable interrupts for all other pins

You can disable the interrupts, but that will not affect the state of the pins as you read them from PIND. So a interrupt disabled pin may change, no interrupt is generated, a interrupt pin is changed, an interrupt is generated, and now you have two pins set in PIND. That will likely screw up:

    if (sS){
    DisableISR(sS);
    }

if I have understood your intention.

    int sS = getSensor(pdOut);
    Serial.print("Past: ");
    Serial.print(pdPast, BIN);
    Serial.print(" - State: ");
    Serial.print(pdState, BIN);
    Serial.print(" - Output: ");
    Serial.print(pdOut, BIN);
    Serial.print(" - Sensor: ");
    Serial.println(sS);
    if (sS){
    DisableISR(sS);
    }
    iFlag = 0;

There is nothing to stop interrupts occurring anywhere in this block of code, and thus changing the values that you are printing out.
You need to check if iFlag (maybe better declared as a byte to make 100% sure reads and writes are atomic) is set, globally block interrupts, copy the vars to a new set of shadow vars (local vars if you like), clear iFlag, globally enable interrupts, then print out the shadow vars.

Where are you trying to go with this anyway? There may be a better way.

I understand the point made about multiple interrupts occurring in between the loop checking iFlag but that would simply mean that we wont end up reading it as our register past and current state would be updated, i have changed iFlag to a byte as you said.

I must say pin change interrupts are a creature of their own if you are not using a library (Scurries off to download for further study), to explain what i want to accomplish i have 5 infrared sensors (I forgot i turned the other pins off in setup to reduce any external influences while testing) that i read from using the ISR and micros function as so far all works well until i introduce more than a single sensor then i get distorted time measurements this is to be expected as multiple ISR's are triggered because more than a single sensor picked up the pulse so to remove this issue i want to be able to detect the header pulse and which pin it occurred on then i can disable all other sensors until i have received the signal from the sensor i am listening to.

Hope this builds a picture of what i am trying to accomplish, another thing im not so certain is working is the DisableISR function as i still pickup data from multiple sensors as you see in the serial monitor i would say it was just picking up the state change on PIND but the status of PIND is only update while in the ISR.

I think you are correct about the ISR interrupting the serial.print functions.

I guess it depends on what else is going to be added to the sketch and how long a duration one signal transmission lasts, but I would say you might be better to either:

  • Forget about interrupts and just poll the pins regularly. When a signal arrives, just poll the one pin for state changes.
  • Use the interrupts to determine the initial incoming signal, then disable them all (PCICR), before polling one pin for state changes.

While polling the one pin, your sketch will not be able to do anything else; so it does assume that the signal is only brief.

What you could do for the second suggestion, is use nested interrupts. ie.:

  • the ISR is called for the initial incoming signal: the global interrupt flag is automatically cleared to disable interrupts,
  • you disable the pin change interrupts that can call the ISR (PCICR again),
  • clear (see datasheet) the PCIF register flag for the ISR to remove any pending interrupt that snuck in before interrupts were disabled,
  • manually set the global interrupt flag to reenable global interrupts so that other business can go on as usual,
  • continue inside the ISR to determine which pin to monitor and busy-wait listen for the remaining signal.