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

Jump to bottom

14 25 sysfs permissions #26

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

14 25 sysfs permissions #26

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
168a874
Fix permissions error for /sys/class/gpio/export
aSmig Sep 13, 2022
20653fa
Fix permissions error for /sys/class/gpio/*/value
aSmig Sep 13, 2022
f9991c3
wait for gpio*/value file group initialization
aSmig Sep 13, 2022
84789ac
Improve FMODE naming: SYS->STR
aSmig Sep 13, 2022
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
19 changes: 12 additions & 7 deletions gpio/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
__version__ = '1.0.0'

from threading import Lock
from time import sleep
try:
from collections.abc import Iterable
except ImportError:
Expand All @@ -16,7 +17,9 @@
GPIO_ROOT = '/sys/class/gpio'
GPIO_EXPORT = os.path.join(GPIO_ROOT, 'export')
GPIO_UNEXPORT = os.path.join(GPIO_ROOT, 'unexport')
FMODE = 'w+' # w+ overwrites and truncates existing files
FMODE_SYS_WO = 'w' # /sys/class/gpio/export is not readable, even by root
aSmig marked this conversation as resolved.
Show resolved Hide resolved
FMODE_SYS_RW = 'w+' # w+ overwrites and truncates existing files
FMODE_BIN_RW = 'wb+' # Using unbuffered binary IO is ~ 3x faster than text
IN, OUT = 'in', 'out'
LOW, HIGH = 0, 1

Expand Down Expand Up @@ -46,12 +49,14 @@ def __init__(self, pin, direction=None, initial=LOW, active_low=None):

if not os.path.exists(self.root):
with _export_lock:
with open(GPIO_EXPORT, FMODE) as f:
with open(GPIO_EXPORT, FMODE_SYS_WO) as f:
f.write(str(self.pin))
f.flush()
# give the kernel a moment to build out the requested sysfs tree
sleep(.1)
Copy link
Owner

Choose a reason for hiding this comment

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

This is what flush was supposed to do.

If this is happening, then a sleep here will only make this flaky -- there will still be cases where failure happens, they just won't show up in tests as often (which is probably even worse).

Copy link
Owner

Choose a reason for hiding this comment

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

Looping with os.path.exists() would likely be a better fix.

It would be, however it can also run into a race condition if another process deletes it before we see it. It's definitely better than a sleep though.

Copy link
Author

Choose a reason for hiding this comment

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

So it turns out the value file was being created immediately on my system. But it is initially created owned by root:root. If you are running as root, then this is why you didn't get errors here. Unfortunately, running as root is not an option in my environment. I believe udev comes along and changes the file to root:gpio (on stock Raspberry Pi OS), but this change takes a few ms. The latest commit watches for this change and is able to continue after a 30ms delay on my Pi 4. The half second timeout should prevent deadlock on race condition.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, that helps explain it.

I'm not sure how best to support this. I'm pretty sure that a sleep is not the right approach :D

What we want: a boolean flag which loops, waiting for the right condition. That condition would wait until...the file ownership changes?

But that likely won't work for everyone. Which owner should we wait for? The default of "current user" should work most of the time, but will it always work?

We could allow you to pass in a function... which we call at the right moment. But that's extremely messy as well.

I think probably we messed up on the API a bit. The constructor should not be performing logic. If we had a staticmethod constructor for the default case and customized constructors for more specialized cases, then we could break up this logic more easily.

However, it's all up to @Gadgetoid as I am no longer the lead maintainer, and that would be a breaking change. Just some thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

9991c3 waits for the group owner to change, matching that of the /sys/class/gpio/ directory. This should work for everyone. Since it gives up waiting after 0.5 seconds, there should be no breaking change here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I can't believe how far behind the curve I am with this. I've been working on getting libgpiod up to scratch, among other things.

This PR needs split up into two individual pulls. One to fix the file modes, which are a very imminent and clear cut problem (well certainly as of a year ago, sigh) and one to address the ownership.

IMO it's my preference simply to duck-type ownership - rather than checking the group we should just try to open the device, catch the error and re-try a number of times before raising an exception to the user.

I'm not averse to changing the API if we need to. I'd rather have a good library than worry about people having to make downstream code changes.

Copy link
Owner

Choose a reason for hiding this comment

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

IMO it's my preference simply to duck-type ownership - rather than checking the group we should just try to open the device, catch the error and re-try a number of times before raising an exception to the user.

Disagree with auto-retry -- that is something for a client (with a retry library) to do, not us.

I'd be okay with having a paramater or default retry function to help folks have an a simple and easy path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I agree there- if it’s common behaviour for the system to be slow to assign relevant permissions to a file node we’re explicitly expecting to be writable, then that’s an implementation detail and not something to be surfaced to the user. Particularly as opening a pin - which necessarily requires an export - already has side-effects that could leave the gpio in an unexpected state.

You could argue for a change of API that requires the user to set up the initial value and direction outside the constructor, but that’s kicking the can down the road somewhat- if the user has to care about implementation details and minor per platform caveats, what’s the library even for?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess I can see the point of keeping things simple and slow. Sorry, I deal with lots of "async" design paths in my work and was probably over-applying the best practices.


# Using unbuffered binary IO is ~ 3x faster than text
self.value = open(os.path.join(self.root, 'value'), 'wb+', buffering=0)
self.value = open(os.path.join(self.root, 'value'), FMODE_BIN_RW, buffering=0)

# I hate manually calling .setup()!
self.setup(direction, initial, active_low)
Expand Down Expand Up @@ -100,7 +105,7 @@ def get_direction(self):
Returns:
str: "in" or "out"
'''
with open(os.path.join(self.root, 'direction'), FMODE) as f:
with open(os.path.join(self.root, 'direction'), FMODE_SYS_RW) as f:
return f.read().strip()

def set_direction(self, mode):
Expand All @@ -112,7 +117,7 @@ def set_direction(self, mode):
if mode not in (IN, OUT, LOW, HIGH):
raise ValueError("Unsupported pin mode {}".format(mode))

with open(os.path.join(self.root, 'direction'), FMODE) as f:
with open(os.path.join(self.root, 'direction'), FMODE_SYS_RW) as f:
f.write(str(mode))
f.flush()

Expand All @@ -125,7 +130,7 @@ def set_active_low(self, active_low):
if not isinstance(active_low, bool):
raise ValueError("active_low must be True or False")

with open(os.path.join(self.root, 'active_low'), FMODE) as f:
with open(os.path.join(self.root, 'active_low'), FMODE_SYS_RW) as f:
f.write('1' if active_low else '0')
f.flush()

Expand Down Expand Up @@ -168,7 +173,7 @@ def cleanup(self):

if os.path.exists(self.root):
with _export_lock:
with open(GPIO_UNEXPORT, FMODE) as f:
with open(GPIO_UNEXPORT, FMODE_SYS_WO) as f:
f.write(str(self.pin))
f.flush()

Expand Down