You are here

Request for Code Review

3 posts / 0 new
Last post
#1 Tue, 2016-04-26 11:11  

Request for Code Review

Ladvien
Offline
Last seen: 2 months 1 week ago
Joined: 25 Sep 2015 - 23:05

Would someone mind critiquing this code for me?

It's my first real attempt at state-machine.  It is a lamp which is driven by a potentiometer and a PIR sensor.  The plan was the following:

1. If the potentiometer is adjusted a countdown timer is set and the LEDs come on for the duration.

2.If the PIR sensor goes high, a countdown timer is set and the LEDs come on.

3. The LEDs brightness is adjustable in real-time by the potentionmeter.

I was following Bdk6's article:  State Machine


/*  C. Thomas Brittain
 *  04-24-16
 *  LED State Machine
 *  M   T   State
 *  0   0   LED Adjusting Dimmer
 *  1   0   LED Off
 *  0   1   LED Adjusting Brighter
 *  1   1   LED On
 */

const int pirPin = 7;
const int potPin = A3;
const int timerThreshold =  10000; //* 1000;
const int onDuration = 10;
const int offDuration = 1;

enum LED_STATES {
  INITIAL,
  OFF,
  ADJ_BRIGHTNESS,
  ON
};

// State Machine inputs.
int match = 0;
int timer = 0;

// Holds brightness level for adjustment comparisons.
int currentBrightness = 0;

void setup() {
  pinMode(pirPin, INPUT);
  pinMode(potPin, INPUT);
  Serial.begin(9600);
}

int LED_state_machine(){
  static int state = INITIAL;
  static int oldPotValue = 0;
  int newState = state;
     
  int potValue = potRead();
  int pirValue = pirRead();



  timer = getTimerValue(timer);
  if(timer <= 0){ potValue = 0;}
  match = checkIfPotMatchesBrightness(potValue);
  //if(potValue != oldPotValue){ timer = timerThreshold;}      


  //Serial.print("Match: ");
  
  //Serial.println(match);
  //Serial.print("Timer: ");
  //Serial.println(timer);
  
  switch(state){
    case INITIAL:
      newState = OFF;
      //Serial.println("Initial");
      break;
    case OFF:
      if(true != match && timer > 0){
        newState = ADJ_BRIGHTNESS;
      }
      //Serial.println("Off");
      break;
    case ADJ_BRIGHTNESS:
      if(true == match && timer > 0){
        newState = ON;
      } else if(true == match && timer <= 0){
        newState = OFF;
      }
      else {
        adjustLeds(potValue);
      }
      //Serial.println("ADJ");
      break;
    case ON:
      timer--;
      if(timer <= 0){
        newState = ADJ_BRIGHTNESS;
      } else if(true != match){
        newState = ADJ_BRIGHTNESS;
      } else {
        newState = ON;
      }
      //Serial.println("On");
      break;
    default:
      newState = OFF;
      //Serial.println("Default");
      break;      
  }
  state = newState;
  oldPotValue = potValue;
  return newState;
}

bool checkIfPotMatchesBrightness(int potValue){
  int mappedPotValue = map(potValue, 0, 1023, 0, 255);
  if(mappedPotValue == currentBrightness){
    //Serial.print("True: ");
    //Serial.println(mappedPotValue);
    return true; 
  } else {
    //Serial.print("False: ");
    //Serial.print(mappedPotValue);
    //Serial.print(", ");
    //Serial.print(currentBrightness);
    return false;
  }
}

int potRead(){
  static int oldPotRead = 0;
  int potValue = 0;
  for(int i = 0; i < 15; i ++){
    potValue += analogRead(potPin);  
    delay(1);
  }
  potValue = potValue / 15;
  if(potValue > oldPotRead + 4 || potValue < oldPotRead - 4){
    oldPotRead = potValue;
    return potValue;
  } else {
    return oldPotRead;
  }
}

int pirRead(){
  //Serial.print("PIR PIN: ");
  //Serial.println(digitalRead(pirPin));
  return digitalRead(pirPin);
}

int getTimerValue(int oldTimerValue){
  if(pirRead()){
    // Resets the timer.
    return timerThreshold;
  } else {
    // No need to reset, return current countdown value.
    return oldTimerValue;
  }
}

void adjustLeds(int potValue){

  int mappedBrightnessToAchieve = map(potValue, 0, 1023, 0, 255);
  int brightnessStep = 0;
  brightnessStep = currentBrightness;
  if(mappedBrightnessToAchieve > currentBrightness){
    while(brightnessStep < mappedBrightnessToAchieve){
      analogWrite(5, brightnessStep);
      analogWrite(9, brightnessStep);
      analogWrite(10, brightnessStep);
      analogWrite(11, brightnessStep);
      brightnessStep++;
      if(brightnessStep > mappedBrightnessToAchieve){ brightnessStep = mappedBrightnessToAchieve;}
      delay(onDuration);
    }
    currentBrightness = brightnessStep;
    //Serial.print("currentBrightness 1: ");

  } else if(mappedBrightnessToAchieve < currentBrightness){

    while(brightnessStep > mappedBrightnessToAchieve){
      analogWrite(5, brightnessStep);
      analogWrite(9, brightnessStep);
      analogWrite(10, brightnessStep);
      analogWrite(11, brightnessStep);
      brightnessStep--;
      if(brightnessStep < 0){brightnessStep = 0;}
      delay(offDuration);
    }
    currentBrightness = brightnessStep; 
    //Serial.print("currentBrightness 2: ");
  }
    //Serial.println(currentBrightness);
}

void loop() {
  LED_state_machine();
}

 

Wed, 2016-04-27 23:23  

Good evening Mr. Bdk6,

Ladvien
Offline
Last seen: 2 months 1 week ago
Joined: 25 Sep 2015 - 23:05

Good evening Mr. Bdk6,

1. Yes, not sure if you can see it here.  There were two different versions.  I realized the diagram could be simplified.

 

2. IF the LEDs are already and the pot moves or the PIR is activated, then the timer should be reset to threshold

if(tPIR == true || potRead != oldPotRead){ timer = countdownThreshold;}

3. Yes, sir.  The LEDs should go off.

4.  The ADJ_BRIGHTNESS under ON allows the LEDs to fade off, instead of a sharp off.

5.  Great point on the "default" case, sir.  It is one of the goals I'm setting now days; nothing is finished until it accounts for error cases.

Wed, 2016-09-28 16:07  

Hi....i am a new user here.As

AlfySande
Offline
Last seen: 7 months 4 weeks ago
Joined: 26 Sep 2016 - 13:48

Hi....i am a new user here.As per my knolwedge in the "ON" case the first "IF" sets the newstate to "ADJ_BRIGHTNESS" if the timer <= 0.Also i want to know how can you get to the "default" case?  If you get there, you set newstate=OFF.  Is that specified behavior?  From what I see that would be some error condition.  I would recommend a comment at least, and better yet some sort of error indication if it is an error.  If it is NOT an error, shouldn't it have a specified case?

turnkey pcb