GPIO wrapper class

Question

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)

• Loki Astari: Check this out for argument about explicitly calling close: codereview.stackexchange.com/q/540/507

Answer 1

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;

• bodokaiser: 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
• Lstor: 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?
• Lstor: (You added the const to Exists() in both the declaration and definition, right?)
• bodokaiser: When I just put bool Exists() const to the header the compiler says that Exists() does not match any declaration
• bodokaiser: ah you have to place the const in the implementation between Exists() and opening bracket - did not knew this