2
\$\begingroup\$

I made a little JavaScript app here that let people generate bitmap for generating characters. Currently it generate code of a dictionary. But I want to also generate a example that let them play like the Arduino samples, use it as starting point or use it as-is.

This is the code I wrote to enable someone to use 7-segment displays

I want some feedback on the code itself also things like suggestions for variable and function names, order of things, aesthetics etc...

//common cathode turns on with HIGH, and common anode turns on with LOW
const bool LED_P HIGH

const uint8_t myDisplayPins[8] = {3, 4, 5, 6, 7, 8, 9, 10}; //dp A B C D E F G

byte font[] = {
    B1111110,   // [0] => "0"
    B0110000,   // [1] => "1"
    B1101101,   // [2] => "2"
    B1111001,   // [3] => "3"
    B0110011,   // [4] => "4"
    B1011011,   // [5] => "5"
    B1011111,   // [6] => "6"
    B1110000,   // [7] => "7"
    B1111111,   // [8] => "8"
    B1111011,   // [9] => "9"
    B1110111,   // [10] => "A"
    B0000001,   // [11] => "dash"
};

void setup(){
    display7Setup(myDisplayPins); //configure myDisplayPins as outputs
}

void loop(){

  display7(myDisplayPins, font[0]); //display "0"
  delay(1000);
  display7(myDisplayPins, font[9]); //display "A"
  delay(1000);
  display7(myDisplayPins, font[11]); //display "-"
  delay(1000);
  display7(myDisplayPins, B00000000); //turn all segments OFF
  delay(1000);

  //count 0 to 9
  for(int i=0; i<=9; i++){
    display7(myDisplayPins, font[i]);
    delay(1000);
  }

}


/* Configure pins as outputs */
void display7Setup(const uint8_t displayPins[]){
  for(uint8_t i=0; i<8; ++i){
    pinMode(displayPins[i], OUTPUT);
  }
}

/* Function that writes a bitmap to a 7-segment display */
void display7(const uint8_t displayPins[], byte bitmap){
  for(uint8_t i=0; i<8; ++i){
    byte segment = bitmap & (0x1<<i); //apply a mask to select just the desired segment bit
    bool state = segment>>i;          //shift the bit to LSF (rightmost) to get a boolean value 0x1 or 0x0
    if(!LEDP) state = !state;         //invert level for common anode
    digitalWrite(displayPins[i], state);
  }
}
\$\endgroup\$
2
\$\begingroup\$
  • I would rename loop to demo.

  • The spec says that the value argument to digitalWrite may only be HIGH or LOW, and it doesn't spell out their bit representations. Even though your code works (I presume), it is better to stick with the spec.

  • LEDP never changes. You don't have to invert each individual bit in the loop. It looks better to invert the whole bitmap at once:

    if (LEDP == LOW) {
        bitmap ^= 0xff;
    }
    
  • Now the body of display7 loop could be streamlined to

    for (uint8_t i=0; i<8; ++i) {
        digitalWrite(displayPins[i], (bitmap & 0x01)? HIGH: LOW);
        bitmap >>= 1;
    }
    

    Notice that segment and state disappear altogether.

\$\endgroup\$
  • \$\begingroup\$ on the arduino.h high/low is the same as true and false #define HIGH 0x1 #define LOW 0x0. But I like to keep it semantic. but why not use ~ ? \$\endgroup\$ – Vitim.us Nov 18 '14 at 22:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.