3
\$\begingroup\$

I've received reviews on the last version here, but as an update would make the answers not fitting anymore, I think it is wise to open a new post.

I followed most of the hints of the last review and now want to check if I done things write or made new mistakes.

If you are interested in the complete source click here

gpio.h

#ifndef SRC_GPIO_H_
#define SRC_GPIO_H_

#include <string>
#include <fstream>
#include <iostream>

using std::string;
using std::fstream;

enum GPIODirection {
    GPIO_IN  = 0,
    GPIO_OUT = 1
};

enum GPIOValue {
    GPIO_LOW  = 0,
    GPIO_HIGH = 1
};

class GPIO {
    public:
        explicit GPIO(int id);
        ~GPIO();

        int  Value();
        void Value(int value);
        int  Direction();
        void Direction(int value);

    private:
        int id_;

        fstream value_;
        fstream direction_;

        bool Exists();
        void Export();
        void Unexport();

        static const string PATH_EXPORT;
        static const string PATH_UNEXPORT;
        static const string PREFIX;
        static const string POSTFIX_VALUE;
        static const string POSTFIX_DIRECTION;
};

#endif

gpio.cc

#include "gpio.h"

#include <string>
#include <sstream>
#include <fstream>
#include <iostream>
#include <exception>
#include <stdexcept>

using std::ios;
using std::endl;
using std::string;
using std::stringstream;
using std::logic_error;
using std::runtime_error;

const string GPIO::PATH_EXPORT       = "/sys/class/gpio/export";
const string GPIO::PATH_UNEXPORT     = "/sys/class/gpio/unexport";
const string GPIO::PREFIX            = "/sys/class/gpio/gpio";
const string GPIO::POSTFIX_VALUE     = "/value";
const string GPIO::POSTFIX_DIRECTION = "/direction";

GPIO::GPIO(int id) {
    id_ = id;

    Export();

    stringstream value_path;
    stringstream direction_path;

    value_path << PREFIX;
    value_path << id;
    value_path << POSTFIX_VALUE;

    direction_path << PREFIX;
    direction_path << id;
    direction_path << POSTFIX_DIRECTION;

    value_.open(value_path.str().c_str());
    direction_.open(direction_path.str().c_str());
}

GPIO::~GPIO() {
    value_.close();
    direction_.close();

    Unexport();
}

bool
GPIO::Exists() {
    stringstream path;

    path << PREFIX;
    path << id_;

    fstream gpio;

    gpio.open(path.str().c_str());

    bool result = gpio.good();

    gpio.close();

    return result;
}

void
GPIO::Export() {
    if (Exists()) return;

    fstream gpio_export;
    stringstream string_stream;

    string_stream << id_;

    gpio_export.open(PATH_EXPORT.c_str(), ios::out);
    gpio_export << string_stream.str();
    gpio_export.close();
}

void
GPIO::Unexport() {
    if (!Exists()) return;

    fstream gpio_unexport;
    stringstream string_stream;

    string_stream << id_;

    gpio_unexport.open(PATH_UNEXPORT.c_str(), ios::out);
    gpio_unexport << string_stream.str();
    gpio_unexport.close();
}

int
GPIO::Value() {
    string value;

    value_.seekp(0);
    value_ >> value;

    if (value == "0") return GPIO_LOW;
    if (value == "1") return GPIO_HIGH;

    throw logic_error("Invalid GPIO value.");
}

void
GPIO::Value(int value) {
    value_.seekp(0);

    switch (value) {
        case GPIO_LOW:
            value_ << "0" << endl; 

            if (!value_.good())
                throw runtime_error("Error writting to value file stream.");

            break;
        case GPIO_HIGH:
            value_ << "1" << endl; 

            if (!value_.good())
                throw runtime_error("Error writting to value file stream.");

            break;
        default:
            throw logic_error("Error cannot set invalid GPIO value.");
    }
}

int
GPIO::Direction() {
    string direction;

    direction_.seekp(0);

    direction_ >> direction;

    if (direction == "in") return GPIO_IN;
    if (direction == "out") return GPIO_OUT;

    throw logic_error("Invalid GPIO direction.");
}

void
GPIO::Direction(int value) {
    direction_.seekp(0);

    switch (value) {
        case GPIO_IN:
            direction_ << "in" << endl;

            if (!direction_.good())
                throw runtime_error("Error writting to direciton file stream.");

            break;
        case GPIO_OUT:
            direction_ << "out" << endl;

            if (!direction_.good())
                throw runtime_error("Error writting to direciton file stream.");

            break;
        default:
            throw logic_error("Error cannot set invalid GPIO direction.");
    }
}

Things I guess could be done better:

  • class methods
  • implementation of Exist()
  • error handling
  • string formatting
  • path constants
  • int to string cast (std::to_string seems not to be available)
\$\endgroup\$
4
\$\begingroup\$

Some quick remarks:

  • To me, value and direction don't sound like stream names. Maybe they should be called something else? It depends on the domain terminology (which I'm not familiar with).

  • Don't ever pollute the global namespace by using using in headers. Write the full qualifiers.

  • Your stringstream operations look like they can be factored out into a separate function.

  • Note that std::endl forces a stream flush. If you just want the newline, use << '\n' instead.

  • I think it's nice to sort #includes alphabetically.

  • In C++11, fstream::open takes a std::string argument as well.

  • Consider changing the names of Value and Direction (like William Morris points out in the other review). If a function changes the state of the object, it should be named differently (probably with a verb).

  • Prefer to initialize variables using initializer lists in constructors.

  • Your Value and Direction functions are nearly identical.

You can refactor to something like this:

std::string GPIO::ReadFromBeginningOfStream(std::fstream& stream)
{
    stream.seekg(0); // NOTE: seekg, *not* seekp
    std::string tmp;
    stream >> tmp;
    return tmp;
}

int
GPIO::Value() {
    std::string const& value = ReadFromBeginningOfStream(value_);

    if (value == "0") return GPIO_LOW;
    if (value == "1") return GPIO_HIGH;

    throw logic_error("Invalid GPIO value.");
}

And then similar but with passing in the other stream in Direction().

NOTE: Seek in an input stream by calling seekg(); in an output stream by calling seekp().

  • Exists() should be const, like this: bool Exists() const;
\$\endgroup\$
  • \$\begingroup\$ What do you mean with the "using" headers? Should I just always use std::string or v8::Object? I renamed the functions to SetValue, GetValue, ... I also need "<< std::endl" else the value is not written (tried this already) Also I get a compiler error on settings Exists() const \$\endgroup\$ – bodokaiser Jul 14 '13 at 16:49
  • 1
    \$\begingroup\$ You can use using in implementation files (preferably inside each function), but not in headers. If you need flushing, then use endl like you are, I just mentioned it :-) Which error do you get if you set Exists() to const? \$\endgroup\$ – Lstor Jul 14 '13 at 17:02
  • \$\begingroup\$ (You added the const to Exists() in both the declaration and definition, right?) \$\endgroup\$ – Lstor Jul 14 '13 at 17:58
  • \$\begingroup\$ When I just put bool Exists() const to the header the compiler says that Exists() does not match any declaration \$\endgroup\$ – bodokaiser Jul 14 '13 at 17:59
  • \$\begingroup\$ ah you have to place the const in the implementation between Exists() and opening bracket - did not knew this \$\endgroup\$ – bodokaiser Jul 14 '13 at 18:01

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.