SysfsGPIO: add invert attribute (active-low) #1609
Conversation
There was a problem hiding this comment.
Why do we need an invert method for the drivers when we already have the invert attribute for the resources? If there are devices where this is a problem, this should be encapsulated inside of a strategy since only switching the io-line from the client side may not meed the timing requirements?
|
My use case is the TP-Link WDR-4900 running openwrt. It has a 2-position sliding switch on the back side (WiFi on/off). It doesn't matter which position it is in (on or off) before the router boots openwrt, the setting in /etc/config/wireless will be what's used. Once the router has booted, changing the position in either direction will disable/enable WiFi on the device. All of this is in disregard to the label on the switch's position. The actual action of changing the position is what calls the hotplug script which in turn einables/disables WiFi. Sliding this switch is what I am emulating in my tests. Naturally I could do everything in the strategies. Let me know if I should get rid of this method. I really need the invert attribute and that is my main goal with this pr. |
|
Please remove the invert functions, IMO this specific behaviour of your device can be either encapsulated in the device strategy (for i.e. something like a boot mode reset), or in case of enabling/disabling wifi in your test case the test case itself retrieve the current polarity and than set the correct direction. |
pmelange
force-pushed
the
sysfsgpio_active_low
branch
from
eb6c4e8 to
bd8afde
Compare
Done |
pmelange
changed the title
The resources SysfsGPIO, NetworkSysfsGPIO and MatchedSysfsGPIO, the GPIOSysFSExport, the driver GpioDigitalOutputDriver and the GpioDigitalOutput agent have been modified to have an additional optional invert (active-low) attribute (default False) which can be used to invert the logical value used on the gpio line. Signed-off-by: Perry Melange <[email protected]>
… for SysfsGPIO Update the SysfsGPIO, MatchedSysfsGPIO to add the new invert (active-low) attribute. Signed-off-by: Perry Melange <[email protected]>
Add the new invert (active-low) attribute to the examples and add calls to the new invert method Signed-off-by: Perry Melange <[email protected]>
pmelange
force-pushed
the
sysfsgpio_active_low
branch
from
bd8afde to
dd6d9a1
Compare
|
Added pylint to disable=unused-variable to labgrid/remote/exporter.py line 657 |
| @@ -2,6 +2,7 @@ | |||
| This module implements switching GPIOs via sysfs GPIO kernel interface. | |||
|
|
|||
| Takes an integer property 'index' which refers to the already exported GPIO device. | |||
| Takes a boolean property 'invert' which inverts logical values if set to True (active-low) | |||
There was a problem hiding this comment.
Why do we have to pass this down to the agent? Isn't it enough to do the invert handling in the driver as for, i.e. ModbusCoilDriver?
There was a problem hiding this comment.
As the export of the gpio line is happening in the __init__ method and setting active_low happens directly afterwards, then knowing the value of invert is needed (see lines 44-46)
There was a problem hiding this comment.
But we don't have to use the active_low property in the sysfs if we invert in the driver or am I missing something?
set()/get() can perform the invert without touching the other sysfs property.
There was a problem hiding this comment.
I decided to use the sysfsgpio property active_low since then when someone on the command line were to do "gpioinfo" they would also see the line is set to active_low along with it being set to an output.
~ $ gpioinfo
gpiochip0 - 58 lines:
line 0: "ID_SDA" input
<snip>
line 23: "GPIO23" input
line 24: "GPIO24" input consumer="interrupt"
line 25: "GPIO25" output active-low consumer="sysfs"
line 26: "GPIO26" input
<snip>
line 57: "RGMII_TXD3" input
There was a problem hiding this comment.
I am not sure whether changing the start params is not a breaking change in the coordinator <-> exporter interface.
There was a problem hiding this comment.
Unless there is a way to explicitly test your concern, then I don't really know what to say.
But I can say that creating a SysfsGpio resource either with the invert flag or without out works fine. Acquiring those resources works, having the resources as named resources also works. Accessing the resource either locally or remotely works.
What kind of conditions are you thinking of which are different from the ones described above?
There was a problem hiding this comment.
Is there some way to test the coordinator<->exporter interface to see if anything is broken?
Description
Adds the invert (active-low) attribute to SysfsGPIO, NetworkSysfsGPIO, MatchedSysfsGPIO, GPIOSysFSExport, GpioDigitalOutputDriver and the GpioDigitalOutput agent.
Adds a new method "invert" to the DigitalOutputProtocol which switches the GPIO output line from True to False or False to True. This is useful for cases where a sensor attached to the gpio line reacts to any change of state (both rising edge and falling edge). The new invert method is implemented in all the drivers implementing the DigitalOutputProtocol (DeditecRelaisDriver, FileDigitalOutputDriver, GpioDigitalOutputDriver, HttpDigitalOutputDriver, LXAIOBusPIODriver, ManualSwitchDriver, ModbusCoilDriver, OneWirePIODriver, SerialPortDigitalOutputDriver, HIDRelayDriver) and the GpioDigitalOutput agent
Updates the documentation and the sysfsgpio examples
Checklist
Note
This is a reduced version of PR #1583 without the new ButtonProtocol.