3
\$\begingroup\$

I am writing program for Raspberry Pi that will show indeterminate progress while user waiting. The program will start automatically and will be killed by another shell script.

Here is what I have at the moment. It works, but I feel that it is not perfect. I will be changing this code frequently after users will give their feedback.

#include <wiringPi.h>
#include <stdio.h>

#define PINS_NUMBER 8

void switchPin(int pin, int status, int timeout) {
  digitalWrite(pin, status);
  delay(timeout);
}

void switchLine(int pins[], int status) {
  int i;
  for (i = 0; i < PINS_NUMBER; i++) {
    switchPin(pins[i], status, 200);
  }
}

int main(void){
  wiringPiSetup();

  int leds[PINS_NUMBER] = {0,1,2,3,4,5,6,7};

  int i;
  for (i = 0; i < PINS_NUMBER; i++) {
    pinMode(leds[i], OUTPUT);
  }

  while(1) {
    switchLine(leds, LOW);
    switchLine(leds, HIGH);
  }
  return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Do you intend to change the number of your pins? \$\endgroup\$ – Akira Nov 24 '17 at 7:45
  • \$\begingroup\$ There is a limited number of usable pins, so it might be increased but not significantly. \$\endgroup\$ – Iaroslav Karandashev Nov 24 '17 at 23:40
2
\$\begingroup\$

Naming

For first, your code looks good, it's not so hard to understand.

It works, but I feel that it is not perfect.

There is no perfect code. A code is evolving, a code is changing. So, instead of chasing perfection, you should pursue such things like readability.

Your function names are a bit confusing. For example without knowing the body of the switchLine function it's hard to firgure out that it will change the state of several pins sequentially with some delay.

How about a main() function like the following?

int main() {
    struct PinLedMap connections = {{
        { 0, LED_0 }, { 1, LED_1 }, { 2, LED_2 }, { 3, LED_3 },
        { 4, LED_4 }, { 5, LED_5 }, { 6, LED_6 }, { 7, LED_7 }
    }};
    assignPinsToLeds(&connections);
    initialize();

    setDelayOfSequentialOperations(250);
    while(1) {
        turnLedsOnSequentially();
        turnLedsOffSequentially();
    }
    return 0;
}

I think that the code above is clear enough, and easy to understand.

Responsibility

I don't see the reason why do the switchPin function applies the delay? The switchPin function should switch the state of specified pin only. Applying delays between turning LEDs on or off is the responsibility of your switchLine function.

In my example the turnLedsOnSequentially and the turnLedsOffSequentially functions work as follows:

void setLedsSequentiallyTo(enum LedState state) {
    int index;
    for(index = 0; index < NUMBER_OF_LEDS; ++index) {
        setLedTo(index, state);
        delay(_delayInMilliseconds);
    }
}    
void turnLedsOnSequentially() {
    setLedsSequentiallyTo(LED_ON);
}    
void turnLedsOffSequentially() {
    setLedsSequentiallyTo(LED_OFF);
}

Complete code

I took my example into pieces, so it contains a header file (led_control.h) and two source files (led_control.c, main.c). I've presented the main.c before, so I won't present it again.

led_control.h

#ifndef LED_CONTROL_H
#define LED_CONTROL_H

enum LedNumber {
    LED_0, LED_1, LED_2, LED_3, LED_4, LED_5, LED_6, LED_7,
    NUMBER_OF_LEDS
};
struct PinLedPair {
    int pinNumber;
    enum LedNumber ledNumber;
};
struct PinLedMap {
    struct PinLedPair elements[NUMBER_OF_LEDS];
};

void assignPinToLed(struct PinLedPair* connection);
void assignPinsToLeds(struct PinLedMap* connections);
void initialize();
void setDelayOfSequentialOperations(int delayInMilliseconds);
void turnLedOn(enum LedNumber ledNumber);
void turnLedOff(enum LedNumber ledNumber);
void turnLedsOnSequentially();
void turnLedsOffSequentially();

#endif /* LED_CONTROL_H */

led_control.c

#include "led_control.h"
#include <wiringPi.h>
#include <assert.h>

int _leds[NUMBER_OF_LEDS] = { 0 };
int _delayInMilliseconds = 100;

enum LedState {
    LED_OFF = LOW, LED_ON = HIGH
};

void assignPinToLed(struct PinLedPair* connection) {
    assert(connection);
    _leds[connection->ledNumber] = connection->pinNumber;
    pinMode(connection->pinNumber, OUTPUT);
}
void assignPinsToLeds(struct PinLedMap* connections) {
    int index;
    assert(connections);
    for(index = 0; index < NUMBER_OF_LEDS; ++index)
        assignPinToLed(&connections->elements[index]);
}

void initialize() {
    wiringPiSetup();
}
void setDelayOfSequentialOperations(int delayInMilliseconds) {
    _delayInMilliseconds = delayInMilliseconds;
}

void _setLedTo(enum LedNumber ledNumber, enum LedState state) {
    digitalWrite(_leds[ledNumber], state);
}
void turnLedOn(enum LedNumber ledNumber) {
    _setLedTo(ledNumber, LED_ON);
}
void turnLedOff(enum LedNumber ledNumber) {
    _setLedTo(ledNumber, LED_OFF);
}

void _setLedsSequentiallyTo(enum LedState state) {
    int index;
    for(index = 0; index < NUMBER_OF_LEDS; ++index) {
        _setLedTo(index, state);
        delay(_delayInMilliseconds);
    }
}
void turnLedsOnSequentially() {
    _setLedsSequentiallyTo(LED_ON);
}
void turnLedsOffSequentially() {
    _setLedsSequentiallyTo(LED_OFF);
}
| improve this answer | |
\$\endgroup\$

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.