adding pmw3360 modules #797
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Really glad to see this PR and just wanted to note that I originally forked this from https://github.com/kbjunky/Dactyl5 when he was experiencing weirdness in the driver with debugging enabled. I "fixed" that by adding the same workaround as QMK uses in their pmw33xx_common.c driver. At line 203, a conditional checks for an odd state and resets burst mode which uses the recovery logic in QMK's driver. |
|
I'd also like to see this merged. Skimming over the code, we may need a couple of iterations of review to get there. Let's start with linting. Run |
|
Hey @xs5871, I did the isort and formatting fixes and pushed. |
|
Please share the error output. |
|
Of course! first error: kmk_firmware $ make test
./kmk/modules/autoshift.py:33:32: W601 .has_key() is deprecated, use 'in'
make: *** [lint] Error 1if I fix that in autoshift.py, replacing kmk_firmware $ make test
EEEEEEEEEEEE
======================================================================
ERROR: tests.test_autoshift (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_autoshift
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_autoshift.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_capsword (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_capsword
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_capsword.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_combos (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_combos
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_combos.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_holdtap (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_holdtap
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_holdtap.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_kmk_extension_stringy_keymaps (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_kmk_extension_stringy_keymaps
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_kmk_extension_stringy_keymaps.py", line 3, in <module>
from kmk.extensions.stringy_keymaps import StringyKeymaps
File "/Users/peter/Dev/kmk_firmware/kmk/extensions/stringy_keymaps.py", line 2, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_kmk_keyboard (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_kmk_keyboard
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_kmk_keyboard.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_kmk_keys (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_kmk_keys
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_kmk_keys.py", line 3, in <module>
from kmk.keys import KC, Key, ModifierKey, make_key
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_layers (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_layers
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_layers.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_oneshot (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_oneshot
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_oneshot.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_sticky_mod (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_sticky_mod
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_sticky_mod.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_string_substitution (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_string_substitution
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_string_substitution.py", line 3, in <module>
from kmk.keys import ALL_ALPHAS, ALL_NUMBERS, KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
======================================================================
ERROR: tests.test_tapdance (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_tapdance
Traceback (most recent call last):
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "/Users/peter/.asdf/installs/python/3.6.15/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
__import__(name)
File "/Users/peter/Dev/kmk_firmware/tests/test_tapdance.py", line 3, in <module>
from kmk.keys import KC
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 495, in <module>
class Key:
File "/Users/peter/Dev/kmk_firmware/kmk/keys.py", line 508, in Key
meta: object = object(),
TypeError: 'type' object is not subscriptable
----------------------------------------------------------------------
Ran 12 tests in 0.001s
FAILED (errors=12)
make: *** [unit-tests] Error 1Do you not see these symptoms? maybe my dev env isn't set up correctly. I'm using kmk_firmware $ pipenv run python --version
Python 3.6.15 |
|
3.6 is ancient, try a current version. |
Indeed it is. Saw it in the docs somewhere. I also tried 3.9, as seen in the dockerfile, with the same results. What version do you recommend? |
|
It worked just before you tried to use it (see #801). My guess is flake8 very recently dropped python 3.9 support |
|
Thanks for fixing my code and happy to see this merged into KMK. |
|
Any updates on making the linter happy with the updated library version requirements? |
|
hey @xs5871, |
|
|
the test failures are unrelated to this changeset (as they fail on master as well), but just being diligent here, running |
|
hey @xs5871, I was planning to start writing documentation for the module. Is there anything major interface-wise that would be good to address before that? I believe the linting is all good at this point |
|
Unfortunately, yes, there's quite a bit that can be improved, code quality wise. Since this is a big chunk of a module, it'll take me a while to go through all of it. I'm positive we can work through it together. |
|
@xs5871 totally understand it'll take a while to review 😃 just making sure I'm not holding up the review on my end. for full transparency I copied this code from bullwinkle3000's fork of kbjunky's original code, and then hacked it a bit myself for my needs, but I think I understand it well enough at this point to address any feedback. I'll look forward to your feedbacks as you have a chance to review. not sure if you like to do comprehensive or iterative reviews, but I'm happy to address points along the way if that's helpful |
|
I went ahead and pushed up some initial docs for the module as well |
xs5871
reviewed
Aug 31, 2023
Comment on lines
+16
to
+18
| class REG: | ||
| Product_ID = 0x0 | ||
| Revision_ID = 0x1 |
There was a problem hiding this comment.
Suggested change
| class REG: | |
| Product_ID = 0x0 | |
| Revision_ID = 0x1 | |
| _PRODUCT_ID = const(0x0) | |
| _REVISION_ID = const(0x1) | |
| # etc. |
Leading underscore + const + top level variable (doesn't work with class members!) makes this the mircopython equivalent of a pre-processor macro. Saves space and runtime lookups. You may prepend register constants with "_REG" and bit masks with "_MSK", but that's not strictly necessary.
Comment on lines
+58
to
+60
| self.cs = digitalio.DigitalInOut(cs) | ||
| self.cs.direction = digitalio.Direction.OUTPUT | ||
| self.spi = busio.SPI(clock=sclk, MOSI=mosi, MISO=miso) |
There was a problem hiding this comment.
Peripheral init (io, bus and so on) are preferable done in during_bootup.
| return comp | ||
|
|
||
| def pmw3360_read_motion(self): | ||
| result = bytearray(12) |
There was a problem hiding this comment.
Make a module level bytearray for reading and writing -- try to avoid runtime allocations.
Comment on lines
+96
to
+97
| while not self.spi.try_lock(): | ||
| pass |
There was a problem hiding this comment.
Suggested change
| while not self.spi.try_lock(): | |
| pass | |
| if not self.spi.try_lock(): | |
| return |
This'll lock up the rest of the firmware if for some reason SPI doesn't work. Refactor every occurance of this pattern.
| while not self.spi.try_lock(): | ||
| pass | ||
| try: | ||
| self.spi.configure(baudrate=self.baud, polarity=self.cpol, phase=self.cpha) |
There was a problem hiding this comment.
Is it really necessary to configure SPI everytime you read or write?
Comment on lines
+187
to
+191
| self.pmw3360_read(REG.Motion) | ||
| self.pmw3360_read(REG.Delta_X_L) | ||
| self.pmw3360_read(REG.Delta_X_H) | ||
| self.pmw3360_read(REG.Delta_Y_L) | ||
| self.pmw3360_read(REG.Delta_Y_H) |
There was a problem hiding this comment.
Reading without storing the result.
Comment on lines
+179
to
+180
| debug('firmware during_bootup() called') | ||
| debug('Debugging not enabled') |
There was a problem hiding this comment.
Remove debugging remnants left over from development.
| # self.pmw3360_write(REG.Config2, 0) # set to wired mouse mode | ||
| self.pmw3360_write(REG.Angle_Tune, -25) # set to wired mouse mode | ||
| self.pmw3360_write(REG.Lift_Config, self.lift_config) # set to wired mouse mode | ||
| if keyboard.debug_enabled: |
There was a problem hiding this comment.
Suggested change
| if keyboard.debug_enabled: | |
| if debug.debug_enabled: |
Didn't notice this earlier: Every debug statement has to be guarded by a if debug.debug_enabled: statement.
| motion = self.pmw3360_read_motion() | ||
| if motion[0] & 0x80: | ||
| if motion[0] & 0x07: | ||
| debug('Motion weirdness') |
There was a problem hiding this comment.
Not a helpfull debug message.
There was a problem hiding this comment.
What's this, what is in there? Where did you get it? Is it something proprietary or closed source? Needs a description answering these basic questions.
Also: if you import this huge binary blob it'll stay in memory indefinitely. Unless you're up for doing some questionable things to the global and local namespaces in order to ensure that the memory gets freed again, you have to find another way to load it.
|
hey @xs5871 thank you for the review and feedback. It may take me some time to address, but just writing now to acknowledge receipt and thank you for the time |
|
It's my fault this took so long. A bunch of my reviews got stuck for some reason and I only noticed yesterday. I actually wrote this over a month ago... apologies. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds pmw3360 trackball module for the pmw3360 motion sensor
PMW3360
For using PMW3360 motion sensor for pointer, scrolling and volume. The default behavior converts sensor XY movement into cursor XY movement.
The firmware for this sensor has to be placed in
kmk\modules\PMW3360_firmware.pyScrolling and Volume
Scrolling and Volumne control can be enabled either in key event handlers, e.g.
or via layers, e.g.
Constructor parameters