Buttons with interrupts not behaving as expected

The following sketch uses 2 buttons, one button is wired to pin 4 and the other to pin 5. Both buttons are wired to ground on the Arduino. The ISR is supposed to add 1 to volatile int counter when I press button connected to pin 4 and subtract 1 when I press button connected to pin 5. However, it does exactly the opposite: subtracts 1 to volatile int counter when I press button connected to pin 4 and adds 1 when I press button connected to pin 5. I checked my connections and they are as per above. Any clues why this may happen?? Thanks for any hints. AA.
PD: there is bouncing and double interrupt trigger from press-release happening but for now I'm just trying to understand the reason(s) for the opposite behavior of the Interrupt Service Routine.

volatile int counter = 0;
void setup() {
  Serial.begin(9600);
  pinMode(4, INPUT_PULLUP);
  pinMode(5, INPUT_PULLUP);
  PCICR = B00000100; // Enable PIN CHANGE interrupts on D port
  PCMSK2 = B00110000; // Allow trigger PIN CHANGE interrupts on pins D4 and D5
}
void loop() {
  cli(); // We pause the interrupts
  Serial.print("The counter is now at: ");
  Serial.println(counter);
  sei(); // We reactivate the interrupts
  delay(100);
}
ISR (PCINT2_vect) {
  if (PIND & B00010000) { 
      counter++; // Subtracts when I press button connected to pin 4!!!!!!!!!!!!
  }
  if (PIND & B00100000) {
      counter--; // Adds when I press button connected to pin 5!!!!!!!!!!!!
  }
}

Is there something that requires you to use interrupts? Otherwise you would be better off polling the buttons. Is this an Arduino UNO?

Also, keep in mind you are getting interrupts whenever the pin changes, both HIGH to LOW and LOW to HIGH. That is in addition to the pin bounce. It is going to be hard to predict how the counter will change.

Yes it's an UNO. I need interrupts for an RC project. At the moment, I'm just trying to understand

OK. But you should rarely ever need to use interrupts directly since for most hardware there are libraries that handle them for you. Interrupts can be very tricky and should be reserved for very time critical operations and should be as short as possible.

Back to your problem specifically, you need to filter the pin change direction (relatively easy and I would increment on LOW->HIGH) and handle debounce (tricky using interrupts).

Is there some reason you are not using the Arduino API to setup and handle the interrupts? You can specify which pin direction you want to interrupt on then all you have to do is debounce.

As a simple step to fix one potential problem, use a byte here instead on an int:

volatile int counter = 0;

Just try it.

a7

It's about an order of magnitude faster (close to 20 times) to manipulate registers than the interrupt API

Unless it really matters, it would be better to make the code more readable using the API.

volatile byte counter = B00000000 instead of volatile int counter = 0works a LOT better in terms of not bouncing all over the place when I press the buttons and adding/subtracting 1, but the ISR behavior remains the opposite as before. Strange.

Strange that the only thing I could see going wrong would not have, as you did carefully turn off interrupts whilst accessing the counter variable.

I see the same behaviour, staying tuned to see what we are not. Yet.

I used bounceless switches, so it isn't any of that.

a7

It is because the input pullup pulls the pin high, so when you press the one button it goes low and triggers the interrupt. Then when the interrupt triggers, the pressed pin is low so it doesn’t add, while the unpressed pin is still high so it subtracts. Then on release the first pin goes high so it adds, and the other pin is still high so it counteracts.

And vice-versa for the other button.

The logic is very awkward—on release both pins are high, and the increment offsets the decrement. On press, only the unpressed button’s logic triggers.

bild
bild

I rest my case

I misread your text? Perhaps.. but hey, try this

  if (PIND & PIND4) { 
      counter++; // Subtracts when I press button connected to pin 4!!!!!!!!!!!!
  }
  if (PIND & PIND5) {
      counter--; // Adds when I press button connected to pin 5!!!!!!!!!!!!
  }

Now they both subtract

Sorry for asking basic questions: Picked the right Arduino? Not any wires mixed up? If you run a 'normal' sketch where you poll the buttons, will they be correct?

Here is a solution:

ISR (PCINT2_vect) {
  if ((PIND & B00010000) == 0) { 
      counter++;
  }
  else if ((PIND & B00100000) == 0) {
      counter--; 
  }
}

Tried a second UNO and same behavior. I think DaveX Faraday has explained what's happening but I'm still trying to get my head around it.

+ you have bouncing

One thing you could try, (but still, no debounce) is this library

PinChangeInterrupt

Rising, Falling or Change detection for every pin separately

I get it what @DaveX means. Button trigger interrupt, but since that button is low, the opposite if is exectued since it's true, and vice versa.

The buttons are correct, but the if's are fed the wrong logic.