Request for code review. pretty basic... i think

Status
Not open for further replies.

flanban

Member
Hi, I'm not a professional programmer, just a hobbyist and I want to make sure I'm going about my programming statements and methods in the right way. I appreciate any help and I aim to not waste anyone's time. That said, here is probably too much information. code & questions are at the end.

What it does:
- A rotary encoder sequentially turns on one led at a time out of an array of ten. very exciting.

Wiring details:
- here's a photo. I didn't make a diagram because it's pretty simple and my questions are all software related.
JOFU5270.jpg

PJRC & other products used:
- teensy 3.2
- pjrc audio shield(not being used in this sketch)
- PEC11 rotary encoder https://cdn-shop.adafruit.com/datasheets/pec11.pdf
- 3mm blue LEDs https://www.adafruit.com/product/780
- resistors 2.2k ohm(all I had on hand)

Software setup:
- building via Arduino 1.8.5 on OSX 10.12.6

Code:
 #include <Encoder.h>

// things on pins
Encoder rotaryEnc(4, 5);
int ledPins[] = {14, 12, 16, 17, 11, 7, 20, 21, 22, 23};

// global vars
int pinCount = 10;
long oldPosition  = -999;
int ledSetting = 1;

// functions
void allSettingsLedsOff() {
  for (int currentPin = 0; currentPin < pinCount; currentPin++) {
    digitalWrite(ledPins[currentPin], LOW);
  }
}

void setup() {
  for (int currentPin = 0; currentPin < pinCount; currentPin++) {
    pinMode(ledPins[currentPin], OUTPUT);
  }
}

void loop() {

  // encoder knob sets active setting via the ledSetting var
  long newPosition = rotaryEnc.read();
  if (newPosition > oldPosition + 2) {  // QUESTION 1
    if (ledSetting > 0) {
      ledSetting--;
    } else {
      ledSetting = pinCount - 1;
    }
    oldPosition = newPosition;
  } else if (newPosition < oldPosition - 2) {
    if (ledSetting < pinCount - 1) {
      ledSetting++;
    } else {
      ledSetting = 0;
    }
    oldPosition = newPosition;
  }

  // do stuff per setting
  switch (ledSetting) {
    case 0:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 1:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 2:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 3:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 4:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 5:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 6:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 7:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 8:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    case 9:
      allSettingsLedsOff();
      digitalWrite(ledPins[ledSetting], HIGH);
      break;
    }

}

Code questions:
1. is this an okay way to debounce a rotary encoder(line 29 & 36)?
2. in the switch case is there a way to avoid repeating: digitalWrite(ledPins[ledSetting], HIGH); for each case?
3. For handling the encoder, starting on line 28, are the nested if statements the best way to go about that? It feels clunky to me, but I can't think of a way to reduce the code.
4. In each switch case is there a way to run allSettingsLedsOff() once per case or a better way to achieve turning off the last LED that was on. Since only one should be on at any given time I figured keeping track of a history state wasn't necessary and while this works it bothers me that the active LED is blinking with each loop(albeit imperceptible at full speed).

Any and all advice is welcomed. Thank you.
 
the switch case makes no sense? if theyre all identical you dont need a switch statement at all, just run allsettings... & digitalwrite thats it

you could store a uint16_t datatype and everytime you enable a led you set the bit position.
next time a led is set you check that variable, disable the led and then the bit, then enable your new led and set new bit, this will allow you to keep track and enable/disable open leds rather than polling
 
oops, yes the switch case is unnecessary. I was envisioning adding various functions to knobs based off the current ledSetting variable and that led me to learning what a switch case was. I guess I jumped the gun a bit on using one.

I am trying out your suggestions now. Thanks!
 
I think I've followed the suggestions. I used the existing ledSetting as the variable that keeps track of the LEDs' state. My approach still feels a little awkward, but there's less code and I've learned a couple things, so I'm feeling good.

Code:
#include <Encoder.h>

// things on pins
Encoder rotaryEnc(4, 5);
uint16_t ledPins[] = {14, 12, 16, 17, 11, 7, 20, 21, 22, 23};

// global vars
uint16_t pinCount = 10;
long oldPosition  = -999;
uint16_t ledSetting = 1;

void setup() {
  for (int currentPin = 0; currentPin < pinCount; currentPin++) {
    pinMode(ledPins[currentPin], OUTPUT);
  }
}

void loop() {
  long newPosition = rotaryEnc.read();
  if (newPosition > oldPosition + 2) {
    if (ledSetting > 0) {
      digitalWrite(ledPins[ledSetting], LOW);
      ledSetting--;
      digitalWrite(ledPins[ledSetting], HIGH);
    } else {
      digitalWrite(ledPins[ledSetting], LOW);
      ledSetting = pinCount - 1;
      digitalWrite(ledPins[ledSetting], HIGH);
    }
    oldPosition = newPosition;
  } else if (newPosition < oldPosition - 2) {
    if (ledSetting < pinCount - 1) {
      digitalWrite(ledPins[ledSetting], LOW);
      ledSetting++;
      digitalWrite(ledPins[ledSetting], HIGH);
    } else {
      digitalWrite(ledPins[ledSetting], LOW);
      ledSetting = 0;
      digitalWrite(ledPins[ledSetting], HIGH);
    }
    oldPosition = newPosition;
  }
}
 
Yup, your incrementing just using a value instead of bit, in that case, even a uint8_t should suffice to save an extra byte of memory, although, thats not a major loss in teensy memory anyways :)

you could also include calculations in the digitalWrites as well:

Code:
void loop() {
  long newPosition = rotaryEnc.read();
  if (newPosition > oldPosition + 2) {
    if (ledSetting > 0) {
      digitalWrite(ledPins[ledSetting], LOW);
      digitalWrite(ledPins[ ( ledSetting-- ) ], HIGH);
    } else {
      digitalWrite(ledPins[ledSetting], LOW);
      digitalWrite(ledPins[ ( ledSetting = pinCount - 1 ) ], HIGH);
    }
    oldPosition = newPosition;
  } else if (newPosition < oldPosition - 2) {
    if (ledSetting < pinCount - 1) {
      digitalWrite(ledPins[ledSetting], LOW);
      digitalWrite(ledPins[ ( ledSetting++ ) ], HIGH);
    } else {
      digitalWrite(ledPins[ledSetting], LOW);
      digitalWrite(ledPins[ (ledSetting = 0) ], HIGH);
    }
    oldPosition = newPosition;
  }
}
 
Status
Not open for further replies.
Back
Top