Allow passing debounce_threshold in keypad.py #1044
Conversation
|
I'm testing this on a direct pin keyboard and I've got no complaints. Maybe new defaults are still too conservative even? But then I didn't have any issues with key chatter anyway, so I'm not the target audience for debouncing... |
docs/en/scanners.md
Outdated
| @@ -30,7 +30,8 @@ class MyKeyboard(KMKKeyboard): | |||
| row_pins=self.row_pins, | |||
| # optional arguments with defaults: | |||
| columns_to_anodes=DiodeOrientation.COL2ROW, | |||
| interval=0.02, # Debounce time in floating point seconds | |||
| interval=0.01, # How often the matrix is sampled | |||
There was a problem hiding this comment.
| interval=0.01, # How often the matrix is sampled | |
| interval=0.01, # Matrix sampling interval in ms |
Same for the other parameter lists. ("how often" means frequency btw, which is the inverse).
docs/en/scanners.md
Outdated
| @@ -30,7 +30,8 @@ class MyKeyboard(KMKKeyboard): | |||
| row_pins=self.row_pins, | |||
| # optional arguments with defaults: | |||
| columns_to_anodes=DiodeOrientation.COL2ROW, | |||
| interval=0.02, # Debounce time in floating point seconds | |||
| interval=0.01, # How often the matrix is sampled | |||
| debounce_threshold=5, # Number of samples needed to change state | |||
There was a problem hiding this comment.
I have a strong opinion on keeping the upstream default, even if it's "maybe suboptimal". Make a note in the docs that values around 5 are tested and should work.
There was a problem hiding this comment.
The way KMK is used we'd need this information in getting started... I'd prefer the minimum viable config to remain within its current scope.
|
Unless you budge on defaults, getting started still needs some rewriting to point to porting guide and scanners. I'm pooped though. |
kmk/scanners/keypad.py
Outdated
| ] | ||
| for i in args: | ||
| if i is None: | ||
| args.pop(i) | ||
| kwargs = { | ||
| 'columns_to_anodes': columns_to_anodes, | ||
| 'interval': interval, | ||
| 'debounce_threshold': debounce_threshold, | ||
| 'max_events': max_events, | ||
| } | ||
| for key, value in kwargs.items(): | ||
| if value is None: | ||
| kwargs.pop(key) | ||
| self.keypad = keypad.Keys(*args, **kwargs) |
There was a problem hiding this comment.
oh dear. That is much too complicated for my taste. Do we want backwards compatibility with CP8?
- No: none if this is necessary
- Yes: Since we're not doing any validation nor any abstraction, just use pokemon arguments:
def __init__(self, *args, **kwargs):
self.keypad = keypad.Keys(*args, **kwargs)
style sidenote: in any case, this screams comprehension:
args = [v for v in args if v is not None]
kwargs = {k:v for k,v in kwargs.items() if v is not None}
There was a problem hiding this comment.
I didn't go with comprehension because init had explicit arguments. I do like the pokemon arguments though, not sure why it wasn't that in the first place -- referring both to my PR and the main branch.
Right, the too complicated stuff came from the place of wanting to provide non-upstream defaults then have the ability to None-out those defaults if it has to work in earlier versions of circuitpython.
regicidalplutophage
changed the title
|
This line seems to break the main branch Same goes for ShiftRegisterKeys |
|
Well, that's embarassing. The fix is merged now! |
This is needed for proper debouncing