Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 4 tasks
skullydazed opened this issue Oct 2, 2019 · 29 comments
Closed
2 of 4 tasks

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

skullydazed opened this issue Oct 2, 2019 · 29 comments
Labels
cli qmk cli command hacktoberfest-accepted All the good issues to tackle during Hacktoberfest! help wanted in progress python

Comments

@skullydazed
Copy link
Member

Feature Request Type

  • Core functionality
  • Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

Description

Currently we extract keymaps to the API using this code: https://github.com/qmk/qmk_compiler/blob/master/update_kb_redis.py#L282

We should bring this code into qmk_firmware and create a command to allow people to do this locally.

One of the open questions is what this command should be called. I don't have any good ideas yet, if you do please speak up. A good name will be a single word, but a two word name may be acceptable if we can't find a single word.

@skullydazed skullydazed added hacktoberfest-accepted All the good issues to tackle during Hacktoberfest! help wanted python cli qmk cli command labels Oct 2, 2019
@rishka
Copy link
Contributor

rishka commented Oct 2, 2019

convert would be a decent command name, but maybe just make it a part of the compile command and allow an output arg

for example: qmk compile -o json 2_milk:binary

@skullydazed
Copy link
Member Author

On discord we discussed having convert that would DTRT (output json when fed c, output c when fed json) but as we add more converters (Such as #6880) I'm worried that would quickly become a large chunk of code that's hard to understand. This may be our best path forward for now though.

From a separation of concerns standpoint I'd like to avoid adding this to the compile command.

@rishka
Copy link
Contributor

rishka commented Oct 2, 2019

All of those options could just be subcommands of convert. so have qmk convert json and qmk convert kle and then qmk convert just outputs the options(a la https://github.com/spf13/cobra)

@skullydazed
Copy link
Member Author

I'm not against that pattern, but I would like to be thoughtful about it. Right now we have a clean and easy to understand abstraction with qmk [global flags] <subcommand> [subcommand args], where the subcommand is essentially the verb, and everything after that is a noun the verb operates on. I'd like to think about how adding a second verb will affect usability and maintainability.

@ThatsWhatSheCoded
Copy link

Would something like qmk translate with subcommand args for the from type to the to type match the style/usage conventions?

@Maxr1998
Copy link
Contributor

Maxr1998 commented Oct 3, 2019

As a user, I'd also think convert would be the most intuitive name for such a command - making it part of the compile command would not only be problematic from a code separation standpoint, but also be hard to find and not very obvious to a user.

@Maxr1998
Copy link
Contributor

Maxr1998 commented Oct 3, 2019

Regarding DTRT: generally not a bad idea, but problematic once more conversion targets get added, maybe one could have two required file/keyboard arguments, one for the source (e.g. a keyboard name or input file) and one for the target (an output file or keyboard name), and extract the type of output from the target.

As in:

qmk convert maxr1998/phoebe:default output.json

@skullydazed
Copy link
Member Author

I agree about DTRT. It might be necessary to prototype this and #6880 to get a feel for how that would work.

@skullydazed
Copy link
Member Author

Using qmk translate might be a good route as well, then #6880 would cleanly fit into a separate command.

@alexweininger
Copy link

Would this be a useful feature to add to the QMK configurator? Because if the keymap.c files for each keyboard were converted to .json then all keyboards with a valid keymap.c would have a valid 'default' configuration within the configurator.

@skullydazed
Copy link
Member Author

@alexweininger We already expose those via the api (EG http://api.qmk.fm/v1/keyboards/clueboard/66/rev3/keymaps/default) but there were problems reliably fetching them from the frontend.

I've been working on V2 of the API, which will not try to parse C keymaps, it will only expose JSON keymaps. That should give Configurator a reliable way to pull defaults directly from qmk_firmware. The next step after that will be to convert all of the default keymaps to JSON files. The next step after that will be to define default keymaps for every layout, not just a single layout. Needless to say this is a long term plan. :)

@mi11y
Copy link
Contributor

mi11y commented Oct 5, 2019

@skullydazed I was taking a look at update_kb_redis.py from the link you provided earlier. I tried taking crack at making a subcommand, and ran into a line of code I had a question on. Namely:
layers_re = re.compile(r'\[[^\]]*]=[0-9A-Z_]*([^[]*)'). I suspect its looking for the layers in a keymap.c file like these? [0]=LAYOUT_60_ansi(KC_ESC,KC_1, ...

@skullydazed
Copy link
Member Author

@jorgemanzo Yes, that's correct.

@pranshuchittora
Copy link

@skullydazed I was taking a look at update_kb_redis.py from the link you provided earlier. I tried taking crack at making a subcommand, and ran into a line of code I had a question on. Namely:
layers_re = re.compile(r'\[[^\]]*]=[0-9A-Z_]*([^[]*)'). I suspect its looking for the layers in a keymap.c file like these? [0]=LAYOUT_60_ansi(KC_ESC,KC_1, ...

Exponential time complexity. Any replacement for such regex.

@lf-
Copy link
Contributor

lf- commented Oct 7, 2019

Is it possible (or acceptable) to include a c parser as a dependency for implementing this? Seems like the current implementation is sort of fragile and wouldn't successfully understand all the keymaps, in particular if custom keycodes are used (though running it through the c preprocessor first should deal with those). Also there are valid concerns over accidental recognition of other arrays in the keymap file.

@yanfali
Copy link
Contributor

yanfali commented Oct 7, 2019

@lf- I encourage you to explore the space. I think you will find it's a non-trivial problem because of pre-processor macro expansion.

@lf-
Copy link
Contributor

lf- commented Oct 7, 2019

I suspect this will be true, and then the question is whether compiling the firmware and running it then dumping the layout is easier than trying to do the c parsing ourselves.

@yanfali
Copy link
Contributor

yanfali commented Oct 7, 2019

I suspect this will be true, and then the question is whether compiling the firmware and running it then dumping the layout is easier than trying to do the c parsing ourselves.

So the part you are missing is data driven qmk discussion. The goal at some point is to remove as many of the keymap.c files as possible and make them generated from a JSON file, this will obviate the need to parse C in many cases.

@helios1101
Copy link

Based on my personal experiences of using the command I personally feel, the naming convention that follows qmk [global flags] <subcommand> [subcommand args], may have tojson type of subcommand so that users actually know as what exactly he/she is doing.

@vaibhavbisht06
Copy link

Would this be a useful feature to add to the QMK configurator

@yanfali
Copy link
Contributor

yanfali commented Oct 22, 2019

Would this be a useful feature to add to the QMK configurator

If the API implements it, the configurator can just call it.

@lf-
Copy link
Contributor

lf- commented Oct 30, 2019

How do we propose to handle stuff like layouts/community/ortho_4x12/colemak_mod_dh_wide/keymap.c where the keymap file has process_record_user in it?

Is putting that in the keymap file a deprecated thing?

@skullydazed
Copy link
Member Author

We haven't solved that use case yet. Right now this will only work for basic keymaps without custom keycodes/functions.

@lf-
Copy link
Contributor

lf- commented Oct 30, 2019

Alright! I've got the beginnings of code to get the information out of a keymap.c using a c parser :)

Is there documentation on the format I need to be outputting? I've looked at some keymap.json files and they seem to be heavily dependent on the physical plate layout, which I don't think I have information on? Unless there is something I'm missing (and/or code to do this already).

@noroadsleft
Copy link
Member

@lf- You could put one keycode on each line. JSON as a file format doesn't care about white space unless the white space is enclosed in quotes. JSON files that resemble their plate layouts is just to make the contents more readable by humans. 🙂

@lf-
Copy link
Contributor

lf- commented Oct 30, 2019

That's not what I mean. When I looked at some keymap.json files in the repository they seemed to be in a very different format from the C and appeared to also encode where the keys physically are on the keyboard within the JSON data itself.

I have extraction of the information from the C source in basically the same format as it went in but I don't know how to make the right JSON data. Is it documented?

@noroadsleft
Copy link
Member

That's not what I mean. When I looked at some keymap.json files in the repository they seemed to be in a very different format from the C and appeared to also encode where the keys physically are on the keyboard within the JSON data itself.

I have extraction of the information from the C source in basically the same format as it went in but I don't know how to make the right JSON data. Is it documented?

This actually sounds like you looked at some info.json files, which do encode where the keys are physically. keymap.json doesn't use the key geography at all.

@stale
Copy link

stale bot commented Jan 29, 2020

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

@stale stale bot added the stale Issues or pull requests that have become inactive without resolution. label Jan 29, 2020
@stale stale bot removed the stale Issues or pull requests that have become inactive without resolution. label Jan 29, 2020
@ymotongpoo ymotongpoo mentioned this issue Aug 22, 2020
13 tasks
@MrAlexWeber
Copy link

Looks like the related code is merged, so could this be closed?

@zvecr zvecr closed this as completed Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command hacktoberfest-accepted All the good issues to tackle during Hacktoberfest! help wanted in progress python
Projects
None yet
Development

Successfully merging a pull request may close this issue.