Abstraction for an IO device

Question

I have a C++ class which acts as an abstraction for an IO device which is controlled through file descriptors. As I am very new to C++, I would be glad if you can give me devastating feedback on things I've done wrong.

This especially covers:

• Code style
• Design patterns
• Error handling
• Things which are open for failures
• C++ approaches which can be used over C approaches (file IO)

gpio.h

#ifndef SRC_GPIO_H_
#define SRC_GPIO_H_
#define GPIO_IN 0
#define GPIO_OUT 1
#define GPIO_LOW 0
#define 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;
 int _value_fd;
 int _direction_fd;
 bool Exists();
 void Export();
 void Unexport();
 void OpenValueFd();
 void OpenDirectionFd();
 void CloseValueFd();
 void CloseDirectionFd();
 void SeekToTopOfValueFd();
 void SeekToTopOfDirectionFd();
};
#endif

gpio.cc

#include "../src/gpio.h"
#define GPIO_PATH_EXPORT "/sys/class/gpio/export"
#define GPIO_PATH_UNEXPORT "/sys/class/gpio/unexport"
#define GPIO_PATH_DIRECTORY "/sys/class/gpio/gpio%d"
#define GPIO_PATH_DIRECTION "/sys/class/gpio/gpio%d/direction"
#define GPIO_PATH_VALUE "/sys/class/gpio/gpio%d/value"
GPIO::GPIO(int id) {
 Export();
 OpenValueFd();
 OpenDirectionFd();
}
GPIO::~GPIO() {
 CloseValueFd();
 CloseDirectionFd();
 Unexport();
}
bool
GPIO::Exists() {
 char * path;
 if (asprintf(&path, GPIO_PATH_DIRECTORY, id) < 0)
 throw "Error generationg GPIO directory path.";
 int result = access(path, F_OK);
 free(path);
 return static_cast<bool> result++;
}
void
GPIO::Export() {
 char * id;
 if (Exists()) return;
 if (asprintf(&id, "%d", _id) < 0)
 throw "Error converting id to char.";
 int fd = open(GPIO_PATH_EXPORT, O_WRONLY);
 if (fd < 0)
 throw "Error opening GPIO export.";
 if (write(fd, id, strlen(id)) < 0)
 throw "Error writing to GPIO export.";
 if (close(fd) < 0)
 throw "Error closing GPIO export.";
 free(id);
}
void
GPIO::Unexport() {
 char * id;
 if (!Exists()) return;
 if (asprintf(&id, "%d", _id) < 0)
 throw "Error converting id to char.";
 int fd = open(GPIO_PATH_UNEXPORT, O_WRONLY);
 if (fd < 0)
 throw "Error opening GPIO unexport.";
 if (write(fd, id, strlen(id)) < 0)
 throw "Error writing to GPIO unexport.";
 if (close(fd) < 0)
 throw "Error closing GPIO unexport.";
 free(id);
}
int
GPIO::Value() {
 char data[2];
 SeekValueFdToTop();
 data[1] = 0;
 if (read(_value_fd, data, 1) < 0)
 throw "Error reading from GPIO value.";
 if (strncmp(data, "0", 1) == 0)
 return GPIO_LOW;
 if (strncmp(data, "1", 1) == 0)
 return GPIO_HIGH;
 throw "Invalid GPIO value.";
}
void
GPIO::Value(int value) {
 SeekValueFdToTop();
 switch (value) {
 case GPIO_LOW:
 if (write(_value_fd, "0\n", 2) < 0)
 throw "Error writing to GPIO value.";
 break;
 case GPIO_HIGH:
 if (write(_value_fd, "1\n", 2) < 0)
 throw "Error writing to GPIO value.";
 break;
 default:
 throw "Error cannot set invalid GPIO value.";
 }
}
int
GPIO::Direction() {
 char data[4];
 SeekValueFdToTop();
 data[3] = 0;
 if (read(_direction_fd, data, 1) < 0)
 throw "Error reading from GPIO direction.";
 if (strncmp(data, "in", 2) == 0)
 return GPIO_IN;
 if (strncmp(data, "out", 3) == 0)
 return GPIO_OUT;
 throw "Invalid GPIO direction.";
}
void
GPIO::Direction(int value) {
 SeekValueFdToTop();
 switch (value) {
 case GPIO_IN:
 if (write(_value_fd, "in\n", 3) < 0)
 throw "Error writing to GPIO direction.";
 break;
 case GPIO_OUT:
 if (write(_value_fd, "out\n", 4) < 0)
 throw "Error writing to GPIO direction.";
 break;
 default:
 throw "Error cannot set invalid GPIO direction.";
 }
}
void
GPIO::OpenValueFd() {
 char * path;
 if (asprintf(&path, GPIO_PATH_VALUE, _id) < 0)
 throw "Error generating GPIO value path.";
 if ((_value_fd = open(path, O_RDWR)) < 0)
 throw "Error opening GPIO value.";
 free(path);
}
void
GPIO::OpenDirectionFd() {
 char * path;
 if (asprintf(&path, GPIO_PATH_DIRECTION, _id) < 0)
 throw "Error generating GPIO direction path.";
 if ((_direction_fd = open(path, O_RDWR)) < 0)
 throw "Error opening GPIO direction.";
 free(path);
}
void
GPIO::CloseValueFd() {
 if (close(_value_fd) < 0)
 throw "Error closing GPIO value.";
}
void
GPIO::CloseDirectionFd() {
 if (close(_direction_fd) < 0)
 throw "Error closing GPIO direction.";
}
void
GPIO::SeekToTopOfValueFd() {
 if (lseek(_value_fd, SEEK_SET, 0) < 0)
 throw "Error seeking to top of GPIO value.";
}
void
GPIO::SeekToTopOfDirectionFd() {
 if (lseek(_direction_fd, SEEK_SET, 0) < 0)
 throw "Error seeking to top of GPIO direction.";
}

Source can be found here

Answer 1

Prefer the C++ way

As you seem to expect, one of the key issues with your code is that it's not very C++y.

1. Prefer std::string over C-style strings.

std::string is better in almost every way. It takes care of its own memory and grows when needed. The speed is usually comparable to C-style strings (sometimes faster, sometimes slower). The code written using string is often more readable. For example, compare your code:

if (strncmp(data, "in", 2) == 0)

with the std::string variant:

if (data == "in")

2. Prefer iostream to FILE*.

The C++ library for IO is typesafe, extensible, object-oriented and should be preferred. Use an ifstream to read from a file and ofstream to write, or fstream for both.

3. Consider throwing exceptions derived from std::runtime_error and std::logic_error

By having an exception hierarchy, you can have fine grained control over which errors to catch where. Your code will also play nice with others, who might expect your exceptions to behave like the standard ones. Want to catch everything? Catch std::exception. Want to catch only your own? Do so. Want to get the error message? Call .what().

Remember that exceptions should always be thrown by value, and catched (caught) by reference(-to-const).

4. Prefer enum or consts to #defined constants.

Instead of

#define GPIO_IN 0
#define GPIO_OUT 1

write

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

For your path name #defines, use const std::string instead. (And consider moving them into class scope as static, or into a namespace.)

You get type safety, easier debugging, normal scope rules; the works.

5. Avoid leading underscores in identifiers.

The following identifiers are reserved:

• Any name that begins with an underscore and is in the global namespace.
• Any name that begins with an underscore followed by a capital letter.
• Any name that contains two consecutive underscores.

While your _id and similar variables are technically correct, don't rely on yourself or others memorizing these rules. Play it safe and move the underscore to the other side: id_, or drop it altogether.

Other things

These are mostly nitpicking, but you asked for a "very critical review" :-)

6. #include the proper headers.

I'm guessing you already are, and that maybe they were removed when posting the code for review. I'm mentioning it just in case. It's nice to include #includes in the review code, too.

7. Let the build system take care of include paths.

Instead of: #include "../src/gpio.h", prefer to just use #include "gpio.h" and let the preprocessor and build system take care of finding the right header for you. This makes it a lot easier to move files at a later point.

8. Don't cast unless you have to.

Instead of

return static_cast<bool> result++;

write

return result++ != 0;

(And use parenthesis on your casts.)

9. Use fclose() instead of close().

The C library function is called fclose(). (This is a moot point, however, since you should prefer C++ IO anyway.)

10. Use descriptive names and a clear interface

Your public interface functions have names that imply that they don't change the state of the class. If they don't, make the functions const. (Your Exists() function should at least be const. The same may apply to functions that are only reading.) Consider separating the state changing functionality from the non-state-changing functionality, and putting the latter into const functions.

Rename functions that do change state to reflect that.

Finally, data is a classic, horrible variable name. I'm sure you can do better!

Answer 2

A few points to add to those from @Lstor.

Firstly I'm puzzled about the underlying GPIO devices - why use character strings in the interface to a GPIO?

And quite fundamentally, you don't seem to have compiled your code.

• GPIO::GPIO(): id is not used (to set the class variable)
• GPIO::Exists(): uses id not _id
• GPIO::Exists(): return statement is missing brackets.
• SeekValueFdToTop des not exist (called in Value and Direction functions)

---

In GPIO::Exists() the result++ gets it wrong in two ways:

• firstly it is horrible to rely on the -1/0 return from access to to be turned into 0/1 by incrementing

• secondly you use post-increment, so it doesn't happen.

You should be using

return result == 0; // returns `true` if file exists

---

You use asprintf everywhere. I had to look that up and find it is a non-standard function that allocates the necessary string. Using it to create paths on-demand seems unnecessary. You could create the base path just once in the constructor. But I'm a C-guy and all that allocation/deallocation is foreign to me. I guess C++ strings will be doing that in the background if you use strings instead...

---

You should use brackets, even when not strictly needed:

if (asprintf(&path, GPIO_PATH_VALUE, _id) < 0) {
 throw "Error generating GPIO value path.";
}

---

Your Value and Direction functions have various issues.

• Using strncmp in Value to check for "0" and "1" is not sensible. Instead, use

  if (data[0] == '0') {...} 
  

  although data does not need to be an array when reading a single char.

• I think you should have separate get/set functions rather than overloading Value and Direction to mean either get or set.

• You use non-existent SeekValueFdToTop in both the Value and Direction functions. The appropriate functions are SeekToTopOfValueFd and SeekToTopOfDirectionFd.