Add Keyboard class with API to send HID reports; prune constants; change examples #1

dhalbert · 2017-04-01T22:57:58Z

This is an extensive rework. There is now a Keyboard class which takes care of building and sending the HID reports. It has an API for pressing and releasing keys, and also a convenience method for typing text. Raw keycodes or ASCII can be used and freely mixed. See README.rst for examples.

The API is inspired by the Arduino Keyboard library, but is more Pythonic. I've tried to minimize storage allocation.

The keycode constants have been pruned to remove those that can be looked up using ASCII characters.

The code is not all that big, but it's still too large to compile in 32k RAM on an M0. So you need to use mpy-cross to generate keyboard.mpy and import that.

A code review is welcome, since this is my first try at writing a CPy library.

tannewt

Thanks for the pull request! I like the functionality you have here. I think it needs some restructuring before it gets pulled in. See the inline comments for more detail. Thanks!

tannewt · 2017-04-02T08:27:49Z

adafruit_hid/keyboard.py

+    """Send HID keyboard reports. You can send low-level raw keycodes, or send ASCII strings.
+
+    Constants for non-printing and a few printing keys are defined here for convenience.
+    Constants for the printing keys (a, comma, etc.) are not included to save memory space.


Any idea how much memory is actually saved? I worry that mixing constants and strings will be confusing.

I tried to calculate the memory usage by doing something like ABC = 0x22 in the REPL and doing gc.collect();gc.mem_free() before and after. I cannot get consistent results but it looks like 16-24 bytes or so per constant. If the code is in an .mpy on an Express board, then it's not significant. Before I compiled to .mpy I was having trouble fitting the source code in flash or RAM of a basic M0 Feather, so I started pruning things. I will put them back.

Yeah, thats the approach I would take. If you are curious I also did some work to log every allocation and free over time: https://github.com/adafruit/circuitpython/blob/master/tools/gc_activity.md

My plan is to use mpy for libraries and .py for user code. We'll provide links to where to get the source code. mpy is the only way currently to make sure no extra ram is needed on load. There is some work that could be done there (like freeing intermediate data structures immediately after use) but its not a priority.

tannewt · 2017-04-02T08:30:56Z

adafruit_hid/keyboard.py

+    F1 = 0x3A
+    """Function key F1"""
+    F2 = 0x3B
+    """etc."""


I was pedantic in comments because of the ReadTheDocs output. https://adafruit-circuitpython-hid.readthedocs.io/en/latest/api.html I wonder if its better to leave it uncommented because of the keyboard layout mapping that the OS does.

Here are instructions on how to test build it: https://docs.readthedocs.io/en/latest/builds.html#understanding-what-s-going-on Dependencies are here: https://docs.readthedocs.io/en/latest/getting_started.html

Originally I had everything docstring'd throughly as you did, and I couldn't even write resulting source code into 64K flash. So I thought that saying """ Caps Lock""" for CAPS_LOCK and the like was probably just redundant, and put docstrings only on the keys that were non-obvious. I also included some notes from the usb.org document. But I can put them back, since it seems like .mpy is going to be a necessity.

It is too bad that the keycodes don't necessarily match their keycaps except on a US keyboard. I was even thinking of using names like US_A to emphasize the point.

Yeah, I'm torn as to whether the comments are more or less confusing. I'm now thinking they aren't. Maybe putting them together into a class with a single comment would be enough. It could then be KeyCode.A for example. This is similar to how enums are now done in Python3.

I had a keycodes module at one point, and then took it back out. But I think I'll put it back in as you suggest.

tannewt · 2017-04-02T08:40:48Z

README.rst

+    kbd.type("abc\n")
+
+    # Type control-x, then "Abc", then backspace.
+    kbd.type((Keyboard.CONTROL, 'x'), 'Abc', Keyboard.BACKSPACE)


What do you think about splitting the type method out into its own module? It feels to me like a convenience on top of the press and release mechanics. Splitting it would provide a clear boundary where press and release can use keycode constants and type can use strings.

In general, I like many smaller modules so that people can import just the functionality they want. It also helps with RAM usage during load because each file is smaller individually.

Separately, type can then get fancier for those who need it to type something specific. For example, you could actually handle keyboard layouts like type("Quickly", layout=en_US) and have it press the right underlying keys.

I see the package providing three pieces of functionality:

Press and release keycodes.

Mapping from character codes to keycodes.

Convenience methods for ordinary keyboard typing.

If the user is doing #1 but not #3, s/he will still need to do #2 if the program doesn't know until run-time what it is going to type. I like your idea of possible future alternate layouts. The Arduino library is very US-keyboard-centric.

Originally I wrote the _char_to_keycode() method to return a two-element tuple of (modifier, keycode). I took that out because it allocated storage and I wanted to avoid that. The ASCII_TO_KEYCODE table is only 128 bytes because of the bit-encoding for shift, and it can live in flash, but it only works for English.

But the general case is a table of tuples, or similar. A Unicode character may require multiple keypresses and releases with modifiers. For instance, on a Mac, to type è, you can type Option + ` then e. That might be represented as ((OPTION, GRAVE_ACCENT), E) or even ((OPTION, GRAVE_ACCENT), (E,))where each top-level element is a combination to press and release. Or if you used dead keys (as on Windows, I think), it could be (GRAVE_ACCENT, E).

So maybe I should give up on the idea of not generating garbage and not using RAM for the lookup tables. There's an efficiency/generality tradeoff here that's difficult given the tight storage constraints. I would love to have constant tuples and dicts in flash, but that may be a long way off.

I think its fine to use RAM for the lookup tables. I just would make sure and only load the lookup table(s) the user's code needs.

Its good to think about efficiency but we shouldn't be hamstrung by it. MicroPython and CircuitPython's strength is ease of use and fast iteration not speed. I think its good to figure about good APIs first and worry about speed second. There are a lot of tricks we can do with a little effort to provide the same API but make it faster or smaller. For example, we could provide a frozendict C implementation that stores the dictionary with a continuous block rather than a bunch of small chunks.

Perhaps each locale should provide a function to do the mapping and then internally decide the most efficient way to do the conversion.

That sounds like the right idea. I'll try to come up with a uniform API.

tannewt · 2017-04-02T08:43:24Z

README.rst

+    kbd.press_keys('ab')
+
+    # Press and hold the A and B keys (same effect as above).
+    kbd.press_keys('a', 'b')


Having two ways to do this seems weird to me. I'd rather have them take keycode constants consistently to make the API uniform and reinforce the idea that they are just codes that may result in any letter. type can be used for higher level "type this on my computer".

I also think you can drop the _keys from the method name because its implicit on the class.

I can remove that generality of multiple strings. It is confusing to have too many ways to do something. Also I'll drop the _keys -- you're right.

tannewt · 2017-04-02T08:52:22Z

adafruit_hid/keyboard.py

+            kbd.type("abc", "DEF")
+
+            # Press and release ctrl-B, then "h", then "i", then backspace.
+            kbd.type((Keyboard.CONTROL, 'b'), "hi", Keyboard.BACKSPACE)


I wouldn't worry about this case with type. Instead I'd assume anyone who wants to use control characters can use press and release themselves. Then it can only take a single string rather than a variable number of arguments. Taking a single string will also be clearer because the sequence is always split rather than the policy being to split strings but not tuples.

That's fine, and see the next reply.

Side question: In Micropython, does using a *args argument do extra storage allocation to build a tuple or anything like that? I am trying to avoid generating garbage.

I haven't looked into it specifically but I don't see how it wouldn't given that the function is given the sequence. I wouldn't sacrifice API clarity for efficiency. Only change it if you think there is a better API.

tannewt · 2017-04-02T20:48:27Z

Thanks a bunch @dhalbert. I'm really excited about how you are making this way more awesome. I'll definitely have to switch my keyboard stuff over to it once its pulled in.

dhalbert · 2017-04-02T22:50:41Z

You're welcome, and thanks for the detailed review and comments. It's great to have someone to discuss this with. I'll do some rework, and also I plan to add USB mouse support, which is easy. I have some other things on my plate the next few evenings (taxes!), so you'll probably see something after you return.

tannewt · 2017-04-03T08:45:15Z

Ok, sounds perfect! I have taxes to do once I get back but I'd be happy to take a break from them and continue the review. Reviews are almost always my top priority so that I can unblock others. Thanks!

New keycode and keyboard-layout modules. New mouse module; not yet tested.

dhalbert · 2017-04-10T11:53:51Z

(moved to proper place in commit sequence and edited since first posted)

Hi - Here are revisions per our discussion. There are now separate Keyboard, Keycode, and USKeyboardLayout classes. Keyboard just has a press/release API. Keycode has regular keys put back (Keycode.A, Keycode.ONE, etc.) and some docstrings are restored.

USKeyboardLayout maps from characters to keycodes and has a write(somestring) method to press and release the appropriate keys. This is the former type() API; I changed the name because type() is a basic Python function and overloading it like that is probably confusing.

Another possibility is to make USKeyboardLayout a subclass of Keyboard, and probably rename it to USKeyboard. USKeyboardLayout takes a Keyboard in its constructor for use in write(), so the layout is completely dependent on the keyboard already. So the subclass is an enhancement of the parent class. Not sure if you would consider this too confusing for casual library users or not.

There is an Mouse class, but it's untested per the HID Mouse CircuitPython issue: adafruit/circuitpython#114.

I added from . import Keyboard etc. in __init__.py so that the user doesn't have to know the filenames to import the classes.

I fixed up the documentation and tried to build it in Sphinx. The README builds, but the api doc doesn't build. It is complaining about being unable to import micropython and other stuff. Below is some error output from sphinx.I guess I don't have a full build environment like you do with micropython in the path. How can I make that available?

  File "/usr/lib/python3/dist-packages/sphinx/ext/autodoc.py", line 386, in import_object
    __import__(self.modname)
  File "/mnt/c/Users/Family/Desktop/CP/Adafruit_CircuitPython_HID/adafruit_hid/__init__.py", line 36, in <module>
    from .keyboard import Keyboard
  File "/mnt/c/Users/Family/Desktop/CP/Adafruit_CircuitPython_HID/adafruit_hid/keyboard.py", line 31, in <module>
    from micropython import const
ImportError: No module named 'micropython'

tannewt · 2017-04-10T15:53:42Z

For Sphinx, there might be a package on pip that provides micropython. I don't know what it is off the top of my head.

The other option is to not use const. I'm not sure what value its giving you now.

I'll give a full review in the next couple hours. Thanks!

tannewt

Thanks for this @dhalbert. Its looking really good! I think its better done than I would have done it. :-)

I wouldn't have the USKeyboardLayout as a subclass. Composition (passing it in) tends to be a better approach because you can then pass other types of objects in with the same API. Two examples that could take a advantage are:

a test class that makes sure the right calls are made.
A bluetooth keyboard class that does bluetooth HID.

tannewt · 2017-04-10T16:22:56Z

README.rst

+    # The method will return (Keycode.SHIFT, Keycode.FOUR).
+    keycodes = layout.keycodes('$')
+
+The ``Mouse` class simulates a three-button mouse with a scroll wheel.


missing a ` after Mouse

tannewt · 2017-04-10T16:30:23Z

adafruit_hid/mouse.py

+    def move(self, x_distance, y_distance, wheel_turn):
+        """Move the mouse and turn the wheel as directed.
+
+        All arguments should be in the range -127 to 127 inclusive.


Please validate the arguments. I know it'll slow it down but it'll save beginners from themselves.

tannewt · 2017-04-10T16:30:40Z

adafruit_hid/us_keyboard_layout.py

@@ -0,0 +1,218 @@
+# The MIT License (MIT)
+#
+# Copyright (c) 2017 Scott Shawcroft for Adafruit Industries


Wanna change this to you?

I wasn't sure how Ada handles copyrights from contributed code from non-employees. I can just hold the copyright or assign it to Adafruit. Do you have management guidance on this?

You can hold the copyright. The concern is the license and MIT is all good.

tannewt · 2017-04-10T18:08:30Z

adafruit_hid/__init__.py

+from .keyboard import Keyboard
+from .keycode import Keycode
+from .mouse import Mouse
+from .us_keyboard_layout import USKeyboardLayout


Please remove these. It forces all adafruit_hid loads to load all of the submodules.

With these imports:

Adafruit CircuitPython 0.9.3-1-g48107220-dirty on 2017-04-07; Adafruit Feather M0 Express with samd21g18 >>> import gc >>> gc.collect() >>> gc.mem_free() 15632 >>> from adafruit_hid.keycode import Keycode >gc.collect() >>> gc.mem_free() 6464 >>> from adafruit_hid.mouse import Mouse >>> gc.collect() >>> gc.mem_free() 6432

After removing them:

Adafruit CircuitPython 0.9.3-1-g48107220-dirty on 2017-04-07; Adafruit Feather M0 Express with samd21g18 >>> import gc >>> gc.collect() >>> gc.mem_free() 15584 >>> from adafruit_hid.mouse import Mouse >>> gc.collect() >>> gc.mem_free() 13856 >>> from adafruit_hid.keycode import Keycode >>> gc.collect() >>> gc.mem_free() 10368

Oops, yes. I forgot it would parse the whole thing.

tannewt · 2017-04-10T18:14:52Z

adafruit_hid/us_keyboard_layout.py

@@ -0,0 +1,218 @@
+# The MIT License (MIT)


I'd rename this file keyboard_layout_us so that other layouts are sorted next to it. We could also make a keyboard_layout subpackage but that may be overkill.

Do you want the class name to match for keyboard_layout_us.py? KeyboardLayoutUS?

That makes sense to me but its your call. I'm ok either way as long as they are consistent.

dhalbert · 2017-04-11T01:30:42Z

It turns out sphinx has a feature that lets you mock modules that would otherwise not be found:
http://stackoverflow.com/questions/15889621/sphinx-how-to-exclude-imports-in-automodule
https://developer.ridgerun.com/wiki/index.php/How_to_generate_sphinx_documentation_for_python_code_running_in_an_embedded_system

tannewt · 2017-04-11T06:25:20Z

automock looks awesome! I wish I had known about that sooner. I need it for simpleio too.

…iles.

dhalbert · 2017-04-12T03:12:32Z

This covers the changes you requested/suggested. I fixed all the errors in the sphinx build and used autodoc_mock_imports in conf.py to suppress the errors about modules not found. I replaced the single api.rst file with .rst files for each module; the original api.rst was getting very long. I added rst markup for parameters, etc. for each method and also added examples in the docstrings.

tannewt · 2017-04-12T16:56:19Z

Thank you very much for this @dhalbert. It looks excellent!

dhalbert added 3 commits April 1, 2017 18:08

Add Keyboard class; prune constants; change examples

01366a7

fix typos after extensive additions

adde1dd

remove keyboard.mpy; fix :mod typo

a877d7d

tannewt requested changes Apr 2, 2017

View reviewed changes

tannewt self-assigned this Apr 2, 2017

Unknown author and others added 4 commits April 9, 2017 13:55

Revised after first code review by @tannewt.

96d1288

New keycode and keyboard-layout modules. New mouse module; not yet tested.

repair .gitignore

a50f0f0

repair line endings

4a883fa

fix line endings in .travis.yml

a129217

tannewt requested changes Apr 10, 2017

View reviewed changes

More review changes. Fix sphinx errors. Break api.rst into separate f…

9860fe9

…iles.

tannewt approved these changes Apr 12, 2017

View reviewed changes

tannewt merged commit 189a1c7 into adafruit:master Apr 12, 2017

Add Keyboard class with API to send HID reports; prune constants; change examples #1

Add Keyboard class with API to send HID reports; prune constants; change examples #1

Uh oh!

Conversation

dhalbert commented Apr 1, 2017

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhalbert Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhalbert Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhalbert Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt commented Apr 2, 2017

Uh oh!

dhalbert commented Apr 2, 2017

Uh oh!

tannewt commented Apr 3, 2017

Uh oh!

dhalbert commented Apr 10, 2017

Uh oh!

tannewt commented Apr 10, 2017

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhalbert Apr 2, 2017 •

edited

Loading

dhalbert Apr 2, 2017 •

edited

Loading

dhalbert Apr 2, 2017 •

edited

Loading