-
Notifications
You must be signed in to change notification settings - Fork 221
Add support for inverted (active low) logic in GPIO plugin #1587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| #include <sys/types.h> | ||
| #include <unistd.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <sstream> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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; | ||
| bool failed = false; | ||
| vector<uint16_t>::const_iterator iter = m_options.gpio_pins.begin(); | ||
| for (; iter != m_options.gpio_pins.end(); ++iter) { | ||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you use our STLContains function? |
||
|
|
||
| GPIOPin pin = {pin_fd, UNDEFINED, false, inverted}; | ||
|
|
||
| // Set dir | ||
| str.str(""); | ||
|
|
@@ -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) { | ||
nkaminski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| OLA_WARN << "Failed to enable output on " << str.str() | ||
| << " with " << (inverted ? "inverted" : "normal") << " logic" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
@@ -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; | ||
nkaminski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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) << ": " | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| */ | ||
|
|
@@ -108,6 +113,7 @@ class GPIODriver : private ola::thread::Thread { | |
| int fd; | ||
| GPIOState state; | ||
| bool last_value; | ||
| bool inverted; | ||
| }; | ||
|
|
||
| typedef std::vector<GPIOPin> GPIOPins; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |||||
|
|
||||||
| #include "plugins/gpio/GPIOPlugin.h" | ||||||
|
|
||||||
| #include <algorithm> | ||||||
| #include <memory> | ||||||
| #include <string> | ||||||
| #include <vector> | ||||||
|
|
@@ -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"; | ||||||
|
|
@@ -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()) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is still outstanding @nkaminski .
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
| 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; | ||||||
| } | ||||||
|
|
@@ -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"); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be
constas we're changing it.