Add support for converting keymap.c -> keymap.json #7218

lf- · 2019-10-31T04:57:17Z

This uses a full C parser and reads the AST to extract the keymap. Thus
it can theoretically work on any keymap. Spooky Hacktoberfest PR :)

Bugs: it doesn't support TO(layer) yet. This is mostly because we only see the output of the macro so it's more difficult to parse this kind of structure out.

Description

This implements at least a basic case of #6877, while allowing for handling keymaps of theoretically unlimited complexity by using a C parser to parse the AST of the keymap.c file.

I realized halfway through the project that we're supposed to target Python 3.5 so ~~unfortunately I think there are still f-strings in here, sorry~~. I developed this on 3.7 and have a habit of trying to use the newest language features, of which there may still be remnants. If anyone has a copy of 3.5 can you please check that at least this doesn't break the rest of the CLI?

Types of Changes

Issues Fixed or Closed by This PR

fixes Command to convert a keymap.c to keymap.json #6877

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes. (yes, doctest where feasible but I don't know how we integrate this into the build process)
I have tested the changes and verified that they work and don't break anything (as well as I can manage). (yes, it has been tested on keyboards/vitamins_included/keymaps/default/keymap.c)

This uses a full C parser and reads the AST to extract the keymap. Thus it can theoretically work on any keymap. Bugs: it doesn't support TO(layer) yet

lf- · 2019-10-31T05:04:23Z

I can confirm that the CI has been broken by these changes. I did add the packages to requirements.txt. What else do I need to do to fix the CI?

noroadsleft · 2019-10-31T05:46:46Z

IIRC @zvecr set up the current CI configuration.

universam1 · 2019-11-08T16:59:17Z

eagerly waiting on this one

drashna · 2019-12-16T19:57:54Z

There are merge conflicts here.

joshdick · 2020-01-18T23:01:49Z

The only merge conflicts are present in requirements.txt and look simple to resolve, is that the only thing that's preventing this PR from being merged?

skullydazed · 2020-01-20T05:31:46Z

Looks like it was missing the labels that would help me find it. Now that they're in place I'll be able to get to it during one of my upcoming scrubbing sessions so it can be reviewed.

jimmybrawn · 2020-01-21T12:13:13Z

This is epic. Would be nice to get a link to the toolbox to see your keymap in the UI, but thats a feature for in the future

skullydazed

Thanks for writing this! This is fantastic, exactly what I wanted to do with my original keymap parsing code but didn't have the time/patience to figure out. :)

From a structural standpoint the parse_keymap_c(path) function is a good entrypoint. I'd like to see this abstracted out into the qmk.keymap module so that it will be available to other commands.

You're duplicating a lot of functionality that already exists in our python library. Using that should simplify what you're doing a lot. You can look at the qmk json-keymap command to get a sense of how I'd want qmk c2json to operate. (Side note: after this is merged it'll definitely be time to rename json-keymap to json2c.)

I'll try to annotate the major changes I want, but it may take a couple iterations until we find the right place for everything. Feel free to hit up #cli on discord if you'd like to have a more realtime back and forth on any of this.

skullydazed · 2020-02-03T16:28:00Z

lib/python/qmk/cli/c2json.py

+def get_qmk_root(from_path):
+    """
+    Finds the path of the QMK repository root relative to a keymaps directory
+
+    Parameters:
+    from_path -- Path to find the QMK root relative to
+
+    Signature:
+    (from_path: Path) -> Path
+    """
+    for part in iter_upwards(from_path.absolute()):
+        if (part / 'keyboards').exists():
+            return part


You can always assume that os.getcwd() is the qmk_firmware root, you shouldn't ever need to look for it.

skullydazed · 2020-02-03T16:30:20Z

lib/python/qmk/cli/c2json.py

+    # I'm guessing make probably doesn't just put the entire source tree as
+    # an include path??


It doesn't, but discovering the current paths outside of make is... problematic. You can use make VERBOSE=1 <keyboard>:<keymap> to see the final list, but it may take some make trickery to get something that's futureproof.

skullydazed · 2020-02-03T16:39:52Z

lib/python/qmk/cli/c2json.py

+        yield from (Path(dirpath) / d for d in dirs)
+
+
+def parse_keymap_c(path):


This function should be moved to qmk.keymap. The helper functions it relies on should probably live in their own module (maybe qmk.c_parse? I'm open to better names.)

skullydazed · 2020-02-03T16:42:06Z

lib/python/qmk/cli/c2json.py

+class Layer(NamedTuple):
+    name: str
+    keys: List[List[Key]]


This should live above the function definitions.

Can you add a docstring explaining what this is used for?

skullydazed · 2020-02-03T16:44:36Z

lib/python/qmk/cli/c2json.py

+
+class Layer(NamedTuple):
+    name: str
+    keys: List[List[Key]]


Using Key here seems to obfuscate rather than improve readability.

Suggested change

keys: List[List[Key]]

keys: List[List[str]]

skullydazed · 2020-02-03T16:44:59Z

lib/python/qmk/cli/c2json.py

+    keys: List[List[Key]]
+
+
+MODIFIER_MAP = {


This should live at the top of the module, just below the imports.

skullydazed · 2020-02-03T16:47:09Z

lib/python/qmk/cli/c2json.py

+    return layers
+
+
+def find_keyboard_name_from_keymap_c(keymap_c):


Is a keyboard name always required? What happens if someone is trying to convert a keymap in the qmk_firmware/layouts directory? We will also soon support keymaps in userspace.

skullydazed · 2020-02-03T16:51:36Z

lib/python/qmk/cli/c2json.py

+    obj = {
+        'keyboard': find_keyboard_name_from_keymap_c(keymap_c),
+        'keymap': keymap_c.parent.name,
+        'layout': 'KEYMAP',  # I don't know what this means


This is the LAYOUT macro for the keyboard. It defines the relationship between the physical layouts of keys and their position on the scanning matrix. Many keyboards support multiple layouts macros. In my example below the layout macro is LAYOUT_all:

const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = { [0] = LAYOUT_all(KC_A), [1] = LAYOUT_all(KC_B) }

Note: KEYMAP is a deprecated name we try not to use anymore.

skullydazed · 2020-02-03T16:52:22Z

lib/python/qmk/cli/c2json.py

+def keymap_c_to_json_string(keymap_c, log):
+    """
+    Performs the full process of parsing a keymap.c and turning it into a JSON
+    keymap file
+
+    Parameters:
+    keymap_c -- Path to the keymap.c
+    """
+    keymaps = find_keymaps_in_ast(parse_keymap_c(keymap_c))
+    layers = extract_layers(keymaps, log=log)
+    return make_keymap_json(keymap_c, layers)


This functionality should be folded into the j2json(cli) function.

Erovia · 2020-02-10T12:23:10Z

Hi @lf-, are you still interested in this PR?

Could you take a look at the suggestions?

Thanks!

lf- · 2020-02-10T16:02:38Z

I am interested but am currently doing midterms. I'll be able to look at it next week.

skullydazed · 2020-02-15T23:10:05Z

Good luck with your midterms! We're not in a hurry to merge this as long as you're planning to come back to it.

skullydazed · 2020-03-17T16:18:32Z

Hi @lf-, I was doing a pass of PR's and noticed it had been a while. Do you still plan to revisit this? Given the state of the world it's understandable if you don't have time right now, we just want to know if we should keep this open for you.

lf- · 2020-03-17T17:37:29Z

I got sick over break and acquired more midterms when I was feeling better. I really want this work to be merged but I'm uncertain as to when I'll find time to work on it before end of term, the way things are going (and indeed external events are making my life messier as well in this regard).

I'm sorry it has been taking so long to get to this contribution. If anyone wants to adopt it, I would appreciate if they did so.

skullydazed · 2020-03-22T15:25:45Z

Hope things improve soon @lf-. We're talking internally and someone may take this on soon.

stale · 2020-05-06T15:41:35Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

lf- · 2020-05-06T17:31:42Z

Given this is superceded, I'm going to close it.

lf- added 2 commits October 30, 2019 21:50

Add support for converting keymap.c -> keymap.json

6bf9798

This uses a full C parser and reads the AST to extract the keymap. Thus it can theoretically work on any keymap. Bugs: it doesn't support TO(layer) yet

remove f-string remnants

35d3733

fauxpark mentioned this pull request Oct 31, 2019

Fix "could not find module" error message formatting #7219

Merged

13 tasks

drashna approved these changes Oct 31, 2019

View reviewed changes

Add docs for c2json

560d304

drashna requested review from zvecr and a team October 31, 2019 19:30

skullydazed added cli qmk cli command core python labels Jan 20, 2020

skullydazed reviewed Feb 3, 2020

View reviewed changes

Erovia mentioned this pull request Apr 15, 2020

[CLI] Add c2json #8817

Merged

13 tasks

stale bot added the awaiting changes label May 6, 2020

lf- closed this May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for converting keymap.c -> keymap.json #7218

Add support for converting keymap.c -> keymap.json #7218

lf- commented Oct 31, 2019 •

edited

Loading

lf- commented Oct 31, 2019

noroadsleft commented Oct 31, 2019

universam1 commented Nov 8, 2019

drashna commented Dec 16, 2019

joshdick commented Jan 18, 2020

skullydazed commented Jan 20, 2020 •

edited

Loading

jimmybrawn commented Jan 21, 2020

skullydazed left a comment

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

skullydazed Feb 3, 2020

Erovia commented Feb 10, 2020

lf- commented Feb 10, 2020

skullydazed commented Feb 15, 2020

skullydazed commented Mar 17, 2020

lf- commented Mar 17, 2020

skullydazed commented Mar 22, 2020

stale bot commented May 6, 2020

lf- commented May 6, 2020

		# I'm guessing make probably doesn't just put the entire source tree as
		# an include path??

		yield from (Path(dirpath) / d for d in dirs)


		def parse_keymap_c(path):

		return layers


		def find_keyboard_name_from_keymap_c(keymap_c):

Add support for converting keymap.c -> keymap.json #7218

Add support for converting keymap.c -> keymap.json #7218

Conversation

lf- commented Oct 31, 2019 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

lf- commented Oct 31, 2019

noroadsleft commented Oct 31, 2019

universam1 commented Nov 8, 2019

drashna commented Dec 16, 2019

joshdick commented Jan 18, 2020

skullydazed commented Jan 20, 2020 • edited Loading

jimmybrawn commented Jan 21, 2020

skullydazed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Erovia commented Feb 10, 2020

lf- commented Feb 10, 2020

skullydazed commented Feb 15, 2020

skullydazed commented Mar 17, 2020

lf- commented Mar 17, 2020

skullydazed commented Mar 22, 2020

stale bot commented May 6, 2020

lf- commented May 6, 2020

lf- commented Oct 31, 2019 •

edited

Loading

skullydazed commented Jan 20, 2020 •

edited

Loading