Skip to content

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

Merged
merged 6 commits into from
Nov 19, 2018
Merged

Conversation

caternuson
Copy link
Contributor

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:

>>> mpr.thresholds = (24, 8)
>>> mpr.thresholds
((24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8))
>>> 

or per channel:

>>> mpr[3].thresholds = (12, 6)
>>> mpr[3].thresholds
(12, 6)
>>> 

@caternuson caternuson requested a review from a team November 6, 2018 22:37
@brennen
Copy link
Contributor

brennen commented Nov 9, 2018

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:

>>> mpr.thresholds = (24, 8)
>>> mpr.thresholds
((24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8), (24, 8))
>>> 

I guess my naive expectation as a user of the library would be that mpr.thresholds would return the value - or at least the same type of value - that I assign to it. I think I'd rather just see set_thresholds() used for this operation?

@caternuson
Copy link
Contributor Author

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 mpr.thresholds = (24, 8) comes from. But since there's no guarantee that all channels are the same, the query part can't really return a single tuple.

But I don't think we want a set_thresholds() method, not pythonic, etc.

Here are some ideas, if anyone has others, please respond.

Option 1
Require a 12 tuple and use it to set each channel. You could no longer do this:

>>> mpr.thresholds = (24, 8)

but could create a 12 tuple on the fly:

>>> mpr.thresholds = ((24, 8), ) * 12

Option 2
Check the length of the supplied tuple. If 1, then use that value for all channels. So you can still do this:

>>> mpr.thresholds = (24, 8)

But if it is 12, then use each one per channel.

>>> 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.

@brennen
Copy link
Contributor

brennen commented Nov 9, 2018

But I don't think we want a set_thresholds() method, not pythonic, etc.

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. :)

@tannewt
Copy link
Member

tannewt commented Nov 12, 2018

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 release_threshold property for the second bit.

@caternuson
Copy link
Contributor Author

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.

@caternuson
Copy link
Contributor Author

For now, I've left in the pluralized thresholds that allows tuple access.

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
>>> 


@property
def threshold(self):
"""The touch threhold."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""The touch threhold."""
"""The touch threshold."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:faceplam: (sic)

@tannewt
Copy link
Member

tannewt commented Nov 15, 2018

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.

@caternuson
Copy link
Contributor Author

Nope. Just needed a reason.

Copy link
Member

@tannewt tannewt left a 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.

@tannewt tannewt merged commit c9e410d into adafruit:master Nov 19, 2018
tannewt pushed a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Nov 20, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants