Linted

[evaherrada]

No description provided.

[KeithTheEE]

Just wanted to double check:

What is the reason you're using for row in range(..): here, and using for col, val, in enumerate(..): further ahead on line 72?

[evaherrada]

Oh, meant to reply that to your review

[KeithTheEE]

I don't think the reply went through?

[evaherrada]

@KeithTheEE Oh, yeah, I see. I started a review but didn't finish it.

@KeithTheEE So the reason I'm doing that is that in this case it's editing the values and you can't do that if you use enumerate or for i in object, in the list, but below, it is not. Or at least that's my understanding of it, but if I'm wrong that would be good to know

[KeithTheEE]

That makes sense. I think you're right in how it behaves. I was just curious if that was the reason between the two styles, or if there was a deeper reason I wasn't aware of. Give me a minute to duplicate this to test the behavior and be sure, but it looks good to me otherwise.

Thank you for clarifying the reasoning!

[KeithTheEE]

Ok I dug into it a bit more and I think I've got a clearer understanding of what's going on. I think enumerate should work here because we're only changing attributes of the pin.

So the code should look like, for row, row_pin in enumerate(self.row_pins):

This has the added benefit of changing the following lines from self.row_pins[row].direction = Direction.OUTPUT, to row_pin.direction = Direction.OUTPUT, etc, which should make the code more readable as an added bonus.

Below is the sample code snippets I wrote to make sure it's possible, and a walk through for what each snippet is doing as best I understand it. Hopefully if I messed something up, these snippets will make it easier to correct me. I also tested it in the REPL to be sure it performs as expected in circuit python as well.

If the list values are just simple objects like ints,

rows = [x+5 for x in range(4)]
print(rows)
for row_index, row_value in enumerate(rows):
    rows[row_index] = row_value-2
print(rows)

Gives

>>> [5, 6, 7, 8]
>>> [3, 4, 5, 6]

However if we do this,

rows = [x+5 for x in range(4)]
print(rows)
for row_index, row_value in enumerate(rows):
    row_value = row_value-2
print(rows)

We end up with

>>> [5, 6, 7, 8]
>>> [5, 6, 7, 8]

Because rows never gets the adjusted value put back into it. We just evaluate the row_value - 2, and assign that to row_value. At no point does the list's content get updated.

Now with a class, it's a bit different because the list isn't filled with copies of the class, but pointers to where the object is stored in memory--this way it saves on memory costs. So when we adjust the value of an attribute in the class, we don't need to worry about putting the adjusted class object back into the list.

I'm going to make a fake pin class so it looks similar to the pin class we'd have in the board, and run through an abridged version of the row value code using the enumerate option:

class Fake_Pin(object):
    def __init__(self):
        self.val = False
        self.direction = "INPUT"
        self.pull = "UP"

row_pins = [Fake_Pin() for x in range(4)]

print("Before Adjusting Values")
for pin in row_pins:
    print(pin.val)

for row_index, row_pin_value in enumerate(row_pins):
    row_pin_value.val = True


print("\nAfter Adjusting Values")
for pin in row_pins:
    print(pin.val)
    

After running that, we end up with:

Before Adjusting Values
False
False
False
False

After Adjusting Values
True
True
True
True

While we never put anything back into the list, we do adjust the value of an attribute of an item in the list. The list is still filled with pointers to where each class is in memory, so the change in the attributes is reflected when we look at it later on.

[KeithTheEE]

Would you mind adjusting the 'row' for loop to use enumerate instead of range?

@kattni once it's done would you mind testing the change with the matrixkeypad_4x4.py demo code?

I tested the suggested change to be sure you can use enumerate while looking for keypresses, enumerating on both rows and columns, and then I just tied the pins together using a wire to simulate keypresses and it seems to work correctly. But since this is one of the finer points for how python manages passing object and memory management I just really want to make sure that the change I'm requesting actually works.

[kattni]

I'm going to have Dan test it when this is ready - he has the hardware. Please tag me again when this is ready for testing, and I'll bring Dan in at that time (so he doesn't have to pay attention for updates).

[evaherrada]

@KeithTheEE Oh, you are completely right. Thanks for the writeup. That definitely helped me understand it better.

[KeithTheEE]

@dherrada I'm glad to hear that! It threw me for such a loop I had to sit down and write tests for a while. I'm glad it helped you too!

[KeithTheEE]

The changes look good to me, all that's left to do is test it to be sure that it correctly recognizes the keypresses.

@kattni

[kattni]

@dhalbert Hello! Please test this update with the matrixkeypad_4x4 example. Thank you!

[dhalbert]

I tested the 4x4 example with a 3x4 keypad (I don't have a 4x4). It worked fine.

[dhalbert]

4x4 example tested with a 3x4 keypad