Refactor `quantum/command.{c,h}` for code size & {read,maintain}ability #11842

liyang · 2021-02-09T13:39:37Z

I accidentally a huge refactor. (This is part 1 of n.) It improves the code size & {read,maintain}ability significantly.

This PR reduces the firmware size by ~1.5kB on my ATmega32, when built with CONSOLE_ENABLE and COMMAND_ENABLE set to yes.

If possible, could this PR be merged rather than squashed?

Description

Because the last commit moves mousekey_console() to mousekey.c (verbatim, trust me :), I recommend reading the penultimate commit separately to see the changes to mousekey_console() itself.

commit 3ec12a579518e1dda45b3163d042d4d781a6850c
Author: Liyang HU <git@liyang.hu>
Date:   Sun Feb 7 16:19:52 2021 +0000

    quantum/command.c: optimise `mousekey_console()` for size & legibility

    Made various tweaks to the interface, but still functionally identical.

    Assume `KC_1`…`KC_0` to be contiguous, and removed `numkey2num(…)` entirely.
    It was exported in `command.h` by 1a0bac8bcc for no obvious reason, before
    which it was `static`. I doubt anyone uses it.

    `mousekey_console()` is now enabled regardless of `MK_3_SPEED`.
    Needs fleshing out for things other than the X11 variant though.

    This commit saves 638 bytes on my ATmega32.

commit 003835288cb193ffda71671bb379bab716c01f2f
Author: Liyang HU <git@liyang.hu>
Date:   Mon Feb 8 11:58:48 2021 +0000

    quantum/command.c: replace `print(…); print_{,val_}{dec,hex}*(…);` sequences with single `xprintf(…)`

    `print_{dec,hex}*(…)` are just `#define`s for `xprintf(…)` anyway.

    Each additional `xprintf(…)` costs ~8 bytes: the call instructions,
    plus an additional NUL terminator.

    This _really_ adds up: this commit saves 814 bytes on my ATmega32.

commit ea79897bb1e957a69e5f26ce67c30996c7882051
Author: Liyang HU <git@liyang.hu>
Date:   Sun Feb 7 13:46:23 2021 +0000

    quantum/command.c: coalesce `print()`s in `command_common_help()` & `print_version()`

    Also undo some damage by clang-format in b624f32f94

Types of Changes

Issues Fixed or Closed by This PR

~~n/a~~

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
~~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.~~
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

liyang · 2021-02-10T02:43:19Z

Travis errored on something unrelated:

Making keebio/dsp40/rev1 with keymap default                                                           [ERRORS]

…print_version()` Also undo some damage by clang-format in b624f32

…quences with single `xprintf(…)` `print_{dec,hex}*(…)` are just `#define`s for `xprintf(…)` anyway. Each additional `xprintf(…)` costs ~8 bytes: the call instructions, plus an additional NUL terminator. This _really_ adds up: this commit saves 814 bytes on my ATmega32.

Made various tweaks to the interface, but still functionally identical. Assume `KC_1`…`KC_0` to be contiguous, and removed `numkey2num(…)` entirely. It was exported in `command.h` by 1a0bac8 for no obvious reason, before which it was `static`. I doubt anyone uses it. `mousekey_console()` is now enabled regardless of `MK_3_SPEED`. Needs fleshing out for things other than the X11 variant though. This commit saves 638 bytes on my ATmega32.

spidey3 · 2021-02-20T20:39:49Z

This mostly looks good, but I would have preferred if the last change a6576c5 were done in a separate PR.

liyang · 2021-02-24T00:54:44Z

[…] I would have preferred if the last change a6576c5 were done in a separate PR.

Done!

tzarc · 2021-02-27T20:32:44Z

Sorry about that, GitHub decided to delete the develop branch from upstream when we merged it, despite being told otherwise. Reopened.

stale · 2021-04-18T23:40:54Z

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.

Erovia

__attribute__ ((weak))

stale · 2021-06-22T14:47:24Z

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.

fauxpark · 2021-06-22T23:36:19Z

quantum/command.h

@@ -28,8 +28,7 @@ bool command_extra(uint8_t code);
 bool command_console_extra(uint8_t code);

 #ifdef COMMAND_ENABLE
-uint8_t numkey2num(uint8_t code);
-bool    command_proc(uint8_t code);
+bool command_proc(uint8_t code);


I think clang-format will disagree with this (at least, the version of clang-format that the Docker image uses).

…ty (qmk#11842) * quantum/command.c: coalesce `print()`s in `command_common_help()` & `print_version()` Also undo some damage by clang-format in b624f32 * quantum/command.c: replace `print(…); print_{,val_}{dec,hex}*(…);` sequences with single `xprintf(…)` `print_{dec,hex}*(…)` are just `#define`s for `xprintf(…)` anyway. Each additional `xprintf(…)` costs ~8 bytes: the call instructions, plus an additional NUL terminator. This _really_ adds up: this commit saves 814 bytes on my ATmega32. * quantum/command.c: optimise `mousekey_console()` for size & legibility Made various tweaks to the interface, but still functionally identical. Assume `KC_1`…`KC_0` to be contiguous, and removed `numkey2num(…)` entirely. It was exported in `command.h` by 1a0bac8 for no obvious reason, before which it was `static`. I doubt anyone uses it. `mousekey_console()` is now enabled regardless of `MK_3_SPEED`. Needs fleshing out for things other than the X11 variant though. This commit saves 638 bytes on my ATmega32.

…ty (qmk#11842) * quantum/command.c: coalesce `print()`s in `command_common_help()` & `print_version()` Also undo some damage by clang-format in c3e062c * quantum/command.c: replace `print(…); print_{,val_}{dec,hex}*(…);` sequences with single `xprintf(…)` `print_{dec,hex}*(…)` are just `#define`s for `xprintf(…)` anyway. Each additional `xprintf(…)` costs ~8 bytes: the call instructions, plus an additional NUL terminator. This _really_ adds up: this commit saves 814 bytes on my ATmega32. * quantum/command.c: optimise `mousekey_console()` for size & legibility Made various tweaks to the interface, but still functionally identical. Assume `KC_1`…`KC_0` to be contiguous, and removed `numkey2num(…)` entirely. It was exported in `command.h` by ffc1b68 for no obvious reason, before which it was `static`. I doubt anyone uses it. `mousekey_console()` is now enabled regardless of `MK_3_SPEED`. Needs fleshing out for things other than the X11 variant though. This commit saves 638 bytes on my ATmega32.

github-actions bot added the core label Feb 9, 2021

liyang marked this pull request as ready for review February 9, 2021 13:55

liyang marked this pull request as draft February 9, 2021 14:06

liyang force-pushed the develop/refactor-command branch from d7311b0 to 2d625a7 Compare February 9, 2021 14:49

github-actions bot added cli qmk cli command keymap labels Feb 9, 2021

liyang force-pushed the develop/refactor-command branch from 2d625a7 to 83bae0b Compare February 9, 2021 16:25

github-actions bot removed cli qmk cli command keymap labels Feb 9, 2021

liyang marked this pull request as ready for review February 10, 2021 02:43

liyang added 3 commits February 17, 2021 19:25

quantum/command.c: coalesce print()s in command_common_help() & `…

ea79897

…print_version()` Also undo some damage by clang-format in b624f32

liyang force-pushed the develop/refactor-command branch from 83bae0b to a6576c5 Compare February 17, 2021 19:29

spidey3 requested a review from skullydazed February 20, 2021 20:37

spidey3 requested review from spidey3 and zvecr February 20, 2021 20:40

liyang force-pushed the develop/refactor-command branch from a6576c5 to 3ec12a5 Compare February 24, 2021 00:53

drashna approved these changes Feb 27, 2021

View reviewed changes

tzarc closed this Feb 27, 2021

tzarc deleted the branch qmk:develop February 27, 2021 20:28

tzarc reopened this Feb 27, 2021

stale bot added the awaiting changes label Apr 18, 2021

drashna requested a review from a team April 19, 2021 03:59

stale bot removed the awaiting changes label Apr 19, 2021

Erovia approved these changes May 7, 2021

View reviewed changes

Erovia requested a review from a team May 7, 2021 18:34

stale bot added the awaiting changes label Jun 22, 2021

fauxpark reviewed Jun 22, 2021

View reviewed changes

stale bot removed the awaiting changes label Jun 22, 2021

tzarc approved these changes Aug 7, 2021

View reviewed changes

tzarc merged commit 383fae5 into qmk:develop Aug 7, 2021

mcp292 mentioned this pull request Sep 27, 2021

[Bug] Implicit declaration of mousekey_console() after pulling master #14621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refactor `quantum/command.{c,h}` for code size & {read,maintain}ability #11842

Refactor `quantum/command.{c,h}` for code size & {read,maintain}ability #11842

Uh oh!

liyang commented Feb 9, 2021 •

edited

Loading

Uh oh!

liyang commented Feb 10, 2021

Uh oh!

spidey3 commented Feb 20, 2021

Uh oh!

liyang commented Feb 24, 2021

Uh oh!

tzarc commented Feb 27, 2021

Uh oh!

stale bot commented Apr 18, 2021

Uh oh!

Erovia left a comment

Uh oh!

stale bot commented Jun 22, 2021

Uh oh!

fauxpark Jun 22, 2021

Uh oh!

Uh oh!

Uh oh!

Refactor quantum/command.{c,h} for code size & {read,maintain}ability #11842

Refactor quantum/command.{c,h} for code size & {read,maintain}ability #11842

Uh oh!

Conversation

liyang commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Uh oh!

liyang commented Feb 10, 2021

Uh oh!

spidey3 commented Feb 20, 2021

Uh oh!

liyang commented Feb 24, 2021

Uh oh!

tzarc commented Feb 27, 2021

Uh oh!

stale bot commented Apr 18, 2021

Uh oh!

Erovia left a comment

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jun 22, 2021

Uh oh!

fauxpark Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Refactor `quantum/command.{c,h}` for code size & {read,maintain}ability #11842

Refactor `quantum/command.{c,h}` for code size & {read,maintain}ability #11842

liyang commented Feb 9, 2021 •

edited

Loading