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.

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

Not all pins support edge detection. #36

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mikalstill
Copy link
Contributor

mikalstill commented Jan 18, 2019

Not all pins on the Orange Pi Prime support edge detection, because
not all have an associated interrupt. This is presented to the user
in sysfs as a permissions denied error accessing the edge file.

Catch this error early and let the user know that the pin they've
selected cannot do edge detection.

mikalstill and others added some commits Jan 18, 2019

Not all pins support edge detection.
Not all pins on the Orange Pi Prime support edge detection, because
not all have an associated interrupt. This is presented to the user
in sysfs as a permissions denied error accessing the edge file.

Catch this error early and let the user know that the pin they've
selected cannot do edge detection.
@rm-hull
Copy link
Owner

rm-hull left a comment

Looks good aside from a few minor style comments.

Also note that the Travis build fails with some tests because of these changes - see https://travis-ci.org/rm-hull/OPi.GPIO/jobs/481240497#L923 for example. You should be able to add some mocking to make the tests pass, but it would also be good to add an extra test that exercises the behaviour you added by doing a with pytest.raises(...): block.

You should be able to develop and test the library on a regular x86 linux machine if you install tox and then from the top level directory, run tox -e py27 to run it against Python 2.7 for example.

# have an interrupt, and therefore cannot do edge detection. An
# example is GPIO PC5 on an OrangePi Prime.
raise PinDoesNotSupportEdgeDetection(
"Unfortunately pin {0} does not support edge "

This comment has been minimized.

@rm-hull

rm-hull Jan 18, 2019

Owner

Change message to just be be "Pin {0} does not support edge detection"

@@ -263,6 +264,23 @@ def _check_configured(channel, direction=None):
raise RuntimeError("Channel {0} is configured for {1}".format(channel, descr))


class PinDoesNotSupportEdgeDetection(Exception):

This comment has been minimized.

@rm-hull

rm-hull Jan 18, 2019

Owner

Maybe rather than using a custom exception, just fail with a runtime exception like _check_configured() does ?

Reason I ask is that because this is a programming error that needs a code level fix, not something that could/should be caught and the program continues.

pass


def _pin_has_interrupt(pin):

This comment has been minimized.

@rm-hull

rm-hull Jan 18, 2019

Owner

can this be called _check_supports_edge_detection() so that it is named similarly to _check_configured() ?

@mikalstill

This comment has been minimized.

Copy link
Contributor Author

mikalstill commented Jan 21, 2019

Sorry for going silent here, I was travelling to linux.conf.au. I'll take a stab at cleaning this up in the next day or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment