7
\$\begingroup\$

I have a very simple C program that continuously reads from a temperature sensor and displays the temperature on two 7-segment LEDs. Reading is simple, but to display the LEDs I need to write to 14 GPIO pins on every iteration. It gets real ugly.

I am running this program on an embedded device (BeagleBone Black), which makes certain aspects more of a concern. In general, would using array structure to manage files be more expensive in terms of memory and performance? Or is the difference trivial enough to be negligible, regardless of the number of files?

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <stdbool.h>
#include <math.h>

// function to light up a segment.
// note that since the led has a common-anode design, 
// a segment lights up when LOW is applied on its pin.
void printSeg(char* segFile) {
    FILE* seg;
    if ((seg = fopen(segFile, "r+")) != NULL) {
        fwrite("0", sizeof(char), 1, seg);
        fclose(seg);
    }
}

// function to light off a segment. 
// note that since the led has a common-anode design, 
// a segment lights off when HIGH is applied on its pin.
void unprintSeg(char* segFile) {
        FILE* seg;
        if ((seg = fopen(segFile, "r+")) != NULL) {
                fwrite("1", sizeof(char), 1, seg);
                fclose(seg);
        }
}

// function to light up the 7 segments of each LED accordingly to display the right temperature
void printTemp(int temperature) {
        // path to all gpio pins used to control the 7 segments on each LED
        char *leftAFile = "/sys/class/gpio/gpio66/value";
        char *rightAFile = "/sys/class/gpio/gpio67/value";
        char *rightFFile = "/sys/class/gpio/gpio68/value";
        char *leftFFile = "/sys/class/gpio/gpio69/value";
        char *leftEFile = "/sys/class/gpio/gpio45/value";
        char *rightEFile = "/sys/class/gpio/gpio44/value";
        char *leftBFile = "/sys/class/gpio/gpio23/value";
        char *rightBFile = "/sys/class/gpio/gpio26/value";
        char *leftGFile = "/sys/class/gpio/gpio47/value";
        char *rightGFile = "/sys/class/gpio/gpio46/value";
        char *leftCFile = "/sys/class/gpio/gpio27/value";
        char *rightCFile = "/sys/class/gpio/gpio65/value";
        char *leftDFile = "/sys/class/gpio/gpio22/value";
        char *rightDFile = "/sys/class/gpio/gpio61/value";

    // get the tenth and unit digits of the temperature
    // print the actual temperature, tenth digit, and unit digit for debugging purpose
    int tenthDigit = temperature/10;
    int unitDigit = temperature%10;
    printf("%d, %d, %d\n", temperature, tenthDigit, unitDigit); 

    // print the tenth digit depending on what the number is.
    // Each number (0-9) has a corresponding configuraturation. 
    // Note that every segment is taken care of for each number.
    // This is necessary because some segments that should be off 
    // may have been on to display a previous number
    switch (tenthDigit) {
        case 0: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftDFile);
                printSeg(leftEFile);
                printSeg(leftFFile);
                unprintSeg(leftGFile);
                break;
        case 1: printSeg(leftCFile);
                printSeg(leftBFile);
                unprintSeg(leftGFile);
                unprintSeg(leftAFile);
                unprintSeg(leftDFile);
                unprintSeg(leftFFile);
                unprintSeg(leftEFile);
                break;
        case 2: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftGFile);
                printSeg(leftDFile);
                printSeg(leftEFile);
                unprintSeg(leftFFile);
                unprintSeg(leftCFile);
                break;
        case 3: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftDFile);
                printSeg(leftGFile);
                unprintSeg(leftFFile);
                unprintSeg(leftEFile);
                break;
        case 4: printSeg(leftGFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftFFile);
                unprintSeg(leftAFile);
                unprintSeg(leftDFile);
                unprintSeg(leftEFile);
                break;
        case 5: printSeg(leftAFile);
                printSeg(leftCFile);
                printSeg(leftDFile);
                printSeg(leftGFile);
                printSeg(leftFFile);
                unprintSeg(leftBFile);
                unprintSeg(leftEFile);
                break;
        case 6: printSeg(leftGFile);
                printSeg(leftCFile);
                printSeg(leftDFile);
                printSeg(leftEFile);
                printSeg(leftFFile);
                printSeg(leftAFile);
                unprintSeg(leftBFile);
                break;
        case 7: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftFFile);
                unprintSeg(leftGFile);
                unprintSeg(leftDFile);
                unprintSeg(leftEFile);
                break;
        case 8: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftDFile);
                printSeg(leftEFile);
                printSeg(leftFFile);
                printSeg(leftGFile);
                break;
        case 9: printSeg(leftAFile);
                printSeg(leftBFile);
                printSeg(leftCFile);
                printSeg(leftGFile);
                printSeg(leftFFile);
                printSeg(leftDFile);
                unprintSeg(leftEFile);
                break;
    }

    switch (unitDigit) {
        case 0: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightDFile);
                printSeg(rightEFile);
                printSeg(rightFFile);
                unprintSeg(rightGFile);
                break;
        case 1: printSeg(rightCFile);
                printSeg(rightBFile);
                unprintSeg(rightGFile);
                unprintSeg(rightAFile);
                unprintSeg(rightDFile);
                unprintSeg(rightFFile);
                unprintSeg(rightEFile);
                break;
        case 2: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightGFile);
                printSeg(rightDFile);
                printSeg(rightEFile);
                unprintSeg(rightCFile);
                unprintSeg(rightFFile);
                break;
        case 3: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightDFile);
                printSeg(rightGFile);
                unprintSeg(rightFFile);
                unprintSeg(rightEFile);
                break;
        case 4: printSeg(rightGFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightFFile);
                unprintSeg(rightAFile);
                unprintSeg(rightDFile);
                unprintSeg(rightEFile);
                break;
        case 5: printSeg(rightAFile);
                printSeg(rightCFile);
                printSeg(rightDFile);
                printSeg(rightGFile);
                printSeg(rightFFile);
                unprintSeg(rightBFile);
                unprintSeg(rightEFile);
                break;
        case 6: printSeg(rightGFile);
                printSeg(rightCFile);
                printSeg(rightDFile);
                printSeg(rightEFile);
                printSeg(rightFFile);
                printSeg(rightAFile);
                unprintSeg(rightBFile);
                break;
        case 7: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightFFile);
                unprintSeg(rightGFile);
                unprintSeg(rightDFile);
                unprintSeg(rightEFile);
                break;
        case 8: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightDFile);
                printSeg(rightEFile);
                printSeg(rightFFile);
                printSeg(rightGFile);
                break;
        case 9: printSeg(rightAFile);
                printSeg(rightBFile);
                printSeg(rightCFile);
                printSeg(rightGFile);
                printSeg(rightFFile);
                printSeg(rightDFile);
                unprintSeg(rightEFile);
                break;
        }       
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Your question looks good, hope you get good answers! \$\endgroup\$ – ferada Apr 23 '15 at 23:15
  • \$\begingroup\$ @ferada Thanks for the warm welcome! And thanks for correcting typos in my question! \$\endgroup\$ – Thomas Hsieh Apr 24 '15 at 1:12
  • \$\begingroup\$ Are you sure that each whole 7-seg LED is not also represented by a single path/device? If the driver source is available it might be better to modify it to provide such a single path. \$\endgroup\$ – William Morris May 17 '15 at 15:14
  • \$\begingroup\$ @WilliamMorris That would've made my life much easier, but the 7-seg LED I had must be set up this way. \$\endgroup\$ – Thomas Hsieh May 17 '15 at 17:56
  • \$\begingroup\$ Unless I'm missing something, that is just the device datasheet. It says (and should say) nothing about how a driver should be implemented. \$\endgroup\$ – William Morris May 17 '15 at 23:01
5
\$\begingroup\$

Here are some things that may help you improve your program.

Use all required #includes

The program uses fopen but doesn't #include <stdio.h>. It should.

Use const where practical

Whenever you pass a pointer, ask yourself whether the called function should be allowed to modify the contents of the pointed-to memory. If not, then that parameter should be const. For example:

void printSeg(const char* segFile) {

Combine similar functions

The only difference between your printSeg and unprintSeg is the value written. I think it would make more sense to pass that as a value:

// note that since the led has a common-anode design, 
// a segment lights off when HIGH is applied on its pin.
void setSeg(const char* segFile, bool val) {
        FILE* seg;
        if ((seg = fopen(segFile, "r+")) != NULL) {
                fwrite(val ? "0" : "1", sizeof(char), 1, seg);
                fclose(seg);
        }
}

Separate calculation from I/O

The mapping of a value to a 7-segment display is common to both digits. The only thing that changes is the mapping of segments to file names. This suggests that a more compact way to approach this would be to use a two-level data structure. One structure would map from digit to segments and the other maps from segments to filenames. Here's test code I wrote to demonstrate the concept. Obviously, you'd need to use your actual file names and use the setSeg routine above rather than my toy version:

void setSeg(const char* segFile, bool val) {
        printf("%s = %d\n", segFile, val);
}

/*
 * segments are defined in this order: A, F, B, G, E, C, D
 * to make it easy to verify visually.
 */
const bool segment[][7] = {
    /* 0 */ {    1,
               1,  1,
                 0,
               1,  1,
                 1      },
    /* 1 */ {    0,
               0,  1,
                 0,
               0,  1,
                 0      },
    /* 2 */ {    1,
               0,  1,
                 1,
               1,  0,
                 1      },
    /* 3 */ {    1,
               0,  1,
                 1,
               0,  1,
                 1      },
    /* 4 */ {    0,
               1,  1,
                 1,
               0,  1,
                 0      },
    /* 5 */ {    1,
               1,  0,
                 1,
               0,  1,
                 1      },
    /* 6 */ {    1,
               1,  0,
                 1,
               1,  1,
                 1      },
    /* 7 */ {    1,
               0,  1,
                 0,
               0,  1,
                 0      },
    /* 8 */ {    1,
               1,  1,
                 1,
               1,  1,
                 1      },
    /* 9 */ {    1,
               1,  1,
                 1,
               0,  1,
                 0      },
    /* A */ {    1,
               1,  1,
                 1,
               1,  1,
                 0      },
    /* b */ {    0,
               1,  0,
                 1,
               1,  1,
                 1      },
    /* C */ {    1,
               1,  0,
                 0,
               1,  0,
                 1      },
    /* d */ {    0,
               0,  1,
                 1,
               1,  1,
                 1      },
    /* E */ {    1,
               1,  0,
                 1,
               1,  0,
                 1      },
    /* F */ {    1,
               1,  0,
                 1,
               1,  0,
                 0      }
};

/* 
 * segfiles are defined in this order: A, F, B, G, E, C, D
 * to match with segment order
 */
const char *segfile[2][7] = {
    { "A", "F", "B", "G", "E", "C", "D" },
    { "a", "f", "b", "g", "e", "c", "d" }
};


void segs(int n)
{
    int lo = n % 10;
    int hi = n / 10;
    for (int i=0; i < 7; ++i) {
        setSeg(segfile[0][i], segment[hi][i]);
        setSeg(segfile[1][i], segment[lo][i]);
    }
}

int main()
{
    segs(83);
}

Consider performance implications

Embedded systems work typically involves sometimes extreme limitations in terms of memory and processing speed that makes such work fundamentally different from writing software that runs on Windows or Linux or OS X, but a BeagleBone Black is a pretty high end embedded system, and so would be much closer to writing programs for an regular Linux workstation. For that reason, I'd recommend doing timing experiments to see if your update rate is fast enough. If not, one thing you might consider is opening the files once and keeping them open during the running of the program. Doing so would allow the setSeg routine to look like this:

void setSeg(FILE* seg, bool val) {
    fwrite(bool ? "0" : "1", sizeof(char), 1, seg);
}

As you can see, you would now be passing file handles rather than opening and closing a file for each segment update. This is undoubtedly going to be faster -- the question is whether the increase in speed is worth the extra code that will have to be written to make sure all files are correctly closed even in case of an error.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I figured that the missing #include was probably just a cut-and-paste error, but mentioning it might help the next person who reads this. Glad the rest of it was useful to you. That's why we're here! :) \$\endgroup\$ – Edward Apr 24 '15 at 1:22
  • 1
    \$\begingroup\$ I've added a section to my answer to attempt to address it. \$\endgroup\$ – Edward Apr 24 '15 at 2:58
  • 1
    \$\begingroup\$ Typically, files only have to be flushed, not necessarily closed to update values. See fflush for details. \$\endgroup\$ – Edward Apr 24 '15 at 3:11
  • 1
    \$\begingroup\$ ... although setting the file to unbuffered may be the better option here. \$\endgroup\$ – user14393 Apr 24 '15 at 5:09
  • 1
    \$\begingroup\$ @Thomas: setvbuf or setbuf. (disclaimer: I've never tried actually using this functionality myself) \$\endgroup\$ – user14393 Apr 26 '15 at 3:24
2
\$\begingroup\$

I'd like to see printSeg deal with an array rather than a single address (release the repeated calls from the stack). i.e. for every address in list, open address and write "0" (or "1").

Also, I figure merging the unit digit and the ten digit code so that you don't repeat your self on the segments. i.e. case digit 5: light up relevant segments array [0,1,1,0,...] in digit_index=0 for ones (or 1 for tens). This will make the code smaller and give you more flexibility in adding digits quickly (without duplicating the case 0 to 9 code again).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips! Could you answer my updated question as well? \$\endgroup\$ – Thomas Hsieh Apr 24 '15 at 1:37

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.