Clean up gpio code.
- Break out the input and output components into separate
subclasses.
- Fix some styleguide issues, including adding more useful
comments.
- Make stuff return bool instead of int like it should.
- Make the API simpler.
- Make failure to export or set pin direction FATAL.
(The rational for this is in the comments.)
It compiles, but remains untested.
diff --git a/bbb_cape/src/bbb/bbb.gyp b/bbb_cape/src/bbb/bbb.gyp
index e960d7c..96e8711 100644
--- a/bbb_cape/src/bbb/bbb.gyp
+++ b/bbb_cape/src/bbb/bbb.gyp
@@ -67,6 +67,8 @@
],
'sources': [
'gpios.cc',
+ 'gpi.cc',
+ 'gpo.cc',
],
},
],
diff --git a/bbb_cape/src/bbb/gpi.cc b/bbb_cape/src/bbb/gpi.cc
new file mode 100644
index 0000000..9045eca
--- /dev/null
+++ b/bbb_cape/src/bbb/gpi.cc
@@ -0,0 +1,48 @@
+#include "gpi.h"
+
+#include <stdio.h>
+
+#include "aos/common/logging/logging.h"
+
+namespace bbb {
+
+Gpi::Gpi(int bank, int pin) {
+ // All the failures here are fatal, seeing as
+ // if we can't initialize the pin, we're
+ // probably screwed anyway.
+ if (!InitPin(bank, pin)) {
+ LOG(FATAL, "Failed to export pin.\n");
+ }
+ if (!DoPinDirSet(1)) {
+ LOG(FATAL, "Failed to set pin as an input.\n");
+ }
+}
+
+int Gpi::Read() {
+ // NOTE: I can't find any docs confirming that one can indeed
+ // poll a pin's value using this method, but it seems that it
+ // should work. Really, the "right" (interrupt driven) way to
+ // do this is to set an event, but that involves messing with
+ // dev tree crap, which I'm not willing to do unless we need
+ // this functionality.
+ char buf, val_path[64];
+ snprintf(val_path, sizeof(val_path), "/sys/class/gpio/gpio%d/value",
+ kernel_pin_);
+
+ if ((handle_ = fopen(val_path, "rb")) == NULL) {
+ LOG(ERROR, "Unable to open file for pin value reading.\n");
+ fclose(handle_);
+ return -1;
+ }
+
+ if (fread(&buf, sizeof(char), 1, handle_) <= 1) {
+ LOG(ERROR, "Reading from file failed with error %d\n",
+ ferror(handle_));
+ fclose(handle_);
+ return -1;
+ }
+ fclose(handle_);
+ return atoi(&buf);
+}
+
+} // namespace bbb
diff --git a/bbb_cape/src/bbb/gpi.h b/bbb_cape/src/bbb/gpi.h
new file mode 100644
index 0000000..d63b9dc
--- /dev/null
+++ b/bbb_cape/src/bbb/gpi.h
@@ -0,0 +1,23 @@
+#ifndef BBB_CAPE_SRC_BBB_GPI_H_
+#define BBB_CAPE_SRC_BBB_GPI_H_
+
+#include "gpios.h"
+
+// Subclass of Pin for input pins.
+
+namespace bbb {
+
+class Gpi : public Pin {
+ public:
+ // See the base class for what these args mean.
+ Gpi(int bank, int pin);
+
+ // Read the current value of the pin.
+ // Returns 1 if it reads high, 0 for low.
+ // Returns -1 if fails to read the pin for some reason.
+ int Read();
+};
+
+} // namespace bbb
+
+#endif
diff --git a/bbb_cape/src/bbb/gpios.cc b/bbb_cape/src/bbb/gpios.cc
index 5a9c76e..4f9dce3 100644
--- a/bbb_cape/src/bbb/gpios.cc
+++ b/bbb_cape/src/bbb/gpios.cc
@@ -7,147 +7,60 @@
namespace bbb {
-Pin::Pin(int bank, int pin)
- : handle_(NULL),
- direction_(0),
- kernel_pin_(bank * 32 + pin),
- exported_(false) {}
-
Pin::~Pin() {
// Unexport the pin.
if ((handle_ = fopen("/sys/class/gpio/unexport", "ab")) == NULL) {
- LOG(WARNING, "Could not unexport gpio pin.\n");
+ LOG(WARNING, "Could not open file to unexport pin.\n");
// There's nothing intelligent we can really do here.
return;
}
- char gpio_num[2];
- snprintf(gpio_num, sizeof(gpio_num), "%d", kernel_pin_);
- fwrite(gpio_num, sizeof(char), 2, handle_);
+ fprintf(handle_, "%d", kernel_pin_);
fclose(handle_);
}
-int Pin::DoExport() {
- char gpio_num[2];
- snprintf(gpio_num, sizeof(gpio_num), "%d", kernel_pin_);
+bool Pin::InitPin(int bank, int pin) {
+ kernel_pin_ = bank * 32 + pin;
// Export the pin.
if ((handle_ = fopen("/sys/class/gpio/export", "ab")) == NULL) {
- LOG(ERROR, "Could not export GPIO pin.\n");
- return -1;
+ LOG(ERROR, "Could not open file for exporting pin.\n");
+ return false;
}
- fwrite(gpio_num, sizeof(char), 2, handle_);
+ fprintf(handle_, "%d", kernel_pin_);
fclose(handle_);
-
- exported_ = true;
- return 0;
+ return true;
}
-int Pin::DoPinDirSet(int direction) {
- char buf[4], type_path[64];
+bool Pin::DoPinDirSet(int direction) {
+ char type_path[64];
snprintf(type_path, sizeof(type_path), "/sys/class/gpio/gpio%d/direction",
kernel_pin_);
if ((handle_ = fopen(type_path, "rb+")) == NULL) {
- LOG(ERROR, "Unable to set pin direction.\n");
- return -1;
+ LOG(ERROR, "Unable open file for pin direction setting.\n");
+ return false;
}
- direction_ = direction;
switch (direction) {
case 1:
- strcpy(buf, "in");
+ // Input.
+ fprintf(handle_, "in");
break;
case 2:
- strcpy(buf, "low");
+ // Output.
+ // If it's an output, we should specify an initial direction.
+ fprintf(handle_, "low");
break;
- case 0:
- return 0;
default:
LOG(ERROR, "Invalid direction identifier %d.\n", direction);
- direction_ = 0;
- return -1;
+ fclose(handle_);
+ return false;
}
- fwrite(buf, sizeof(char), 3, handle_);
+
fclose(handle_);
-
- return 0;
-}
-
-int Pin::MakeInput() {
- if (!exported_) {
- if (DoExport()) {
- return -1;
- }
- }
-
- return DoPinDirSet(1);
-}
-
-int Pin::MakeOutput() {
- if (!exported_) {
- if (DoExport()) {
- return -1;
- }
- }
-
- return DoPinDirSet(2);
-}
-
-int Pin::Write(uint8_t value) {
- if (direction_ != 2) {
- LOG(ERROR, "Only pins set as output can be written to.\n");
- return -1;
- }
-
- char buf, val_path[64];
- snprintf(val_path, sizeof(val_path), "/sys/class/gpio/gpio%d/value",
- kernel_pin_);
- if (value != 0) {
- value = 1;
- }
-
- if ((handle_ = fopen(val_path, "rb+")) == NULL) {
- LOG(ERROR, "Unable to set pin value.\n");
- return -1;
- }
-
- snprintf(&buf, sizeof(buf), "%d", value);
- fwrite(&buf, sizeof(char), 1, handle_);
- fclose(handle_);
-
- return 0;
-}
-
-int Pin::Read() {
- // NOTE: I can't find any docs confirming that one can indeed
- // poll a pin's value using this method, but it seems that it
- // should work. Really, the "right" (interrupt driven) way to
- // do this is to set an event, but that involves messing with
- // dev tree crap, which I'm not willing to do unless we need
- // this functionality.
-
- if (direction_ != 1) {
- LOG(ERROR, "Only pins set as input can be read from.\n");
- return -1;
- }
-
- char buf, val_path[64];
- snprintf(val_path, sizeof(val_path), "/sys/class/gpio/gpio%d/value",
- kernel_pin_);
-
- if ((handle_ = fopen(val_path, "rb")) == NULL) {
- LOG(ERROR, "Unable to read pin value.\n");
- return -1;
- }
-
- if (fread(&buf, sizeof(char), 1, handle_) <= 0) {
- LOG(ERROR, "Failed to read pin value from file.\n");
- return -1;
- }
- fclose(handle_);
- return atoi(&buf);
+ return true;
}
} // namespace bbb
diff --git a/bbb_cape/src/bbb/gpios.h b/bbb_cape/src/bbb/gpios.h
index a377d68..ba5da00 100644
--- a/bbb_cape/src/bbb/gpios.h
+++ b/bbb_cape/src/bbb/gpios.h
@@ -1,7 +1,6 @@
#ifndef BBB_CAPE_SRC_BBB_CAPE_CONTROL_H_
#define BBB_CAPE_SRC_BBB_CAPE_CONTROL_H_
-#include <stdint.h>
#include <stdio.h>
// As it turns out, controlling the BBB's GPIO pins
@@ -9,30 +8,31 @@
// code is to provide a simple wrapper that masks
// all the ugly stuff and exposes a nice API.
-// Based on example from
+// Based on example from
// <http://learnbuildshare.wordpress.com/2013/05/29/beaglebone-black-digital-ouput/>
+// This is a base class for all gpio related stuff.
+// Use either a gpi or gpo subclass if you want to do things.
namespace bbb {
class Pin {
- FILE *handle_;
- int direction_;
- int kernel_pin_;
- bool exported_;
+ public:
+ // Not strictly necessary for this to be virtual,
+ // but it's a good idea.
+ virtual ~Pin();
- int DoPinDirSet(int direction);
- int DoExport();
-
-public:
+ protected:
+ // Set the pin direction.
+ // 1 makes it an input.
+ // 2 makes it an output and sets the initial state to low.
+ bool DoPinDirSet(int direction);
+ // Export the pin, so we can use it.
// Here, for example, if you wanted to use
- // GPIO1_28, you would give the ctor
- // 1, 28 as arguments.
- Pin(int bank, int pin);
- ~Pin();
- int MakeInput();
- int MakeOutput();
- int Write(uint8_t value);
- int Read();
+ // GPIO1_28, you would give it 1, 28 as arguments.
+ bool InitPin(int bank, int pin);
+
+ FILE *handle_ = NULL;
+ int kernel_pin_ = -1;
};
} // namespace bbb
diff --git a/bbb_cape/src/bbb/gpo.cc b/bbb_cape/src/bbb/gpo.cc
new file mode 100644
index 0000000..5926a7c
--- /dev/null
+++ b/bbb_cape/src/bbb/gpo.cc
@@ -0,0 +1,37 @@
+#include "gpo.h"
+
+#include <stdio.h>
+
+#include "aos/common/logging/logging.h"
+
+namespace bbb {
+
+Gpo::Gpo(int bank, int pin) {
+ // All the failures here are fatal, seeing as
+ // if we can't initialize the pin, we're probably
+ // screwed anyway.
+ if (!InitPin(bank, pin)) {
+ LOG(FATAL, "Failed to export the pin.\n");
+ }
+ if (!DoPinDirSet(2)) {
+ LOG(FATAL, "Failed to make the pin an output.\n");
+ }
+}
+
+bool Gpo::DoSet(const int value) {
+ char val_path[64];
+ snprintf(val_path, sizeof(val_path), "/sys/class/gpio/gpio%d/value",
+ kernel_pin_);
+
+ if ((handle_ = fopen(val_path, "rb+")) == NULL) {
+ LOG(ERROR, "Unable open file for pin value setting.\n");
+ return false;
+ }
+
+ fprintf(handle_, "%d", value);
+ fclose(handle_);
+
+ return true;
+}
+
+} // namespace bbb
diff --git a/bbb_cape/src/bbb/gpo.h b/bbb_cape/src/bbb/gpo.h
new file mode 100644
index 0000000..3d7b2ce
--- /dev/null
+++ b/bbb_cape/src/bbb/gpo.h
@@ -0,0 +1,29 @@
+#ifndef BBB_CAPE_SRC_BBB_GPO_H_
+#define BBB_CAPE_SRC_BBB_GPO_H_
+
+#include "gpios.h"
+
+namespace bbb {
+
+// Gpios subclass for output pins.
+class Gpo : public Pin {
+ public:
+ // See the base class for what these args mean.
+ Gpo(int bank, int pin);
+
+ // Methods set the pin to read either high or low.
+ inline bool SetHigh() {
+ return DoSet(1);
+ }
+ inline bool SetLow() {
+ return DoSet(0);
+ }
+
+ private:
+ // Does the actual pin setting.
+ bool DoSet(const int value);
+};
+
+} // namespace bbb
+
+#endif
diff --git a/bbb_cape/src/bbb/uart_reader_main.cc b/bbb_cape/src/bbb/uart_reader_main.cc
index 5bda02c..d42f319 100644
--- a/bbb_cape/src/bbb/uart_reader_main.cc
+++ b/bbb_cape/src/bbb/uart_reader_main.cc
@@ -5,7 +5,7 @@
#include "aos/atom_code/init.h"
#include "aos/common/logging/logging_impl.h"
#include "aos/common/time.h"
-#include "bbb/gpios.h"
+#include "bbb/gpo.h"
#include "bbb/uart_reader.h"
using ::aos::time::Time;
@@ -20,8 +20,7 @@
// the board.
static const Time kPacketTimeout = Time::InSeconds(1);
- ::bbb::Pin reset_pin = bbb::Pin(1, 6);
- reset_pin.MakeOutput();
+ ::bbb::Gpo reset_pin = bbb::Gpo(1, 6);
#endif
::bbb::UartReader receiver(1500000);
@@ -41,9 +40,9 @@
#if DO_RESET
if (!last_packet_time.IsWithin(Time::Now(), kPacketTimeout.ToNSec())) {
LOG(ERROR, "No good packets for too long. Resetting cape.\n");
- reset_pin.Write(1);
+ reset_pin.SetHigh();
::aos::time::SleepFor(Time::InSeconds(1));
- reset_pin.Write(0);
+ reset_pin.SetLow();
last_packet_time = Time::Now();
}