Add support for inverted (active low) logic in GPIO plugin #1587

nkaminski · 2019-10-31T21:10:03Z

This changeset adds support for inverted/complimentary logic in the GPIO plugin.

Inverted logic is commonly used with optically isolated relay boards such as https://www.sainsmart.com/products/8-channel-5v-relay-module

peternewman

Thanks for this @nkaminski .

I've got a few comments if you wouldn't mind looking at them please. They're mostly just style things.

plugins/gpio/GPIODriver.cpp

plugins/gpio/GPIOPlugin.cpp

plugins/gpio/GPIODriver.cpp

nkaminski · 2019-11-12T15:44:13Z

Thanks for the feedback, will refactor as requested and retest.

peternewman · 2020-01-21T08:52:35Z

Thanks for the feedback, will refactor as requested and retest.

Hi @nkaminski ,

Did you get a chance to look at finishing this off thanks?

nkaminski · 2020-01-21T14:22:35Z

Will aim to do so within the next week or so.On Jan 21, 2020 2:52 AM, Peter Newman <[email protected]> wrote: Thanks for the feedback, will refactor as requested and retest. Hi @nkaminski , Did you get a chance to look at finishing this off thanks? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

nkaminski · 2020-01-28T05:12:02Z

Requested changes implemented, will rebuild and retest tomorrow or Wednesday.

peternewman · 2020-07-18T03:43:29Z

Requested changes implemented, will rebuild and retest tomorrow or Wednesday.

Sorry @nkaminski , it looks like I missed your comment.

I think this is still outstanding:
#1587 (comment)

Other than that, and some lint errors ( https://travis-ci.org/github/OpenLightingProject/ola/jobs/642732720#L1600-L1607 ):
./plugins/gpio/GPIODriver.cpp:159: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:163: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:163: Extra space after ( [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:163: Extra space before ) [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:166: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:166: Extra space after ( [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:166: Extra space before ) [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:167: Tab found; better to use spaces [whitespace/tab] [1]

This is hopefully good to go if you retested?

peternewman · 2021-04-14T01:24:45Z

Hi @nkaminski ,

Hope you are well. Are you still interested in spending a tiny bit of time to get this merged? There are just some fairly minor tweaks to be resolved I think:

I think this is still outstanding:
#1587 (comment)

Other than that, and some lint errors ( https://travis-ci.org/github/OpenLightingProject/ola/jobs/642732720#L1600-L1607 ):
./plugins/gpio/GPIODriver.cpp:159: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:163: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:163: Extra space after ( [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:163: Extra space before ) [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:166: Tab found; better to use spaces [whitespace/tab] [1]
./plugins/gpio/GPIODriver.cpp:166: Extra space after ( [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:166: Extra space before ) [whitespace/parens] [2]
./plugins/gpio/GPIODriver.cpp:167: Tab found; better to use spaces [whitespace/tab] [1]

This is hopefully good to go if you retested?

nkaminski · 2021-04-14T02:06:34Z

Apologies, I just lost sight of this as I tend to use OLA rather seasonally (around Halloween in particular).Will set up another dev env on a Pi4 and test this within the next week or so.On Apr 13, 2021 8:24 PM, Peter Newman ***@***.***> wrote: Hi @nkaminski , Hope you are well. Are you still interested in spending a tiny bit of time to get this merged? There are just some fairly minor tweaks to be resolved I think: I think this is still outstanding: #1587 (comment) Other than that, and some lint errors ( https://travis-ci.org/github/OpenLightingProject/ola/jobs/642732720#L1600-L1607 ): ./plugins/gpio/GPIODriver.cpp:159: Tab found; better to use spaces [whitespace/tab] [1] ./plugins/gpio/GPIODriver.cpp:163: Tab found; better to use spaces [whitespace/tab] [1] ./plugins/gpio/GPIODriver.cpp:163: Extra space after ( [whitespace/parens] [2] ./plugins/gpio/GPIODriver.cpp:163: Extra space before ) [whitespace/parens] [2] ./plugins/gpio/GPIODriver.cpp:166: Tab found; better to use spaces [whitespace/tab] [1] ./plugins/gpio/GPIODriver.cpp:166: Extra space after ( [whitespace/parens] [2] ./plugins/gpio/GPIODriver.cpp:166: Extra space before ) [whitespace/parens] [2] ./plugins/gpio/GPIODriver.cpp:167: Tab found; better to use spaces [whitespace/tab] [1] This is hopefully good to go if you retested? —You are receiving this because you were assigned.Reply to this email directly, view it on GitHub, or unsubscribe.

peternewman · 2021-04-15T04:20:24Z

Apologies, I just lost sight of this as I tend to use OLA rather seasonally (around Halloween in particular).Will set up another dev env on a Pi4 and test this within the next week or so.

Awesome thanks @nkaminski , although no problem if you want to wait until your next spooktacular! 👻

peternewman

See the suggestion to fix the lint issues

plugins/gpio/GPIODriver.cpp

nkaminski · 2021-06-14T16:42:02Z

Thanks for the suggestions. I just rebased and made sure everything built correctly on a newly configured Pi 4 (which it did) before making changes; therefore please consider this a work in progress at the moment.

peternewman · 2021-06-14T17:11:51Z

Thanks for the suggestions. I just rebased and made sure everything built correctly on a newly configured Pi 4 (which it did) before making changes;

Brilliant, thanks for this.

therefore please consider this a work in progress at the moment.

Okay, noted. Looking forward to more good news at some point.

nkaminski · 2021-10-27T02:53:27Z

Apologies, I just lost sight of this as I tend to use OLA rather seasonally (around Halloween in particular).Will set up another dev env on a Pi4 and test this within the next week or so.

Awesome thanks @nkaminski , although no problem if you want to wait until your next spooktacular! 👻

Alright, I went ahead and installed the new OLA package with updated GPIO plugin on my Halloween prop controller with a whole bunch of both standard and inverted logic outputs and caught one functional bug that I addressed in commit 4f54e46 ( 1c23794 post-rebase ) where I was inverting the output twice.

After this change, I can confirm both standard and inverted/active low outputs both initialize correctly to the "off" state as well as function correctly at runtime.

As a final note, as the upstream Linux Kernel is deprecating the sysfs interface for GPIO, would you be open to a future change to port this driver to use libgpiod and the new GPIO API documented at https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html ?

Co-authored-by: Peter Newman <[email protected]>

nkaminski · 2021-10-27T03:35:56Z

Rebased onto latest master as a result of test failures in unrelated plugins.

nkaminski · 2021-10-27T03:43:25Z

@peternewman Ready for another review as well.

peternewman

A few, mostly minor comments @nkaminski .

peternewman · 2021-11-28T23:29:59Z

plugins/gpio/GPIODriver.h

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


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

peternewman · 2021-11-28T23:35:35Z

plugins/gpio/GPIODriver.cpp

-  const string direction("out");
+  const string normal_direction("low");
+  const string inverted_direction("high");
+  const string* direction = nullptr;


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

peternewman · 2021-11-28T23:39:06Z

plugins/gpio/GPIODriver.cpp

+    bool inverted = (find(m_options.gpio_inverted_pins.begin(),
+                          m_options.gpio_inverted_pins.end(),
+                          (*iter)) != m_options.gpio_inverted_pins.end());


Can't you use our STLContains function?
http://docs.openlighting.org/ola/doc/latest/group__stl.html#ga6bc94dfb62cab831bfa2f0fc7e4daa04

peternewman · 2021-11-28T23:43:34Z

plugins/gpio/GPIODriver.cpp

+              << " 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"


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".

plugins/gpio/GPIODriver.cpp

peternewman · 2021-11-28T23:48:35Z

plugins/gpio/GPIOPlugin.cpp

+      return false;
+    }
+
+    if (find(pin_list.begin(), pin_list.end(), (*iter)) == pin_list.end()) {


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)) {

peternewman · 2021-11-28T23:57:24Z

As a final note, as the upstream Linux Kernel is deprecating the sysfs interface for GPIO, would you be open to a future change to port this driver to use libgpiod and the new GPIO API documented at https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html ?

I feel like I replied to this elsewhere on an issue or something? These two are relevant anyway ( #1385 and #1707 ). In summary though, yes please, but ideally in such a way that it will detect what's available and use libgpiod if present and fall back to other stuff if not. Probably PIMPL style:
https://en.cppreference.com/w/cpp/language/pimpl

See our network stuff for an example of this style. Otherwise some ifdef stuff would work too, but might get messier/more complicated for some use cases.

peternewman self-assigned this Nov 5, 2019

peternewman added this to the 0.11.0 milestone Nov 5, 2019

peternewman requested changes Nov 6, 2019

View reviewed changes

nkaminski force-pushed the invertedlogic branch 3 times, most recently from 90a3150 to 196d8dc Compare January 28, 2020 05:10

peternewman added Component-Plugin enhancement labels Jul 18, 2020

peternewman assigned nkaminski and unassigned peternewman Apr 14, 2021

nkaminski force-pushed the invertedlogic branch 2 times, most recently from 196d8dc to b59c4f2 Compare June 12, 2021 00:22

peternewman requested changes Jun 14, 2021

View reviewed changes

plugins/gpio/GPIODriver.cpp Outdated Show resolved Hide resolved

nkaminski force-pushed the invertedlogic branch from 85d593d to f74d792 Compare June 24, 2021 00:52

nkaminski force-pushed the invertedlogic branch from 4f54e46 to 0eee34f Compare October 27, 2021 03:32

nkaminski and others added 3 commits October 26, 2021 22:34

Add support for inverted (active low) logic in GPIO plugin

6b3b304

Update plugins/gpio/GPIODriver.cpp to resolve lint issues

3d4683c

Co-authored-by: Peter Newman <[email protected]>

Correct double inversion bug in inverted logic extension

1c23794

nkaminski force-pushed the invertedlogic branch from 0eee34f to 1c23794 Compare October 27, 2021 03:35

peternewman requested changes Nov 28, 2021

View reviewed changes

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

Add support for inverted (active low) logic in GPIO plugin #1587

Are you sure you want to change the base?

Add support for inverted (active low) logic in GPIO plugin #1587

Uh oh!

Conversation

nkaminski commented Oct 31, 2019

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkaminski commented Nov 12, 2019

Uh oh!

peternewman commented Jan 21, 2020

Uh oh!

nkaminski commented Jan 21, 2020 via email

Uh oh!

nkaminski commented Jan 28, 2020

Uh oh!

peternewman commented Jul 18, 2020

Uh oh!

peternewman commented Apr 14, 2021

Uh oh!

nkaminski commented Apr 14, 2021 via email

Uh oh!

peternewman commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkaminski commented Jun 14, 2021 via email • edited by peternewman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternewman commented Jun 14, 2021

Uh oh!

nkaminski commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkaminski commented Oct 27, 2021

Uh oh!

nkaminski commented Oct 27, 2021

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

peternewman Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peternewman Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman commented Nov 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

peternewman commented Apr 15, 2021 •

edited

Loading

nkaminski commented Jun 14, 2021 via email •

edited by peternewman

Loading

nkaminski commented Oct 27, 2021 •

edited

Loading