-
Notifications
You must be signed in to change notification settings - Fork 18
Add property style access to thresholds #14
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
Conversation
I'm not sure if there's a broader convention that this is in line with, so I might be off base here, but something about this leaves me pretty uncomfortable:
I guess my naive expectation as a user of the library would be that |
I agree. That's why we code review. I struggled with this a bit myself and decided to just throw the current approach out and see what people thought. Ideally it would be symmetric. I thought it would be convenient to have a quick way to set all channels, since that's probably the most frequent use case. So that's where But I don't think we want a Here are some ideas, if anyone has others, please respond. Option 1 >>> mpr.thresholds = (24, 8) but could create a 12 tuple on the fly: >>> mpr.thresholds = ((24, 8), ) * 12 Option 2 >>> mpr.thresholds = (24, 8) But if it is >>> mpr.thresholds = ((24, 8), (52, 6), (45, 7), (22, 3), (41, 9), (33, 1), (71, 21), (24, 8), (27, 8), (21, 5), (44, 6), (51, 9)) I doubt that syntax would ever get used, but it would at least support symmetry. |
I mean, it's already defined, and it seems to communicate intent to the reader pretty clearly, but my sense of the Python aesthetic is extremely not developed at this point. Option 1 above seems to me in some sense the most "correct" thing, albeit verbose and a little unfriendly. Option 2 continues to seem sort of... fuzzily DWIM in a way that subverts my expectations of the syntax. With that, I have probably exhausted my useful input on this one. :) |
Why not move it to the channel object? Then you can: for channel in mpr:
mpr.threshold = 24 That way you match the TouchIn API. You can add a separate |
I like it. So basically do everything through the already existing per channel access and just use iterator syntax to set them all. And get rid of the tuple in favor of two separate properties. |
For now, I've left in the pluralized Adafruit CircuitPython 3.1.1 on 2018-11-02; Adafruit ItsyBitsy M4 Express with samd51g19
>>> import mpr_test as test
>>> mpr = test.mpr121
>>> for channel in mpr:
... channel.threshold = 20
... channel.release_threshold = 10
...
>>> mpr[0].thresholds
(20, 10)
>>> for channel in mpr:
... channel.thresholds = (12, 4)
...
>>> mpr[0].thresholds
(12, 4)
>>> mpr[0].threshold
12
>>> mpr[0].release_threshold
4
>>> |
adafruit_mpr121.py
Outdated
|
||
@property | ||
def threshold(self): | ||
"""The touch threhold.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""The touch threhold.""" | |
"""The touch threshold.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:faceplam: (sic)
I'd drop thresholds myself because returning tuples requires a memory allocation, leaves the values unnamed and increases the memory footprint of the library. If you insist then you can leave it. |
Nope. Just needed a reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you!
Please make sure the next release bumps the major version and notes the API change.
Updating https://github.com/adafruit/Adafruit_CircuitPython_74HC595 to 1.0.1 from 1.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_74HC595#2 from kattni/pypi-setup Updating https://github.com/adafruit/Adafruit_CircuitPython_MPR121 to 2.0.0 from 1.2.1: > Merge pull request adafruit/Adafruit_CircuitPython_MPR121#14 from caternuson/iss12 Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM69 to 1.2.3 from 1.2.2: > Merge pull request adafruit/Adafruit_CircuitPython_RFM69#12 from jerryneedell/jerryn_header Updating https://github.com/adafruit/Adafruit_CircuitPython_BusDevice to 2.2.7 from 2.2.6: > Merge pull request adafruit/Adafruit_CircuitPython_BusDevice#22 from pdp7/master Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Added the following libraries: Adafruit_CircuitPython_74HC595
Fix for #12.
Kept it pretty simple. You either pass in a tuple or get a tuple of
(touch, release)
threshold values. Can be done for all channels:or per channel: