Initial Release #1
Conversation
Wasn't originally intended for inline marking; intent was to match the argument definition. But documentation intent survives without the *.
adafruit_trellis/trellis.py
Outdated
| * Adafruit's Bus Device library: https://github.com/adafruit/Adafruit_CircuitPython_BusDevice | ||
| """ | ||
|
|
||
| __version__ = "1.0.0" |
There was a problem hiding this comment.
Leave the placeholder here. The build tools automatically replace it.
There was a problem hiding this comment.
I wondered why it remained after cookiecutting... 👍
EDIT: actually...I'm confused on which placeholder you're talking about. I initially though you were talking about the Register lib.
There was a problem hiding this comment.
The version should be 0.0.0-auto.0 if I remember right. Its close to that. Check cookiecutter for the value.
adafruit_trellis/trellis.py
Outdated
| 0x13, 0x12, 0x11, 0x31) | ||
| # pylint: enable=bad-whitespace, invalid-name | ||
|
|
||
| class TRELLIS: |
adafruit_trellis/trellis.py
Outdated
| with self.i2c_device: | ||
| self.i2c_device.write(self._temp) | ||
|
|
||
| def blink_rate(self, rate=None): |
There was a problem hiding this comment.
Please do this and other similar ones as a property and drop the "Get or set" from the comment.
adafruit_trellis/trellis.py
Outdated
| # bytes are the display register data to set. | ||
| self.i2c_device.write(self._led_buffer) | ||
|
|
||
| def led_on(self, x): |
There was a problem hiding this comment.
I think leds (led_on, led_off and led_status) could be done as a separate object that has __setitem__ and __getitem__ allowing you to do trellis.led[0,0] = True and print(trellis.led[0,0]).
Also, does it make more sense for it to be two dimensional?
There was a problem hiding this comment.
I adopted it this way from the Arduino & CPython versions, and the LEDs/buttons on Trellis boards are silkscreened 1-16. The specific addressing also played a part in the decision. Moving to a object & property shouldn't be an issue at all though.
Two dimensional is possible; would just have to change the lookup tables to a 2D array. Would also involve some additional math when using multiple Trellis boards.
One thing I like about the single dimension is the ease of stepping through each address. No need to nest loops. At minimum, I can branch it and test it out.
There was a problem hiding this comment.
Ah, I'd match the silkscreen. One trick we could do to make it easy is to have it work like a dictionary rather than a list. You can have the numbers 1 - 16 as keys in the dictionary.
How would you do the numbering when you have a matrix?
There was a problem hiding this comment.
Ignore the last comment. The Arduino library is zero indexed (0-15) despite the silkscreen.
adafruit_trellis/trellis.py
Outdated
| self._led_buffer[i+1] = fill | ||
|
|
||
| def read_buttons(self): | ||
| """Read the button matrix register on the Trellis""" |
There was a problem hiding this comment.
comment on what the return is please.
adafruit_trellis/trellis_set.py
Outdated
| __repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_Trellis.git" | ||
|
|
||
|
|
||
| class MATRIX(object): |
adafruit_trellis/trellis_set.py
Outdated
| * Adafruit_CircuitPython_Trellis base library (trellis.py) | ||
| """ | ||
|
|
||
| __version__ = "1.0.0" |
There was a problem hiding this comment.
Leave the original auto line here too.
adafruit_trellis/trellis_set.py
Outdated
| 'have attempted to use:', len(matrices)) | ||
|
|
||
| for matrix in matrices: | ||
| if matrix.__qualname__ != 'TRELLIS': |
There was a problem hiding this comment.
from . import trellis above and if not isinstance(matrix, trellis.Trellis): here.
There was a problem hiding this comment.
pylint doesn't like from . import trellis:
E: 54, 0: Attempted relative import beyond top-level package (relative-beyond-top-level)
disable pylint, or switch to from adafruit_trellis import trellis?
adafruit_trellis/trellis_set.py
Outdated
| offset = position % 16 | ||
| return self._matrices[matrix], offset | ||
|
|
||
| def blink_rate(self, rate, *matrices): |
There was a problem hiding this comment.
blink_rate and brightness shouldn't take in matrices. Instead, they could also be 2d arrays. blink_rate would be smaller since its of matrices and not individual leds.
There was a problem hiding this comment.
I can't see that either would be 2D arrays; they both apply to the whole board/chip not individual LEDs. With regard to matrices, I extended it since all the other functions extend it. You can still call a single trellis.Trellis object and work that way, but extending in trellis.trellis_set allows program code to be consistent with object usage vs bouncing around. However, if you want it gone...it can be gone. Doesn't technically break anything. 😄
adafruit_trellis/trellis_set.py
Outdated
| for matrix in matrices: | ||
| matrix.show() | ||
|
|
||
| def led_on(self, x, *matrices): |
There was a problem hiding this comment.
This is a lot of duplicate code. I wonder if you could unify these two classes into one. Trellis itself is just a special case of a matrix where it only has one device.
There was a problem hiding this comment.
I didn't want to shackle the single Trellis users with the extra code; especially for non-Express board users (Trinket/Gemma). That may be an overly cautious fear. With comments stripped, trellis_set is 4KB; I haven't mpy'd them yet, so bytecoded may negate my "fears".
|
I would suggest `from adafruit_trellis import trellis`.
…On Wed, Feb 7, 2018 at 11:22 PM, sommersoft ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In adafruit_trellis/trellis_set.py
<#1 (comment)>
:
> + trelly.show()
+ except KeyboardInterrupt:
+ break
+ time.sleep(.1)
+ """
+
+ def __init__(self, *matrices):
+ self._matrices = []
+ if not matrices:
+ raise ValueError('You must include at least one Trellis object.')
+ elif len(matrices) > 8:
+ raise ValueError('A maximum of 8 Trellis objects can be used. You',
+ 'have attempted to use:', len(matrices))
+
+ for matrix in matrices:
+ if matrix.__qualname__ != 'TRELLIS':
pylint doesn't like from . import trellis:
E: 54, 0: Attempted relative import beyond top-level package (relative-beyond-top-level)
disable pylint, or switch to from adafruit_trellis import trellis?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHgDAiCDQYFdN244j6NDwrxQcMP3X1S7ks5tSnaJgaJpZM4R6OYA>
.
|
adafruit_trellis/trellis.py
Outdated
| def blink_rate(self): | ||
| """ | ||
| Get or set the blink rate. | ||
| Get the blink rate. |
There was a problem hiding this comment.
Please see this PR on proeprty comment style: https://github.com/adafruit/circuitpython/pull/600/files#diff-bfeb98b7b89d49ac80bacb1dca44a39bR188
Will be checked in soon. :-)
adafruit_trellis/trellis.py
Outdated
| break | ||
| time.sleep(.1) | ||
| """ | ||
| def __init__(self, i2c, address=0x70): |
There was a problem hiding this comment.
Ok, I don't think you need two separate classes. Instead of address being a single value, make it a list that defaults to [0x70]. Then create an I2CDevice for each address. Everywhere you handle an index you'll do index // 16 to get the I2CDevice to use and index % 16 to determine the index within the device.
Introduce a private class to do this for led so you can do trellis.led[17] = True. I'd also have led "auto-write" by default so it feels responsive just like NeoPixel does. Trellis has the advantage of only needing to update the particular board which hosts the led rather than the entire matrix.
brightness and blink_rate are almost the same except they'll loop over all the devices.
I think we should take a new approach to buttons. We won't tell the user what buttons are currently pressed. Instead, we'll tell them which have changed by returning a set of numbers that are now pressed and a set of those that are now released. Here is some example code-ish:
t = Trellis() #whatever
pressed_buttons = set()
while True:
just_pressed, released = t.read_buttons()
for b in just_pressed:
# do new things
pressed_buttons.update(just_pressed)
for b in released:
# stop doing things
pressed_buttons.difference_update(released)
for b in pressed_buttons:
# do anything you need for actively pressed buttonsI believe this is a clearer way to interact with buttons than having to read through a list of all button state every time. Performance-wise it trades the cost of allocating new sets every return for shortening the loop length significantly.
I realize this is a large change but I think its for the better. What do you think?
There was a problem hiding this comment.
Ok, I don't think you need two separate classes. Instead of address being a single value, make it a list that defaults to [0x70]. Then create an I2CDevice for each address. Everywhere you handle an index you'll do index // 16 to get the I2CDevice to use and index % 16 to determine the index within the device.
I started to mock this up (early, non-optimized/pythonicized)
def __init__(self, i2c=None, addresses=None):
if addresses is None:
addresses = [0x70]
if i2c is None: # Not sure if this was part of your intention with Trellis()?
from busio # would definitely still like to allow for user supplied i2c object
from board import SCL, SDA # in case non-standard pins are used
i2c = busio.I2C(SCL, SDA)
self._i2c_devices = []
for i2c_address in addresses:
self._i2c_devices.append(i2c_device.I2CDevice(i2c, i2c_address))
self._temp = bytearray(1)
self._led_buffer = bytearray(17)but, something jumped out at me:
- Each
selfvariable is no longer unique upon init. Does your suggestion include wrapping everything into a list, or index them individually (lists, larger bytearrays)? Wrapping everything example:
self._matrices = []
for i2c_address in addresses:
matrix = [i2c_device.I2CDevice(i2c, i2c_address), bytearray[17], etc] # list version
matrix = {'i2c': i2c_device.I2CDevice(i2c, i2c_address), # dict version
'led_buffer': bytearray[17],
'brightness': None }
self._matrices.append(matrix)I'd also have led "auto-write" by default so it feels responsive just like NeoPixel does.
I was thinking that the other night. Agree.
With regards to the button reading, I agree that would be a better user experience. And should be very doable, with however the above works out.
And overall, I'm not opposed to the changes. Like I said, I mostly just stuck with the other libraries structure.
There was a problem hiding this comment.
The first example is close to what I was thinking! I wouldn't have a default value for i2c and the default for addresses can be [0x70] directly.
For other data I'd suggest larger bytearrays. So _led_buffer would be bytearray(2 * len(addresses)).
For brightness and blink_rate I'd just have them always be identical for all matrices for simplicity.
|
Finally completed a working copy. See this gist for current state (notes are in a comment on the gist). After any further adjustments, I'll start pushing up to my repo to get comments/doc strings finalized and Travis cleared. |
|
Cool! Looks like the right idea. Please push here and I'll comment on some minor things next week. My brain is fried now. Good job! |
.travis.yml
Outdated
| provider: releases | ||
| api_key: $GITHUB_TOKEN | ||
| file_glob: true | ||
| file: bundles/* |
There was a problem hiding this comment.
The newest version of this file has it as:
file: $TRAVIS_BUILD_DIR/bundles/*
Please double check it against the cookiecutter repo.
README.rst
Outdated
| # Turn on every LED | ||
| print('Turning all LEDs on...') | ||
| trellis.fill(1) |
There was a problem hiding this comment.
I think this would make more sense as trellis.leds.fill(True).
examples/trellis_simpletest.py
Outdated
|
|
||
| # This allows the program to stop running when Ctrl+C is | ||
| # pressed in the REPL. | ||
| except KeyboardInterrupt: |
There was a problem hiding this comment.
I don't think you need this try/except. The KeyboardInterrupt should break out of the while True loop on its own. Removing it will simplify the example code a bit.
adafruit_trellis.py
Outdated
| for b in pressed_buttons: | ||
| print('still pressed:', b) | ||
| trellis.led[b] = True | ||
| except KeyboardInterrupt: |
There was a problem hiding this comment.
try/except can be removed here too. The .. include:: would probably work here too if we think its ok to omit from the file itself.
adafruit_trellis.py
Outdated
| raise ValueError("Auto show value must be True or False") | ||
| self._auto_show = value | ||
|
|
||
| def fill(self, color): |
There was a problem hiding this comment.
Calling this color is deceptive I think. I think moving this onto the LED object and taking a boolean would make more sense. You could just call it on then rather than color.
adafruit_trellis.py
Outdated
| return pressed, released | ||
|
|
||
| def _is_pressed(self, button): | ||
| if button > self._num_leds: |
There was a problem hiding this comment.
I'd remove these checks since you always call it yourself with the correct value. If we encouraged users to call it directly then I would validate the input.
adafruit_trellis.py
Outdated
| raise ValueError('Button must be between 0 &', self._num_leds) | ||
| # pylint: disable=invalid-unary-operand-type | ||
| return self._is_pressed(button) & ~self._was_pressed(button) | ||
| # pylint: enable=invalid-unary-operand-type |
There was a problem hiding this comment.
No need to re-enable. pylint is smart enough to scope the disable to the method.
adafruit_trellis.py
Outdated
| # pylint: enable=invalid-unary-operand-type | ||
|
|
||
| # pylint: disable=protected-access, too-few-public-methods | ||
| class _led_obj(): |
There was a problem hiding this comment.
I'd define this above the other class and rename it TrellisLEDs. That way its less nesting and pylint shouldn't bug you about the weird name.
adafruit_trellis.py
Outdated
|
|
||
| # pylint: disable=protected-access, too-few-public-methods | ||
| class _led_obj(): | ||
| def __init__(self, num_leds, led_buffer, show, auto_show): |
There was a problem hiding this comment.
Just take in a Trellis object and access its protected state. As it is now, it won't know you changed auto_show part way through because its storing a copy of the value from when it was created.
adafruit_trellis.py
Outdated
| self._parent_show = show | ||
|
|
||
| def __getitem__(self, x): | ||
| if 0 < x > self._parent_num_leds: |
There was a problem hiding this comment.
I think you want >= because the number of leds is one higher than the highest valid index since it starts at zero.
|
Couple explanations on pylint disables:
That's about it. Thanks again for all your work reviewing, and your suggestions. It looks much better, and definitely more pythonic and user friendly than it's sisters I ported from. |
3 participants
One thing I must mention. I changed some of the function names from the other flavors of Trellis libraries (Arduino & CPython). This was for a couple reasons. One, the original function names used camelCase, and don't meet the prescribed CircuitPython standard of lower_case. Two, I feel that
led_on&led_offare easier for beginners to understand thansetLED&clrLED.Having said all that, if you would like them changed to maintain the standard across the flavors, I can totally understand that.