Performance optimization and code cleanup #22

Gadgetoid · 2021-08-12T11:42:26Z

This is a work in progress that rolls various suggestions from over the years into one changeset, including a rewrite to use setup.cfg for all setup business (albeit this approach is obsolete).

This replaces PRs #21 and #12.

This will probably drop Python 2.x support.

The main thrust of this change is tidyup and not performance, but I've achieved around a 2.5x speedup over even the optimized versions mostly by switching to binary IO for reading/writing pin state.

On a Pi 400 the following snippet - using GPIOPin directly to avoid extra checks and function calls - achieves a pin switch speed of roughly 160KHz:

import gpio

pin = gpio.GPIOPin(14, gpio.OUT)
print(gpio._open_pins)

value = 1

while True:
    value = 0 if value else 1
    pin.write(value)

Changes

Remove logging, user can print() or log this themselves
Support creating a GPIOPin instance directly for (slightly) faster IO
Rewrte IO to use bytes mode, for a ~2.5x performance gain over the optimized versions
Add write to match GPIOPin's write/read
Move all pin state handling, setup and teardown to GPIOPin
Remove function aliases
Apply flake8 linting
Move logging inside "_export_lock" so Export/Unexport are logged when they happen
Support list or tuple of pins in setup() and cleanup() for Updated README #13 and setup() function that handles a list of pins for RPi.GPIO compatibility #10
Support str/int GPIO number for cleanup(pin) does not remove pin file from /sys/class/gpio #7 - costs performance but using GPIOPin directly gets it back
General code tidyup
Version bump to 1.0.0

- Remove logging, user can print() or log this themselves - Support creating a GPIOPin instance directly for (slightly) faster IO - Rewrte IO to use bytes mode, for a ~2.5x performance gain over the optimized versions - Add write to match GPIOPin's write/read - Move all pin state handling, setup and teardown to GPIOPin - Remove function aliases - Apply flake8 linting - Move logging inside "_export_lock" so Export/Unexport are logged when they happen - Support list or tuple of pins in setup() and cleanup() for vitiral#13 and vitiral#10 - Support str/int GPIO number for vitiral#7 - costs performance but using GPIOPin directly gets it back - General code tidyup - Version bump to 1.0.0 Co-authored-by: rahulraghu94 <[email protected]> Co-authored-by: sheffieldnick <[email protected]>

Gadgetoid · 2021-08-12T12:47:03Z

Running belt and braces with the new GPIOPin object in this library gets me pretty close to 200KHz on a Pi 400:

Code:

import gpio

pin = gpio.GPIOPin(14, gpio.OUT)

value = 1

while True:
    value = 0 if value else 1
    pin.write(value)

And the RPi.GPIO compatibility API gets around 120KHz:

Code:

import gpio

gpio.setup(14, gpio.OUT)

value = 1

while True:
    value = 0 if value else 1
    gpio.output(14, value)

Compared to this same code running on #21 -

I'd say we have a little headroom for some more hand-holding if it's needed.

vitiral

This looks really good, I especially like the research you put into it and changing the model to use objects, which should improve understandability and speed.

Left a few comments, feel free to discuss. After all, you're the maintainer now!

vitiral · 2021-08-12T14:08:56Z

gpio/__init__.py

+ def __init__(self, pin, mode=None, initial=LOW, active_low=None):
+ existing = _open_pins.get(pin)
+ if existing:
+ existing.cleanup()


I don't think you want to do this on class __init__. Init should just be a way to "create" the data, it should not perform logic. The only thing you might have it do is throw an error saying it is "not setup" and directing hte user to use GPIOPin.setup

You should have a @classmethod called setup which does any setup logic you need. I think throwing an error if the pin already exists is better behavior.

All the logic around setting up the pin should be done in the setup classmethod.

There should also be a @static called already_opened or something that simply returns the GPIOPin instance that is already open for a particular pin.

Agreed- I wasn't sure how best to implement the case where a (relatively bonkers) user tries to mix gpio.setup() with pin = gpio.GPIOPin() but something like already_opened is a great way to work around this.

(I forgot to send this, but now looking back over my rpio compat setup function your approach has some merit)

I don't necessarily agree that __init__ should not have side-effects or run logic. Where do you draw the line?

If a class involves configuring some hardware state (ie: in this case exporting a pin and opening a file descriptor) then you create an issue where you can pin = GPIOPin(n) but all other class methods fail with an error until you have also called pin.setup().

Broadly, if anything in setup must be called in order for the class to function, then this might just as well be part of __init__ or you're just creating busywork for the programmer.

I backtracked a bit myself. I think that if you ensure singleton instances then doing the setup in __init__ is probably fine.

vitiral · 2021-08-12T14:13:35Z

gpio/__init__.py

+ Returns:
+ str: "in" or "out"
+ '''
+ with open(os.path.join(self.root, 'direction'), FMODE) as f:


Not needed in this PR, but it might make sense to have a get_direction and read_direction. get_direction simply returns the direction that GPIOPin believes is current, whereas read_direction actually reads the direction.

What's your impression of properties with setters instead of get and set methods? I tend to avoid them since I don't like the fact you can object.some_typo = value and quietly fail to set a property. __slots__ addresses this to some extent, I think.

not sure about __slots__, I forget the complexity there.

Properties are fine. so you could do @property def direction(self) and def read_direction(self)

for this review I would implement read/write direction. Then in a later review I would track that state internally and use properties.

vitiral · 2021-08-12T14:14:15Z

gpio/__init__.py

+ '''Set the direction of pin
+
+ Args:
+ mode (str): "in" or "out"


add "low" "high"

Consider using the constants instead of the strings for the docs.

"low" / "high" are technically pin states while "in" / "out" are directions/modes. Mode should probably be "direction".

gpio/__init__.py

setup.cfg

Gadgetoid · 2021-08-12T14:28:25Z

Thanks for the enormously thorough review. I've got some other code to bash my head against for a while, but I'll come back to this and implement your suggestions ASAP.

vitiral · 2021-08-12T14:29:16Z

I mean, no rush. The library has been stagnant for like 5 years...

Just glad someone's working on it :D

gpio/__init__.py

@staticmethod

- Drop RPi.GPIO compatibility stubs - GPIOPin: Throw RuntimeError if pin is already configured - GPIOPin: Use FMODE for export/unexport - GPIOPin: Implicitly cast str pin to int - GPIOPin: new "configured" @staticmethod for retrieving existing pins - GPIOPin: active_low now writes "0"/"1" - GPIOPin: some docstrings + tweaks - GPIOPin: now handles removing itself from _open_pins - use "configured" in lieu of "set up"

- Remove duplicate code - Allow "configured" to raise a RuntimeError directly for unconfigured pins - Test coverage for cleanup + multi-pin cleanup - Improve cleanup with no arguments - Cut back cleanup to single flow, no recursion - Store pin in class as int (this was an unecessary optimisation)

- Add setup method to GPIOPin - Simplify gpio.setup, avoid recursion, use GPIOPin.setup - Improve docstrings

- Test rpio equivilents of GPIOPin functions - Test invalid pins - Test error when trying to configure pullup - Try to use constants in tests - GPIOPin "mode" is now "direction" - GPIOPin catch ValueError for implicit str to int pin conversion - GPIOPin "active_low" "direction" is now "polarity" - Note: Can't test python2.7 in python3 and vice versa

vitiral

Looking much better. A few minor nits but Approved.

gpio/__init__.py

- Support generators such as dict.keys() - Add comment to explain the use of list(dict.keys()) in cleanup - Add tests for generators - Use None instead of '' in tests, since '' results in an empty iterable and a quiet noop in cleanup/setup

vitiral · 2021-08-12T19:57:46Z

I'm going to let you merge BTW. I have no other major comments. If you have concerns feel free to call them out and I'll look.

Gadgetoid · 2021-08-12T20:33:40Z

No worries! Thank you.

I’m going to sleep on it, and push a package up to test pypi tomorrow to make sure I’ve not done anything silly.

Gadgetoid · 2021-08-29T08:34:26Z

Turns out I slept on this for more than a night!

I've been wondering if it's worth adding any functions to give visibility on:

What pins are actually available
What pins are in use, and for what reason

Though, granted, sysfs GPIO is deprecated so I'm not sure what the benefit of expanding the scope of this project beyond a bit of a polish would be!

It could also be compelling to add libgpiod support which would be of far more ongoing utility but also a huge undertaking. A package name as succinct as "gpio" is a good opportunity to be the canonical package for ... well... gpio!

Gadgetoid · 2021-11-03T13:26:25Z

This was an unceremonious merge, but it's probably time to move forward!

Gadgetoid · 2021-11-03T13:31:20Z

Slight hiccup @vitiral - I cannot upload to https://test.pypi.org/project/gpio/

Permissions/users on test.pypi.org are discrete from those on real pypi.

No rush to fix. I have worked around this by publishing to gpio-test temporarily: https://test.pypi.org/project/gpio-test/1.0.0/

This was referenced Aug 12, 2021

Speed optimizations to gpio.py #21

Closed

Issue 11 speed optimization for io #12

Closed

Gadgetoid force-pushed the gadgetoid-working branch from 127d951 to c5152e0 Compare August 12, 2021 11:52

Gadgetoid changed the title ~~Performance optimizaton and code cleanup~~ Performance optimization and code cleanup Aug 12, 2021

Gadgetoid force-pushed the gadgetoid-working branch 3 times, most recently from e1ba0d4 to d2b3fc9 Compare August 12, 2021 12:01

Gadgetoid mentioned this pull request Aug 12, 2021

Updated README #13

Closed

Gadgetoid force-pushed the gadgetoid-working branch from d2b3fc9 to 4a6b7f6 Compare August 12, 2021 12:11

Gadgetoid mentioned this pull request Aug 12, 2021

setup() function that handles a list of pins for RPi.GPIO compatibility #10

Closed

Gadgetoid force-pushed the gadgetoid-working branch 3 times, most recently from 0b1665e to a79b974 Compare August 12, 2021 12:29

Gadgetoid force-pushed the gadgetoid-working branch from a79b974 to 6675711 Compare August 12, 2021 12:36

vitiral reviewed Aug 12, 2021

View reviewed changes

gpio/__init__.py Outdated Show resolved Hide resolved

gpio/__init__.py Outdated Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

Gadgetoid force-pushed the gadgetoid-working branch 5 times, most recently from 123924b to 3658bc0 Compare August 12, 2021 16:30

vitiral reviewed Aug 12, 2021

View reviewed changes

gpio/__init__.py Outdated Show resolved Hide resolved

gpio/__init__.py Outdated Show resolved Hide resolved

gpio/__init__.py Show resolved Hide resolved

Gadgetoid force-pushed the gadgetoid-working branch from 3658bc0 to 3820ee4 Compare August 12, 2021 16:35

Gadgetoid added 2 commits August 12, 2021 18:03

Improve setup, docstrings, simplify code

cf121da

- Add setup method to GPIOPin - Simplify gpio.setup, avoid recursion, use GPIOPin.setup - Improve docstrings

vitiral approved these changes Aug 12, 2021

View reviewed changes

gpio/__init__.py Outdated Show resolved Hide resolved

gpio/__init__.py Show resolved Hide resolved

gpio/__init__.py Outdated Show resolved Hide resolved

gpio/__init__.py Show resolved Hide resolved

Support generators in setup/cleanup

6139351

- Support generators such as dict.keys() - Add comment to explain the use of list(dict.keys()) in cleanup - Add tests for generators - Use None instead of '' in tests, since '' results in an empty iterable and a quiet noop in cleanup/setup

Gadgetoid mentioned this pull request Sep 8, 2021

Different path to gpio on Atmel Sama5d3x #23

Open

Gadgetoid mentioned this pull request Nov 3, 2021

pypi package contains debug enabled line which is creating unwanted debug noise also for other modules than this one #24

Closed

Gadgetoid merged commit fd8e954 into vitiral:master Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance optimization and code cleanup #22

Performance optimization and code cleanup #22

Gadgetoid commented Aug 12, 2021 •

edited

Loading

Gadgetoid commented Aug 12, 2021

vitiral left a comment

vitiral Aug 12, 2021

vitiral Aug 12, 2021

Gadgetoid Aug 12, 2021

Gadgetoid Aug 12, 2021

vitiral Aug 12, 2021

vitiral Aug 12, 2021

Gadgetoid Aug 12, 2021

vitiral Aug 12, 2021

vitiral Aug 12, 2021

vitiral Aug 12, 2021

Gadgetoid Aug 12, 2021

Gadgetoid commented Aug 12, 2021

vitiral commented Aug 12, 2021

vitiral left a comment

vitiral commented Aug 12, 2021

Gadgetoid commented Aug 12, 2021

Gadgetoid commented Aug 29, 2021

Gadgetoid commented Nov 3, 2021

Gadgetoid commented Nov 3, 2021

Performance optimization and code cleanup #22

Performance optimization and code cleanup #22

Conversation

Gadgetoid commented Aug 12, 2021 • edited Loading

Changes

Gadgetoid commented Aug 12, 2021

vitiral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gadgetoid commented Aug 12, 2021

vitiral commented Aug 12, 2021

vitiral left a comment

Choose a reason for hiding this comment

vitiral commented Aug 12, 2021

Gadgetoid commented Aug 12, 2021

Gadgetoid commented Aug 29, 2021

Gadgetoid commented Nov 3, 2021

Gadgetoid commented Nov 3, 2021

Gadgetoid commented Aug 12, 2021 •

edited

Loading