OpenLightingProject
Add support for inverted (active low) logic in GPIO plugin
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
Thanks for the feedback, will refactor as requested and retest.
Hi @nkaminski ,
Did you get a chance to look at finishing this off thanks?
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.
Requested changes implemented, will rebuild and retest tomorrow or Wednesday.
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: https://github.com/OpenLightingProject/ola/pull/1587#discussion_r380373137
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?
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?
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.
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! :ghost:
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.
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.
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 4f54e46adc0c2c9f3a5ecf2a47fc7e969de4fb64 ( 1c23794bbad8b59064cd5c329e404b46182b0177 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 ?
Rebased onto latest master as a result of test failures in unrelated plugins.
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.