Skip to content
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.

Sign up for GitHub

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

SysfsGPIO: add invert attribute (active-low) #1609

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

SysfsGPIO: add invert attribute (active-low) #1609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39ab4fe
SysfsGpio: add invert attribute (active-low) to the SysfsGPIO classes
pmelange Jan 27, 2025
7c210f6
doc/configuration.rst: Add documentation for the new invert attribute…
pmelange Jan 27, 2025
dd6d9a1
examples/sysfsgpio: add invert (active-low) attribute to examples
pmelange Jan 27, 2025
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
4 changes: 4 additions & 0 deletions doc/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,11 @@ A :any:`SysfsGPIO` resource describes a GPIO line.

SysfsGPIO:
index: 12
invert: False

Arguments:
- index (int): index of the GPIO line
- invert (bool, default=False): optional, whether the logic level is inverted(active-low)

Used by:
- `GpioDigitalOutputDriver`_
Expand All @@ -577,6 +579,7 @@ USB based gpiochips.
'@SUBSYSTEM': 'usb'
'@ID_SERIAL_SHORT': 'D38EJ8LF'
pin: 0
invert: False

The example would search for a USB gpiochip with the key `ID_SERIAL_SHORT`
and the value `D38EJ8LF` and use the pin 0 of this device.
Expand All @@ -585,6 +588,7 @@ The `ID_SERIAL_SHORT` property is set by the usb_id builtin helper program.
Arguments:
- match (dict): key and value pairs for a udev match, see `udev Matching`_
- pin (int): gpio pin number within the matched gpiochip.
- invert (bool, default=False): optional, whether the logic level is inverted (active-low)

Used by:
- `GpioDigitalOutputDriver`_
Expand Down
3 changes: 2 additions & 1 deletion examples/sysfsgpio/export-gpio.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
desk:
GpioDigitalOutputDriver:
SysfsGPIO:
index: 60
invert: False
2 changes: 0 additions & 2 deletions examples/sysfsgpio/import-gpio.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,3 @@ targets:
name: gpio
drivers:
GpioDigitalOutputDriver: {}
options:
coordinator_address: 'labgrid:20408'
3 changes: 2 additions & 1 deletion examples/sysfsgpio/sysfsgpio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
StepLogger.start()

t = Target("main")
r = SysfsGPIO(t, name=None, index=60)
r = SysfsGPIO(t, name=None, index=60, invert=True)
d = GpioDigitalOutputDriver(t, name=None)

p = t.get_driver("DigitalOutputProtocol")
print(t.resources)
print("Testing IO")
p.set(True)
print(p.get())
time.sleep(2)
Expand Down
1 change: 1 addition & 0 deletions examples/sysfsgpio/sysfsgpio_remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

p = t.get_driver("DigitalOutputProtocol")
print(t.resources)
print("Testing IO")
p.set(True)
print(p.get())
time.sleep(2)
Expand Down
4 changes: 2 additions & 2 deletions labgrid/driver/gpiodriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ def on_deactivate(self):
@Driver.check_active
@step(args=['status'])
def set(self, status):
self.proxy.set(self.gpio.index, status)
self.proxy.set(self.gpio.index, self.gpio.invert, status)

@Driver.check_active
@step(result=True)
def get(self):
return self.proxy.get(self.gpio.index)
return self.proxy.get(self.gpio.index, self.gpio.invert)
3 changes: 3 additions & 0 deletions labgrid/remote/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,16 +642,19 @@ def _get_params(self):
return {
"host": self.host,
"index": self.local.index,
"invert": self.local.invert,
}

def _get_start_params(self):
return {
"index": self.local.index,
"invert": self.local.invert,
}

def _start(self, start_params):
"""Start a GPIO export to userspace"""
index = start_params["index"]
invert = start_params["invert"] # pylint: disable=unused-variable

if self.export_path.exists():
self.system_exported = True
Expand Down
4 changes: 3 additions & 1 deletion labgrid/resource/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,7 @@ class SysfsGPIO(Resource):
"""The basic SysfsGPIO contains an index

Args:
index (int): index of target gpio line."""
index (int): index of target gpio line.
invert (bool) : optional, whether the logic level is inverted (active-low)"""
index = attr.ib(default=None, validator=attr.validators.instance_of(int))
invert = attr.ib(default=False, validator=attr.validators.instance_of(bool))
1 change: 1 addition & 0 deletions labgrid/resource/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ class NetworkSysfsGPIO(NetworkResource, ManagedResource):

"""The NetworkSysfsGPIO describes a remotely accessible gpio line"""
index = attr.ib(validator=attr.validators.optional(attr.validators.instance_of(int)))
invert = attr.ib(default=False, validator=attr.validators.instance_of(bool))
def __attrs_post_init__(self):
self.timeout = 10.0
super().__attrs_post_init__()
Expand Down
4 changes: 3 additions & 1 deletion labgrid/resource/udev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,8 +758,10 @@ class MatchedSysfsGPIO(USBResource):
"""The MatchedSysfsGPIO described a SysfsGPIO matched by Udev

Args:
pin (int): gpio pin number within the matched gpiochip."""
pin (int): gpio pin number within the matched gpiochip.
invert (bool): optional, whether the logic level is inverted (active-low)"""
pin = attr.ib(default=None, validator=attr.validators.instance_of(int))
invert = attr.ib(default=False, validator=attr.validators.instance_of(bool))
index = None

def __attrs_post_init__(self):
Expand Down
21 changes: 12 additions & 9 deletions labgrid/util/agents/sysfsgpio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@pmelange pmelange Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether changing the start params is not a breaking change in the coordinator <-> exporter interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@pmelange pmelange Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is there some way to test the coordinator<->exporter interface to see if anything is broken?


"""
import logging
Expand All @@ -23,7 +24,7 @@ def _assert_gpio_line_is_exported(index):
if not os.path.exists(gpio_sysfs_path):
raise ValueError("Device not found")

def __init__(self, index):
def __init__(self, index, invert):
self._logger = logging.getLogger("Device: ")
GpioDigitalOutput._assert_gpio_line_is_exported(index)
gpio_sysfs_path = os.path.join(GpioDigitalOutput._gpio_sysfs_path_prefix,
Expand All @@ -40,6 +41,10 @@ def __init__(self, index):
gpio_sysfs_value_path = os.path.join(gpio_sysfs_path, 'value')
self.gpio_sysfs_value_fd = os.open(gpio_sysfs_value_path, flags=(os.O_RDWR | os.O_SYNC))

gpio_sysfs_active_low_path = os.path.join(gpio_sysfs_path, 'active_low')
with open(gpio_sysfs_active_low_path, 'w') as active_low_fd:
active_low_fd.write(str(int(invert)))

def __del__(self):
os.close(self.gpio_sysfs_value_fd)
self.gpio_sysfs_value_fd = None
Expand All @@ -66,24 +71,22 @@ def set(self, status):

os.write(self.gpio_sysfs_value_fd, binary_value)


_gpios = {}

def _get_gpio_line(index):
def _get_gpio_line(index, invert):
if index not in _gpios:
_gpios[index] = GpioDigitalOutput(index=index)
_gpios[index] = GpioDigitalOutput(index=index, invert=invert)
return _gpios[index]

def handle_set(index, status):
gpio_line = _get_gpio_line(index)
def handle_set(index, invert, status):
gpio_line = _get_gpio_line(index, invert)
gpio_line.set(status)


def handle_get(index):
gpio_line = _get_gpio_line(index)
def handle_get(index, invert):
gpio_line = _get_gpio_line(index, invert)
return gpio_line.get()


methods = {
'set': handle_set,
'get': handle_get,
Expand Down