Suggestions regarding and issues with initial release.

[kattni]

From @PaintYourDragon in attempting to adapt the Macropad_Hotkeys code to use the library:

Library inits NeoPixels to brightness=0.5 and auto_write=True, project code relies on 1.0 default for former and sets auto_write=False when declaring pixels … either those arguments could be passed into Macropad() and through to the NeoPixel constructor, or the project code can set the two values manually after the fact.

Project code relies on the keypad library to debounce the encoder button “free,” requires no extra import and it generates events in the same manner as the keys rather than handling it distinctly (each of the example configs assigns some macro to the button … the revised code, using the Macropad lib, now misses these). Could either make Macropad lib treat encoder as 13th button, or project code can be modified to discretely poll and act on the encoder switch.

Project code is also pulling a shenanigan of declaring 13 rather than 12 NeoPixels, so the same code can handle key press/release (with pixel) and encoder press/release (no physical pixel, but it writes data that’s off the end), and also just uses the 13th element of the 'macros' array that it imports. Same deal, lib or project code could go one way or other. But, if it’s the project code, this kind of undoes the “library to make the project code smaller” since the project code now needs extra stuff to handle this.

Key rotation is neat but skimming the code it seems to be incomplete … it’s rotating the keys but not the NeoPixels, indices won’t match up in rotated states, making it difficult to set the pixel corresponding to the key. The pre-rotated tables of key pins are speed-efficient, but to handle both I wonder if it’s better to always declare the key pins in the “upright” rotation and then change these to remapping tables and provide setters/getters for both the keys and pixels. Except then I don’t think this’ll work with events.get(), so maybe you need both the key pin tables AND remapping/setter/getter for the NeoPixels. I don’t know. Is this rotation thing really just trading one problem for another?

[kattni]

@PaintYourDragon Regarding NeoPixel init including brightness - I will remove the brightness setting from init. Shouldn't be there anyway, the user should set that. To change auto_write you would simply include macropad.pixels.auto_write = False and call macropad.pixels.show() later in your code. It does not need to be passed through the constructor, it is already available separately.

I will address each of the rest of your comments individually.

[kattni]

@PaintYourDragon As for the following:

Project code relies on the keypad library to debounce the encoder button “free,” requires no extra import and it generates events in the same manner as the keys rather than handling it distinctly (each of the example configs assigns some macro to the button … the revised code, using the Macropad lib, now misses these). Could either make Macropad lib treat encoder as 13th button, or project code can be modified to discretely poll and act on the encoder switch.

I love the simplicity of treating the encoder switch as a thirteenth "key", however, I'm less ok with the naming convention and usage. It would be accessed through the key event, and no longer available as a separate entity which renders simple code such as:

macropad.red_led = macropad.encoder_switch

to

key_event = macropad.keys.events.get()
if event:
    if key_event.key_number == 12:
        macropad.red_led = key_event.pressed

There's no way to implement the first example with the encoder being part of the keypad initialisation.

I use the encoder switch in a lot of the documentation examples because of its simplicity, which, if it's reasonable, I'm happy to change. But I feel like including it as a thirteenth key is obfuscating it on some level, and making it less accessible.

[kattni]

@PaintYourDragon Regarding:

Project code is also pulling a shenanigan of declaring 13 rather than 12 NeoPixels, so the same code can handle key press/release (with pixel) and encoder press/release (no physical pixel, but it writes data that’s off the end), and also just uses the 13th element of the 'macros' array that it imports. Same deal, lib or project code could go one way or other. But, if it’s the project code, this kind of undoes the “library to make the project code smaller” since the project code now needs extra stuff to handle this.

As I will not be implementing the rotary encoder switch through keypad, I feel like this is answered - the project will need to implement it in some other way than the 13th shenanigan.

There's two sides to the "making the project smaller" situation. On one hand, it will simplify what most folks want to do with the MacroPad in general. On the other hand, creating a shortcut macropad is also a thing a lot of folks are going to want to do, so your project is an important example, and therefore should be made to work as easily as possible, regardless of whether it uses the library.

I look at it like the Circuit Playground Library, which makes 95% of examples easier and faster to write, but that other 5% is either made longer, or can't even use it because it initialises everything and doesn't allow for individually manipulating features.

I think at this point, it's up to you how you want to handle it. I believe Limor wants this library used as much as possible, but if it's going to make your project impossible, then it may not be worth it. Not every CPX/CPB project uses the Playground library either.

[kattni]

@PaintYourDragon Finally:

Key rotation is neat but skimming the code it seems to be incomplete … it’s rotating the keys but not the NeoPixels, indices won’t match up in rotated states, making it difficult to set the pixel corresponding to the key. The pre-rotated tables of key pins are speed-efficient, but to handle both I wonder if it’s better to always declare the key pins in the “upright” rotation and then change these to remapping tables and provide setters/getters for both the keys and pixels. Except then I don’t think this’ll work with events.get(), so maybe you need both the key pin tables AND remapping/setter/getter for the NeoPixels. I don’t know. Is this rotation thing really just trading one problem for another?

Good catch. JP and I both missed this. I have implemented code to generate a pixel map based on a specified order, and then generate the key and pixel maps together on initialisation. I'll tag you on the PR and in Slack for testing.

[PaintYourDragon]

Good points. I mean yeah, I’ll update the code to use the library regardless and get the encoder macro back, but you totally called it…it’s like I was trying to be 1337 or something instead of writing legibly like one’s supposed to do in Python!

[PaintYourDragon]

OK, project code’s been updated to use the library, but not PR’d yet as I wanted to check a couple things with you…

Disabling NeoPixel auto_write or changing brightness requires accessing _pixels, a protected member. The docstring example for macropad.pixels.brightness doesn’t actually work. I think this is because macropad.pixels returns self._rotated_pixels now rather than self._pixels … but returning the latter is probably its own can of worms since it’s not rotated.

In the meantime, I just have the project code accessing _pixels … pylint would complain at that so I added protected-access to the disable rules, does that seem OK or is that dirty pool?

Additionally, project code needs to access the protected _keyboard_layout.write() function. Same deal, I’m just accessing it anyway and rerouting pylint around the damage, but wondering if it’s better to make the layout a non-protected element.

[kattni]

@PaintYourDragon The needing to call _pixels was a problem with the PixelMap code. We just caught it as well. pixels.brightness and so on will work in a moment. I PR'd what I thought was the fix, but forgot to include auto_write. Fix incoming. As for keyboard_layout, I thought I exposed that. Looking into it now.

[kattni]

@PaintYourDragon I just tested macropad.keyboard_layout.write() and it worked for me. Can you provide more detail as to what you're doing and why it's requiring you to access _keyboard_layout? The keyboard_layout property returns _keyboard_layout. You shouldn't need the protected function.