Can't get something to work right

#include "SevSeg.h"
int buttonup = 2;	                // pin to connect the UP button
int buttondn = 3;			// pin to connect the DOWN button
int presses = 0;		        // variable to store number of presses
long time = 0;			        // used for debounce
long debounce = 100;	                // how many ms to "debounce"
const byte numPins = 4;                 // how many LED, 4 here
int state;				// used for HIGH or LOW
   // pins to connect leds
byte pins[] = {5, 6, 7, 8};   
int leddisplay = 1;                     // start LED display at 1
SevSeg sevseg; //Instantiate a seven segment controller object


void setup()
{
	/* we setup all pins as OUTPUT and start at LOW */
	for(int i = 0; i < numPins; i++) {
		pinMode(pins[i], OUTPUT);
                digitalWrite (pins[i], LOW);
	}
	pinMode(buttonup, INPUT);
	pinMode(buttondn, INPUT);
	/* use pin 2 and 3 which has interrupt 0 and 1 on Arduino UNO */
	attachInterrupt(0, countup, LOW);
	attachInterrupt(1, countdn, LOW);

      // setting up LED display
         byte numDigits = 2; // 2 digits display
         byte digitPins[] = {A5, A4}; 
         byte segmentPins[] = {9, 10, 11, 12, 13, A0, A1, A2}; // pin a, b, c, d, e, f, g, and DP
         sevseg.begin(COMMON_ANODE, numDigits, digitPins, segmentPins); // change to cathode if used
         sevseg.setBrightness(90); // change to be brighter or dimmer
}
 
void loop()
{
	/* convert presses to binary and store it as a string */
	String binNumber = String(presses, BIN);
	/* get the length of the string */
	int binLength = binNumber.length();	
        
	if(0 <= presses <= 15) {	
		for(int i = 0, x = 1; i < binLength; i++, x+=2) { 
			if(binNumber[i] == '0') state = LOW;
			if(binNumber[i] == '1') state = HIGH;
			digitalWrite(pins[i] + binLength - x, state);
		}	
	} else {
                if (presses > 15) presses = 15; 
                if (presses < 0) presses = 0; // prevent rollover
	}

    // display current page
    leddisplay = presses + 1 ;
    sevseg.setNumber(leddisplay, 0);
    sevseg.refreshDisplay(); // Must run repeatedly
}
 
/* function to count the presses */
void countup() { 
	// we debounce the button and increase the presses
	if(millis() - time > debounce)	presses++;
	time = millis();
}

void countdn() { 
	// we debounce the button and decrease  the presses
	if(millis() - time > debounce)	presses--;
	time = millis();
}

It is not "counting" right when I press the button and the display or LED output is not changing or is suddenly jumping around like there's noise somewhere. Is the code OK or is there something that may be causing it to not register buttons?

What I wanted to do is show the 4 LED binary from 0000 to 1111 and have the display show from 1 to 16 (in decimal)

	if(0 <= presses <= 15) {

Inventing shortcuts rarely works. That is NOT how to test that a value is in a range.

	String binNumber = String(presses, BIN);

If presses is 7, binNumber will be "111".

	int binLength = binNumber.length();

and binLength will be 3.

			digitalWrite(pins[i] + binLength - x, state);

So, you will be writing to the pins pins[0]+3-1, pins[1]+3-3, and pins[2]+3-5, or pins 7, 6, and 5. Is that what you expect?

Using interrupts to read the state of switches pressed by humans does not make sense.

Delta_G:
Do you have pull up or pulldown resistors on your buttons? If not, Google floating input.

You probably also don't want to use LOW for the interrupt signal as that will continuously fire the interrupt over and over really fast as long as the button pin is low. You probably want FALLING there if you are wanting to react to presses.

There is pull up resistor on both buttons. The pins are held high when not pressed, and short to ground when pressed. I am using 10k resistor. I'll hook my O-scope to the button to see if it looks normal or if there's something, bad button could screw this up.

I'll try the falling instead of low for triggering interrupt. Seems like it'd be set to count one per press, not count while holding button down.

PaulS:

	if(0 <= presses <= 15) {

Inventing shortcuts rarely works. That is NOT how to test that a value is in a range.

Can you suggest alternative please? (0 <= presses && presses <= 15) or something like it?

	String binNumber = String(presses, BIN);

If presses is 7, binNumber will be "111".

	int binLength = binNumber.length();

and binLength will be 3.

			digitalWrite(pins[i] + binLength - x, state);

So, you will be writing to the pins pins[0]+3-1, pins[1]+3-3, and pins[2]+3-5, or pins 7, 6, and 5. Is that what you expect?

Using interrupts to read the state of switches pressed by humans does not make sense.

Without the interrupt code, it would need to check the button presses, determine if it changed and wasn't just held down for a long time, and jump to counting loop, and that seems like more extra code plus extra variable containers to keep track than just dumping the work on interrupt handler.

I am using this page for the source of binary counter. It seems to work: http://www.electroschematics.com/9809/arduino-8-bit-binary-led/ My change is 4 bits instead of 8 and using 2 buttons for up and down. Then adding in 7-segment library to display the number in decimal (with +1 so it starts at 0 and ends at 16)

I don't fully understand how the code worked but it is much smaller than my original binary counter and I'd like to make the final code under 8k

With my limited knowledge, I can at least help with this little item:

if(0 <= presses <=15) {

should be:
if ((presses >= 0) && (presses <= 15)) {

--Michael

Without the interrupt code, it would need to check the button presses, determine if it changed and wasn't just held down for a long time, and jump to counting loop

Yeah.

and that seems like more extra code plus extra variable containers to keep track

So? You write the code once.

than just dumping the work on interrupt handler.

Using a sledge hammer to kill flies is more effective, too. Harder on the furniture, arms, etc. But, hey, its easier than doing it right.

should be:
if ((presses >= 0) && (presses <= 15)) {

You don't need so many brackets, comparison operators bind tighter than logical operators.

Yes, my thinking too about the parentheses.

cheers,
Michael

Ended up replacing the binary output with something a little less convoluted (and wasted a few bytes of storage)

			digitalWrite(pins[0], (presses &        B1));
			digitalWrite(pins[1], (presses &       B10));
			digitalWrite(pins[2], (presses &      B100));
			digitalWrite(pins[3], (presses &     B1000));

And replaced the push button. Now it works exactly how I wanted.

Unless I'm having a bit of brain flatulence (I'm sure somebody will point it out if so), I think you can use a for loop to do that:

for (byte n = 0; n <= 3; n++) {
digitalWrite(pins[n], (presses & (B1 << n)))
}

--Michael
edited to add a parenthesis
PS. Annoying that I can't edit my own post without waiting 5 minutes

Edited again on 3rd thought. Fixing really unnecessary brackets this time (& necessary semicolon):
digitalWrite(pins[n], presses & (B1 << n));