Sketch Optimization and Feasibility On An ATtiny

Hello! I just got my Arduino Uno a few weeks ago and have been learning the ropes since I got it. I have no prior programming knowledge and the most small electronics work I've done is wiring up a few LEDs, but I'm learning and have had a lot of fun with it.

The point of this project is to implement a couple fancy features in a car that I'm restoring. The first thing I want to achieve is simply fading on the courtesy and dome lights (which are LEDs) when one of the doors is open, and off when they're both closed. The second thing I want to implement is a simple push button that will be located somewhere on the drivers side. When it's pressed, the next time the driver door is open, the window will automatically roll down; when the door is closed, the window will roll back up. The button will have to be pressed each time before it will do this.

I have written a completely functional sketch and prototyped it on a breadboard using a red/green LED to simulate the window. Being new to programing, I'm sure I've used the long winded, inefficient way to achieve much of what I did, and the eventual goal is, if possible, to use an ATtiny chip to control these functions in the car. So I would like the sketch to be as concise and efficient as possible, and I was hoping I could get some suggestions on what I can do to improve it and get some input on how an ATtiny (probably an 84 or 85) will handle this sketch.

/* This sketch has two purposes and is designed for use in a car.  The first 
 purpose of this sketch is to fade a set of LEDs on when the car door is opened,
 then fade them back off when the car door is closed.  The second purpose of
 this sketch is to make a push button that when pressed, the door window will
 roll down when the door is opened, then roll back up when the door is clossed.*/

// Define the pins the pushbutton and door switches are on
const int buttonPin[] = {2, 4, 6};
// Define the pins that will ultimately control the window rolling up or down
const int windowPin[] = {7, 8};
// Define the pin the LEDs to fade are on
const int ledPin = 3;
// Define the amount of time the fade delay will be
const int fadeDelay = 15;
// Define the brightness increase interval
const int fadeAmount = 3; 
// Define the delay for button debounce
const int debounceDelay = 15;
// Define the amount of time the windows will spend rolling up and down.
//rollUpDelay will be longer to ensure window rolls all the way up.
const int rollUpDelay = 3000; 
const int rollDownDelay = 2000; 

//Variables that will change

// Variable to track the current state of the push button, in an array the size
//of buttonPin.
boolean currentButtonVal[3];
// Initialize variables to define and store the debounced switch values. doorSwitch
//is in an array the size of buttonPin
boolean doorSwitch[3];
boolean windowButton;
// Define the inital brightness of the LED. Starts off, or 0
int brightness = 0;
// Define the inital "previous time" variable for comparison with millis().
//Will start at 0 and be redifned every time fadeDelay has passed. 
unsigned long previousTime = 0;
// Define the inital comparison time for the debounce function.  Has the same
//purpose as in "previousTime."
unsigned long lastDebounceTime = 0;
// Initialize a variable that is set to millis() every time a button state is 
//toggled. In an array the size of buttonPin[], and is unique to each button.
unsigned long toggleTime[3];
// Define the inital state for a button toggle. This value will be reversed
//when a button is pressed. In an array the size of buttonPin[], and is unique
//to each button.
boolean buttonToggle[3] = {false};
// Initialize a variable to track if the rollWindow function should be active
boolean rollWindowToggle;
// Initialize a variable to detect if the rollWindow button should finish
boolean continueFunction; 

void setup () {
  pinMode (buttonPin[0], INPUT);
  pinMode (buttonPin[1], INPUT);
  pinMode (buttonPin[2], INPUT);
  pinMode (windowPin[0], OUTPUT);
  pinMode (windowPin[1], OUTPUT);
  pinMode (ledPin, OUTPUT);
}


void loop () {
  // Create a variable to store the millis() value this time through the loop
  unsigned long currentTime = millis();
  
  // Define variables to debounce and keep track of the different switches
  doorSwitch[0] = debounce(0); 
  doorSwitch[1] = debounce(1);
  windowButton = debounce(2);

  // If either car door is open, then fade the LEDs.  If both car doors are
  //closed, then fade the LEDs off.
  if (doorSwitch[0] == HIGH || doorSwitch[1] == HIGH) {
    if (currentTime - previousTime > fadeDelay) {
      previousTime = currentTime;
      brightness = brightness + fadeAmount;
      // Limit the fade amount from going above 255
      brightness = min(brightness, 255);
      analogWrite (ledPin, brightness);
    }
  }
  else {
    if (currentTime - previousTime > fadeDelay) {
      previousTime = currentTime;
      brightness = brightness - fadeAmount;
      // Limit the fade amount from going bellow 0
      brightness = max(brightness, 0);
      analogWrite (ledPin, brightness);
    }
  }

  // If windowButton has been toggled to true (on) and the door switch is HIGH,
  //then make rollWindowToggle true. This allows the rollWindow function to 
  //begin. If buttonToggle is not true (on), then make rollWindowToggle false.
  if (buttonToggle[2] == true) { //Start function when buttonToggle is true
    if (doorSwitch[0] == HIGH) { //If the door is open
      rollWindowToggle = true; //Let the rollWindow function execute
    }
  }
  else rollWindowToggle = false;
  
  // Test to see if rollWindowToggle was toggled on while the door was open,
  //if it was, then reset toggleTime so the window will still roll the correct
  //amount of time. continueFunction must not equal true so toggleTime does not
  //continuously reset if rollWindowToggle gets toggled while the function is
  //in progress.
  if (rollWindowToggle == false && doorSwitch[0] == HIGH && continueFunction != true) {
    toggleTime[0] = millis();
  }
  
  // Iniate rollWindow function if either requirment is meant. continueFunction
  //tracks if the function should continue, even if rollWindowToggle is 
  //toggled to false.
  if (rollWindowToggle == true || continueFunction == true) {
    // Iniate rollWindow function, doorSwitch and toggleTime indicate which
    //switch will activate the function. currentTime passes the variable assigned
    //to millis().
    rollWindow(0, currentTime);
  }
}


void rollWindow (int thisSwitch, unsigned long currentTime) {
  // Switch case to control what happens when the door is open or closed
  switch (doorSwitch[thisSwitch]) {
    // If the door is open:
    case HIGH:
      // Define the state of continueFunction to indicate that the function is
      //in progress and should continue until it is complete
      continueFunction = true;
      
      // If the rollDownDelay time has passed, stop rolling the window. If not,
      //roll the window down. Make windowPin[1] LOW to ensure the window will
      //not try to roll down and up at the same time.
      if (currentTime - toggleTime[thisSwitch] > rollDownDelay) { //If roll up time has passed:
        digitalWrite (windowPin[0], LOW); //Stop rolling the window down
      }
      else {
        digitalWrite (windowPin[1], LOW); //Make sure the window is not rolling up
        digitalWrite (windowPin[0], HIGH); //Make the window roll down
      }
      break;
  
    // If the door is closed:
    case LOW:
    
      // If the rollUpDelay time has passed, stop rolling the window, reset
      //buttonToggle[2] to false, and make continueFunction false to indicate
      //the function is complete. If rollUpDelay time has not passed, then
      //roll the window up and make windowPin[0] LOW to ensure the window will
      //not try to roll up and down at the same time.
      if (currentTime - toggleTime[thisSwitch] > rollUpDelay) { 
        digitalWrite (windowPin[1], LOW); 
        buttonToggle[2] = false; 
        continueFunction = false;
      }
      else {
        digitalWrite (windowPin[0], LOW); 
        digitalWrite (windowPin[1], HIGH);
      }
      break;
  }
}

// Create a debounce function to debounce the switches, start the toggleTime
//timer, and toggle the buttonToggle variable. Return the value current.
boolean debounce(int thisButton) {
  // Take a reading of the switch pin and store it in a local variable.
  boolean current = digitalRead(buttonPin[thisButton]);
  // If the reading does not equal the current recorded state of the button,
  //wait the debounceDelay, then take another reading of the currenet value and
  //begin the toggle timer.  If the state of the switch is high, then toggle
  //buttonToggle.
  if (currentButtonVal[thisButton] != current) {
    if (millis() - lastDebounceTime > debounceDelay) {
      current = digitalRead(buttonPin[thisButton]); 
      toggleTime[thisButton] = millis(); 
      if (current == HIGH) {
        buttonToggle[thisButton] = !buttonToggle[thisButton];
      }
    }
  }
  // Record the curent button value and return current
  currentButtonVal[thisButton] = current;
  return current;
}

Thanks a bunch!
Acliad

  pinMode (buttonPin[0], INPUT);
  pinMode (buttonPin[1], INPUT);
  pinMode (buttonPin[2], INPUT);
  pinMode (windowPin[0], OUTPUT);
  pinMode (windowPin[1], OUTPUT);

Arrays and for loops were meant for each other...

The == true bits in your code are unnecessary. true == true will return true. false == true will return false.

The == false bits are equally unnecessary. Just use the ! operator.

  if (!rollWindowToggle && doorSwitch[0] == HIGH && !continueFunction) {

The only inefficient thing you are doing, that I can see, is using a switch statement when there are only two (mutually exclusive) cases. An if/else would result in smaller code.

Be careful with your use of boolean variables and checking their value. Whilst it is possible to create a boolean variable whose value is normally expressed as true or false and actually check whether its value is HIGH/LOW or 1/0 it is not so obvious what is going on when you read the code. There is also a possible problem when using boolean varaibles in that HIGH/LOW and 0/1 are either/or values whereas that is not the case for true/false. 0 is false and any other value is true.

Another thought. How are the switches wired ? Do they have any resistors attached to ensure a consistent voltage when not activated ? Whilst it is admirable that you have debounced the switches is it really necessary in this application ?

boolean buttonToggle[3] = {false};

this sets only the first element to false, the others are undefined....

Thanks everyone for your replies!

UKHeliBob, I didn't fully understand what you were saying about booleans. Do you mean to be careful with checking a boolean for its intended value? As in, make sure to only check if true/false booleans are true or false, not HIGH or LOW? Should I use an int to define HIGH and LOW, and reserve boolean for true/false?
To answer your other question, the switch is wired with a 10k resistor on the same side as the input wire going to the Arduino, the other side is wired to 5v. Essentially the same as the button tutorial on the examples page.
Thinking about it, the debounce function only seems necessary for the windowButton so that the rollWindow function does not get toggled off due to bouncing. However, it's also useful for keeping track of the button toggles and timing.

As per PaulS' suggestions and robtillaart's catch, I have revised certain parts of the sketch:

My void setup() is now in a clean for loop. Though the second for loop doesn't seem to save me much unless that array were bigger. I didn't think I could use the same for loop for windowPin, as its array size is only 2 and I would be accessing past the array index?

void setup () {
  for (int i = 0; i < 3; i++) {
    pinMode (buttonPin[i], INPUT);
  }
  for (int i = 0; i < 2; i++) {
  pinMode (windowPin[i], OUTPUT);
  }
  pinMode (ledPin, OUTPUT);
}

Cleaned up the == true and == false if statements

if (buttonToggle[2]) { //Start function when buttonToggle is true
    if (doorSwitch[0] == HIGH) { //If the door is open
      rollWindowToggle = true; //Let the rollWindow function execute
    }
  }
  else rollWindowToggle = false;

  // Test to see if rollWindowToggle was toggled on while the door was open,
  //if it was, then reset toggleTime so the window will still roll the correct
  //amount of time. continueFunction must not equal true so toggleTime does not
  //continuously reset if rollWindowToggle gets toggled while the function is
  //in progress.
  if (!rollWindowToggle && doorSwitch[0] == HIGH && !continueFunction) {
    toggleTime[0] = millis();
  }

And I changed my switch statement to an if/else statement

  if (doorSwitch[thisSwitch] == HIGH) {

    // Define the state of continueFunction to indicate that the function is
    //in progress and should continue until it is complete
    continueFunction = true;

    // If the rollDownDelay time has passed, stop rolling the window. If not,
    //roll the window down. Make windowPin[1] LOW to ensure the window will
    //not try to roll down and up at the same time.
    if (currentTime - toggleTime[thisSwitch] > rollDownDelay) { //If roll up time has passed:
      digitalWrite (windowPin[0], LOW); //Stop rolling the window down
    }
    else {
      digitalWrite (windowPin[1], LOW); //Make sure the window is not rolling up
      digitalWrite (windowPin[0], HIGH); //Make the window roll down
    }
  }
  // If the door is closed:
  else {
    // If the rollUpDelay time has passed, stop rolling the window, reset
    //buttonToggle[2] to false, and make continueFunction false to indicate
    //the function is complete. If rollUpDelay time has not passed, then
    //roll the window up and make windowPin[0] LOW to ensure the window will
    //not try to roll up and down at the same time.
    if (currentTime - toggleTime[thisSwitch] > rollUpDelay) { 
      digitalWrite (windowPin[1], LOW); 
      buttonToggle[2] = false; 
      continueFunction = false;
    }
    else {
      digitalWrite (windowPin[0], LOW); 
      digitalWrite (windowPin[1], HIGH);
    }
  }

The issue robtillaart pointed out

boolean buttonToggle[3] = {false, false, false};

Thanks again

I didn't fully understand what you were saying about booleans. Do you mean to be careful with checking a boolean for its intended value? As in, make sure to only check if true/false booleans are true or false, not HIGH or LOW?

Checking that booleans are true/false is what I meant. Currently you have

boolean doorSwitch[3];
  if (doorSwitch[0] == HIGH || doorSwitch[1] == HIGH)

This works but is not as clear as it might be.

My void setup()

"My setup() function"!

Though the second for loop doesn't seem to save me much unless that array were bigger.

No, it doesn't. Until you change the size of the array. What you really should have is the loop indexes be based on the size of the arrays, so that a change in the number of elements in the array can be made without having the change any other code.

  for (int i = 0; i < sizeof(buttonPin)/sizeof(buttonPin[0]); i++) 
  {
    pinMode (buttonPin[i], INPUT);
  }

Now, you could change the size of the array, and the for loop would still iterate the correct number of times.

I didn't think I could use the same for loop for windowPin, as its array size is only 2 and I would be accessing past the array index?

Correct. Optimization has to consider factors like this.

robtillaart:

boolean buttonToggle[3] = {false};

this sets only the first element to false, the others are undefined....

The others are default initialized. As boolean is an uint8_t, its default value is zero. The line above produces the same result as:

boolean buttonToggle[3] = {};

"My setup() function," got it. The sizeof() operator makes perfect sense. Is it better then to create a variable that contains the size of the array, rather then using the sizeof() operator every time I need it? For instance: int buttonPinSize = sizeof(buttonPin/sizeof(buttonPin[0]); then use buttonPinSize anywhere I need to define the size of that array. I also don't understand why I need to divide by the size of buttonPin[0], as I thought the size of buttonPin[0] would be 1? Is the division only necessary if the byte is larger than 255, and thus would contain 2 or more bytes and throw off the sizeof() value, but is good practice anyway? Or am I all screwed up here?

UKHeliBob, would it be better to change

boolean doorSwitch[3];

to

int doorSwitch[3];

to make it more clear that I'm actually checking for HIGH/LOW rather than true/false?

pYro_65, it sounds like

boolean buttonToggle[3] = {false, false, false};

is the same as

boolean buttonToggle[3] = {};

as the values would default to 0, which is the same as false?

Thanks, everyone for your time.

UKHeliBob, would it be better to change
Code:
boolean doorSwitch[3];
to
Code:
int doorSwitch[3];
to make it more clear that I'm actually checking for HIGH/LOW rather than true/false?

const byte doorSwitch[3];would be better. const because the values will never change and byte because it takes less space.

Is it better then to create a variable that contains the size of the array, rather then using the sizeof() operator every time I need it?

The size of the array is fixed at compile time, so the sizeof macro isn't actually called at run time. The compiler replace the two sizeof "function calls" with a constant. So, in general, no, the variable will not reduce code size or execution time.

Acliad:
pYro_65, it sounds like

boolean buttonToggle[3] = {false, false, false};

is the same as

boolean buttonToggle[3] = {};

as the values would default to 0, which is the same as false?

Yep, that is correct. Also on an AVR, unless you specify, all global variables will contain zero on start up, so you can save some flash and remove the initialization as the variable is already zero/false. This does not apply to local variables though ( variables defined inside a function )

It sounds like I'm in good shape for now. Thanks everyone for your help! UKHeliBob, I changed the values to bytes, but unless I was misunderstanding you, doorSwitch[] isn't a constant, as it's changed by

  // Define variables to debounce and keep track of the different switches
  doorSwitch[0] = debounce(0); 
  doorSwitch[1] = debounce(1);

In the loop() function.

Again, thanks to everyone for clearing up my questions, I appreciate it!

Sorry for the bum steer on the use of const for that variable.

pYro_65:

Acliad:
pYro_65, it sounds like

boolean buttonToggle[3] = {false, false, false};

is the same as

boolean buttonToggle[3] = {};

as the values would default to 0, which is the same as false?

Yep, that is correct. Also on an AVR, unless you specify, all global variables will contain zero on start up, so you can save some flash and remove the initialization as the variable is already zero/false. This does not apply to local variables though ( variables defined inside a function )

A simple test shows that the compiler optimizes the {false, false, false}; initialization as it is equal in size compared to {}

Explicitly initialize the variables improves the robustness, readability and portability of the code.

  • robustness: when you decide the global array can be moved to a local array it will be initialized correctly
  • readability: you can see explicit what the initial value is.
  • portability: if another compiler (setting) initializes global var differently (or not at all)

robtillaart:
Explicitly initialize the variables improves the robustness, readability and portability of the code.

  • robustness: when you decide the global array can be moved to a local array it will be initialized correctly
  • readability: you can see explicit what the initial value is.
  • portability: if another compiler (setting) initializes global var differently (or not at all)

For sure with respect to global variables that aren't initialized at all on the AVR, however {} is an explicit 'default initialization'. I wouldn't recommend manually defaulting all the elements of say: bool Settings[ 4000 ]; over using a single short line.

Default initialization is not just an idiom of arrays, it is used for POD's too.

struct Foo{
  char a;
  int b;
  float c;
  bool d;
};

//Can be default initialized:
Foo f = {};

Also when using dynamic memory, default initialization is all you can do with arrays ( slightly different syntax ).

char *arr = new char[ max ]();