Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions plugins/gpio/GPIODriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <sys/types.h>
#include <unistd.h>

#include <algorithm>
#include <sstream>
#include <string>
#include <vector>
Expand All @@ -43,6 +44,7 @@ namespace gpio {
const char GPIODriver::GPIO_BASE_DIR[] = "/sys/class/gpio/gpio";

using ola::thread::MutexLocker;
using std::find;
using std::string;
using std::vector;

Expand Down Expand Up @@ -122,7 +124,9 @@ bool GPIODriver::SetupGPIO() {
* echo N > /sys/class/gpio/export
* That requires root access.
*/
const string direction("out");
const string normal_direction("low");
const string inverted_direction("high");
const string* direction = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be const as we're changing it.

bool failed = false;
vector<uint16_t>::const_iterator iter = m_options.gpio_pins.begin();
for (; iter != m_options.gpio_pins.end(); ++iter) {
Expand All @@ -134,7 +138,12 @@ bool GPIODriver::SetupGPIO() {
break;
}

GPIOPin pin = {pin_fd, UNDEFINED, false};
// Check if pin is in the inverted pin list
bool inverted = (find(m_options.gpio_inverted_pins.begin(),
m_options.gpio_inverted_pins.end(),
(*iter)) != m_options.gpio_inverted_pins.end());
Comment on lines +142 to +144
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


GPIOPin pin = {pin_fd, UNDEFINED, false, inverted};

// Set dir
str.str("");
Expand All @@ -144,9 +153,19 @@ bool GPIODriver::SetupGPIO() {
failed = true;
break;
}
if (write(fd, direction.c_str(), direction.size()) < 0) {
OLA_WARN << "Failed to enable output on " << str.str() << " : "
<< strerror(errno);
// Assign correct initial state
if (inverted) {
direction = &inverted_direction;
} else {
direction = &normal_direction;
}

OLA_DEBUG << "Configuring GPIO pin " << static_cast<int>(*iter)
<< " with " << (inverted ? "inverted" : "normal") << " logic";
if (write(fd, direction->c_str(), direction->size()) < 0) {
OLA_WARN << "Failed to enable output on " << str.str()
<< " with " << (inverted ? "inverted" : "normal") << " logic"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is showing the full path to the pin, perhaps also show the direction string here, rather than the friendly inverted/normal output.

Or turn it on it's head and put that in the DEBUG now we've added it and just make the WARN more user friendly "Failed to enable output on pin n as high/low".

<< " : " << strerror(errno);
failed = true;
}
close(fd);
Expand Down Expand Up @@ -191,7 +210,14 @@ bool GPIODriver::UpdateGPIOPins(const DmxBuffer &dmx) {

// Change the pin state if required.
if (action != NO_CHANGE) {
char data = (action == TURN_ON ? '1' : '0');
char data;
bool state = (action == TURN_ON);
// Handle inverted logic appropriately
if (m_gpio_pins[i].inverted) {
state = !state;
}
// Convert to char and write to sysfs
data = (state ? '1' : '0');
if (write(m_gpio_pins[i].fd, &data, sizeof(data)) < 0) {
OLA_WARN << "Failed to toggle GPIO pin " << i << ", fd "
<< static_cast<int>(m_gpio_pins[i].fd) << ": "
Expand Down
6 changes: 6 additions & 0 deletions plugins/gpio/GPIODriver.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ class GPIODriver : private ola::thread::Thread {
*/
std::vector<uint16_t> gpio_pins;

/**
* @brief A list of gpio_pins which use inverted logic.
*/
std::vector<uint16_t> gpio_inverted_pins;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the variable name match the config name please, just to simplify things.


/**
* @brief The DMX512 start address of the first pin
*/
Expand Down Expand Up @@ -108,6 +113,7 @@ class GPIODriver : private ola::thread::Thread {
int fd;
GPIOState state;
bool last_value;
bool inverted;
};

typedef std::vector<GPIOPin> GPIOPins;
Expand Down
28 changes: 28 additions & 0 deletions plugins/gpio/GPIOPlugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "plugins/gpio/GPIOPlugin.h"

#include <algorithm>
#include <memory>
#include <string>
#include <vector>
Expand All @@ -35,10 +36,12 @@ namespace plugin {
namespace gpio {

using std::auto_ptr;
using std::find;
using std::string;
using std::vector;

const char GPIOPlugin::GPIO_PINS_KEY[] = "gpio_pins";
const char GPIOPlugin::GPIO_PINS_INVERTED_KEY[] = "gpio_pins_inverted";
const char GPIOPlugin::GPIO_SLOT_OFFSET_KEY[] = "gpio_slot_offset";
const char GPIOPlugin::GPIO_TURN_OFF_KEY[] = "gpio_turn_off";
const char GPIOPlugin::GPIO_TURN_ON_KEY[] = "gpio_turn_on";
Expand Down Expand Up @@ -87,6 +90,28 @@ bool GPIOPlugin::StartHook() {
options.gpio_pins.push_back(pin);
}

pin_list.clear();
StringSplit(m_preferences->GetValue(GPIO_PINS_INVERTED_KEY), &pin_list, ",");
iter = pin_list.begin();
for (; iter != pin_list.end(); ++iter) {
if (iter->empty()) {
continue;
}

uint16_t pin;
if (!StringToInt(*iter, &pin)) {
OLA_WARN << "Invalid value for GPIO pin to invert: " << *iter;
return false;
}

if (find(pin_list.begin(), pin_list.end(), (*iter)) == pin_list.end()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be options.gpio_pins? Aren't you just searching yourself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you're checking they're not duplicated, which might make sense and perhaps we should do on the main pin list too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intention was to check for entries listed in the list of pins to invert but not in the list of all GPIO pins that this plugin shall interact with as opposed to ignoring silently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intention was to check for entries listed in the list of pins to invert but not in the list of all GPIO pins that this plugin shall interact with as opposed to ignoring silently.

Yeah in which case I don't believe you're currently doing that @nkaminski . You're checking pin_list, which is the contents of GPIO_PINS_INVERTED_KEY for iter, which is an iterator across pin_list. One of the two needs to become options.gpio_pins, so you check each entry in pin_list exists in options.gpio_pins or vice versa.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still outstanding @nkaminski .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @nkaminski , still this please...

Also you can use our STLContains again I think:

Suggested change
if (find(pin_list.begin(), pin_list.end(), (*iter)) == pin_list.end()) {
if (!STLContains(options.gpio_pins, pin)) {

OLA_WARN << "Inverted pin " << (*iter) << " not found in GPIO pin list";
return false;
}

options.gpio_inverted_pins.push_back(pin);
}

if (options.gpio_pins.empty()) {
return true;
}
Expand Down Expand Up @@ -124,6 +149,9 @@ bool GPIOPlugin::SetDefaultPreferences() {
save |= m_preferences->SetDefaultValue(GPIO_PINS_KEY,
StringValidator(),
"");
save |= m_preferences->SetDefaultValue(GPIO_PINS_INVERTED_KEY,
StringValidator(),
"");
save |= m_preferences->SetDefaultValue(GPIO_SLOT_OFFSET_KEY,
UIntValidator(1, DMX_UNIVERSE_SIZE),
"1");
Expand Down
1 change: 1 addition & 0 deletions plugins/gpio/GPIOPlugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class GPIOPlugin: public ola::Plugin {
bool SetDefaultPreferences();

static const char GPIO_PINS_KEY[];
static const char GPIO_PINS_INVERTED_KEY[];
static const char GPIO_SLOT_OFFSET_KEY[];
static const char GPIO_TURN_OFF_KEY[];
static const char GPIO_TURN_ON_KEY[];
Expand Down
3 changes: 3 additions & 0 deletions plugins/gpio/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ The offset (start address) of the GPIO pins is configurable.
`gpio_pins = [int]`
The list of GPIO pins to control, each pin is mapped to a DMX512 slot.

`gpio_pins_inverted = [int]`
The list of GPIO pins listed under `gpio_pins` which use inverted logic.

`gpio_slot_offset = <int>`
The DMX512 slot for the first pin. Slots are indexed from 1.

Expand Down