Location of pinMode matters?

Projects Programming
1

(I apologize if this is the wrong forum for asking this, but I wasn't sure exactly where this goes. It's sort of a "this works, someone please explain why." It's also an outgrowth of this post: http://arduino.cc/forum/index.php/topic,77892.0.html)

Having successfully created an array of LEDs, my next step was to abstract this into a couple of classes: LedArray, which is used to define an array of LEDs and turn them off and on; and BinaryLedArray, which inherits from LedArray and lets me feed in a number and have the array display it in binary. I'm doing this not because I think it's necessarily useful to do so, but because I want to fool around a bit with classes and inheritance, and this seemed like a good way to start.

Now, when making my LedArray class, I was getting very inconsistent results from the LEDs. The circuit was good, the code checked out, I know the board can power all ten at once because I've seen it do so, but some just wouldn't light. The most recent of the many things I tried to correct this was to move the pinMode call, which sets the pins in the array to OUTPUT, from the class constructor to the method that actually lights the pin. Basically, instead of setting them all at object instantiation, it only sets the pins as it needs them to light the LEDs. And suddenly everything works perfectly.

Now, it's nice that I got it working and all, but I don't understand why that worked. Anyone able to explain to this poor, addle-brained neophyte? I've attached the .cpp file for reference.

/*
  LedArray.cpp - Arduino library to control an array of LEDs
  Created by Michael C. Smith, Nov. 4, 2011
  Released into the public domain.
*/

#include "WProgram.h"
#include "LedArray.h"

LedArray::LedArray( int startingPin, int numberOfPins ) {
	for( int i = 0; i < 20; i++ )
	{
		if( i < startingPin || i >= startingPin + numberOfPins )
		{
			_pins[ i ] = -1;
		} else {
			//pinMode( _pins[ i ], OUTPUT ); <---this is where it was originally
			_pins[ i ] = i - startingPin;
		}
	}
}

int LedArray::getPinFromIndex( int index )
{
	for( int i = 0; i < 20; i++ ) {
		if( _pins[ i ] == index )
		{
			return i;
		}
	}
	return -1;
}

void LedArray::setLight( int index, bool isHigh )
{
	int pin = getPinFromIndex( index );

	if( pin > -1 && pin < 14 ) {
		pinMode( pin, isHigh );
		digitalWrite( pin, isHigh );
	} else if( pin > 13 && pin < 20 ) {
	    //eventually add code to handle analog pins with an else statement
	}
}

void LedArray::light( int index )
{
	setLight( index, true );
}

void LedArray::extinguish( int index )
{
	setLight( index, false );
}
2

And, if I'd bothered to check my email first, I would've seen that friend just pointed out the answer to me: I should've been using pinMode( i, OUTPUT ) in the constructor instead of pinMode( _pins[ i ], OUTPUT ).

Never mind.

3

And, if I'd bothered to check my email first, I would've seen that friend just pointed out the answer to me: I should've been using pinMode( i, OUTPUT ) in the constructor instead of pinMode( _pins[ i ], OUTPUT ).

This is still not the best approach. When is your constructor called relative to when the pins are ready to have their mode set?

You don't know, do you?

Relying on the pins being ready to have their mode set is not a good thing, in a constructor.

The solution is to have a begin() method that sets the mode of the pins. The constructor can define/set the pins to be used, but the pinMode() function should not be called in the constructor. Rather it should be called in the begin() method.

The reason for this is that you can't call the begin() method until the Arduino is ready to set the mode of the pins, so that in the begin() method, you are guaranteed to be able to set the mode, and know that it will work, regardless of timing, order of constructor calls, or which Arduino the code is running on.

4

You're correct, that's not an issue I was aware of. Thanks for bringing it up.

In the interest of not inserting an extraneous method, how about this: in the setLight method, I check if the pin is set; if not, I set it accordingly. That way it still can't be called before the pins are ready because it'd have to wait until someone called light() or extinguish(), but it's also not setting the pins constantly (which I'm assuming would involve a bit of overhead). It'd look something like this:

void LedArray::setLight( int index, bool isHigh )
{
	int pin = getPinFromIndex( index );

	if( pin > -1 && pin < 14 ) {
                if( !isOutput( pin ) { // Err... how does one determine this, actually? It seems like there should be an easy way.
                        pinMode( pin, OUTPUT );
                }
		digitalWrite( pin, isHigh );
	}
}

Or does that introduce other issues I haven't thought of yet?

5

Anyone up for a code review?

After a fair amount of further tinkering, I'm pretty happy with how these classes ended up. I implemented the begin() method as Paul suggested, after deciding that my counter-proposal would add unnecessary processing; I finally took a stab at figuring out bitmath and used that for the binary stuff; I took some advice on cleaning up the pin array a bit.

I'm attaching what's come out of all this. I've packaged it up as a library ("Displays") with some examples. If anyone of you have some spare time, I'd appreciate it if you could look through it and tell me what I've done wrong, or could at least do better. I'm not really planning on this library being used by anyone other than me, but I learn best by getting beat up by smarter people.

One caveat: The code assumes that your Arduino has 20 pins. I'm aware that this isn't true for the Mega, but I don't have one to test on. Anyone who does, feel free to suggest ways to make it Mega-compatible. (Or compatible with other variants I'm not aware of.)

Displays.zip (5.07 KB)

6

Your LedArray code assumes that pins 0 and 1 can be used for LEDs. It also assumes that they are available for Serial.print()ing. One of these assumptions is clearly invalid.

You could make setLight(), in the LedArray class, protected, instead of private. That way, derived classes could use it like a private function. This would make the BinaryLedArray::displayNumber() function a lot simpler.

7

Whoops! That Serial call was left in by accident from when I was testing. In general, I think it's worthwhile keeping pins 0 and 1 available for use in applications where serial communication isn't required, though.

I made setLight protected as you suggested, and now displayNumber looks like this:

void BinaryLedArray::displayNumber( int value )
{
    for( byte i = 0; i < _arrayLength; i++ ) // iterate through bits of number from right to left, stopping after all digits in array accounted for
    {
        setLight( i, 1 & bitRead( value, i ) );  // set each LED according to its bit value
    }
}

Which is definitely much cleaner, thanks.