Binary counter, button problem

[blh64]

Since I do not have your shield or that library, it is not surprising it doesn't work. Have you tried putting some Serial.print() statements into each of the if/then/else statements so you can see if the logic is correct? Only you know what "ON" and "OFF" are defined to mean and how that relates to digitalRead() returning HIGH/LOW.

Also, if your professor is using addition with millis(), you should get a new professor. It is a matter of correctness, not simply more or less logical to your way of thinking. The elapsed amount of time returned by millis() rolls over and back to 0 after a bit more than 49 days. (32 bit integer). This probably does not matter in your exact case, but if it does, you get a problem around this rollover time (current time will be less than past time) but with unsigned integer subtraction, it works out without any issues.

[migraun]

You caught me just in time, as actually, I made it work in the end...but its really late so I will sleep before making it less chaotic and posting it here.

Also yeaah, we were warned about the rollover time, and I don't think I can get a new professor. A bit too late in the semester for that, and so far he seems actually pretty alright?
We will see come the finals though, hah.
Thank you for the help though, the substraction is what I used in the working version, and I wouldn't have come up with it without you.

[dougp]

Visual aid to what @blh64 posted at #21

[migraun]

Alright, so I figured it out in the end, thank you to everyone who helped!
Here is the final code:

#include "funshield.h"
//setting up
constexpr int ledPins[] { led1_pin, led2_pin, led3_pin, led4_pin };
constexpr int ledPinsCount = sizeof(ledPins) / sizeof(ledPins[0]);
constexpr int Button1 = button1_pin;
constexpr int Button2 = button2_pin;
constexpr int activationDelay = 1000;
constexpr int periodicDelay = 300;

unsigned long lastTime = 0;
unsigned long lastTime2 = 0;
bool firsttime = true;
bool firsttime2 = true;
int counter = 0;
bool wasDown = false;
bool wasDown2 = false;
int isDown = 0;
int isDown2 = 0;

//Binary counter
void BN(int number, const int leds[], int ledsCount)
{
  for (int i = ledsCount - 1; i >= 0; --i) {
    bool currentBit = number & 1;
    digitalWrite(leds[i], currentBit ? ON : OFF);
    number = number >> 1;
  }
}
//Smart function
void TheThing(int Button) {
  if (Button == Button1) {
    counter = (counter + 1) % 16;
    BN(counter, ledPins, ledPinsCount);
  }
  if (Button == button2) {
    counter = (counter - 1) % 16;
    BN(counter, ledPins, ledPinsCount);
  }
}

void setup() {
  for (int i = 0; i < ledPinsCount; ++i) {
    pinMode(ledPins[i], OUTPUT);
    digitalWrite(ledPins[i], OFF);
  }
  pinMode(button1_pin, INPUT);
  pinMode(button2_pin, INPUT);
}

void loop()

{
  isDown = digitalRead(button1_pin);
  isDown2 = digitalRead(button2_pin);
  unsigned long times = millis();
  //Button1 pressed and first loop it is being held
  if (isDown == ON && firsttime == true) {
    if (times - lastTime >= activationDelay) {
      TheThing(Button1);
      firsttime = false;
      lastTime = times;
    }
  }
  //Button1 pressed and not first loop it is being held
  if (isDown == ON && firsttime == false) {
    if (times - lastTime >= periodicDelay) {
      TheThing(Button1);
      lastTime = times;
    }
  }
  //Button1 stopped being pressed
  if (isDown == OFF) {
    firsttime = true;
    lastTime = times;
  }
  //Button2 pressed and first loop it is being held
  if (isDown2 == ON && firsttime2 == true) {
    if (times - lastTime2 >= activationDelay) {
      TheThing(Button2);
      firsttime2 = false;
      lastTime2 = times;
    }
  }
  //Button2 pressed and not first loop it is being held
  if (isDown2 == ON && firsttime2 == false) {
    if (times - lastTime2 >= periodicDelay) {
      TheThing(Button2);
      lastTime2 = times;
    }
  }
  //Button2 stopped being pressed
  if (isDown2 == OFF ) {
    firsttime2 = true;
    lastTime2 = times;
  }
  //Button1 changed states
  if (wasDown != isDown) {
    //Button1 currently down
    if (isDown == ON) {
      TheThing(Button1);
    }
    wasDown = isDown;
  }
  //Button2 changed states
  if (wasDown2 != isDown2) {
    //Button2 currently down
    if (isDown2 == ON) {
      TheThing(Button2);
    }
    wasDown2 = isDown2;
  }
}

If anyone has any tips or ideas, feel free to share!
For now I will enjoy the fact that I managed it.