qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
17.52k stars 37.8k forks source link

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

Closed skullydazed closed 1 year ago

skullydazed commented 4 years ago

Feature Request Type

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.

rishka commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Maxr1998 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

skullydazed commented 4 years ago

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

alexweininger commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@jorgemanzo Yes, that's correct.

pranshuchittora commented 4 years ago

@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- commented 4 years ago

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 commented 4 years ago

@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- commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

yanfali commented 4 years ago

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

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

lf- commented 4 years ago

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 commented 4 years ago

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

lf- commented 4 years ago

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 commented 4 years ago

@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. :slightly_smiling_face:

lf- commented 4 years ago

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 commented 4 years ago

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[bot] commented 4 years ago

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.

MrAlexWeber commented 2 years ago

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