Forum Rule: Always post complete source code & details to reproduce any issue!
Results 1 to 7 of 7

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

  1. #1
    Junior Member
    Join Date
    May 2018
    Location
    Baltimore
    Posts
    15

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

    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.
    Click image for larger version. 

Name:	JOFU5270.jpg 
Views:	17 
Size:	120.6 KB 
ID:	13947

    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.

  2. #2
    Junior Member
    Join Date
    May 2018
    Location
    Baltimore
    Posts
    15
    Sorry about referencing line numbers that aren't there ^^

  3. #3
    Senior Member
    Join Date
    Dec 2016
    Location
    Montreal, Canada
    Posts
    3,276
    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

  4. #4
    Junior Member
    Join Date
    May 2018
    Location
    Baltimore
    Posts
    15
    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!

  5. #5
    Junior Member
    Join Date
    May 2018
    Location
    Baltimore
    Posts
    15
    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;
      }
    }

  6. #6
    Senior Member
    Join Date
    Dec 2016
    Location
    Montreal, Canada
    Posts
    3,276
    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;
      }
    }

  7. #7
    Junior Member
    Join Date
    May 2018
    Location
    Baltimore
    Posts
    15
    Thanks again tonton81,
    I'll happily save that byte.
    A good deal of freeverb is on the horizon.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •