build size-check enhancement #5485

mtei · 2019-03-25T15:15:01Z

Description

Changed to display a warning when the free size of compilation result is less than 512 bytes. :-)

Example:

$ make crkbd:default
.....
Copying crkbd_rev1_default.hex to qmk_firmware folder                        [OK]
Checking file size of crkbd_rev1_default.hex                                 [WARNINGS]

 * The firmware size is the limit soon - 28388/28672 (284 bytes free)

$ make crkbd:all
QMK Firmware 0.6.321
Making crkbd/rev1 with keymap default                                        [WARNINGS]

 * The firmware size is the limit soon - 28388/28672 (284 bytes free)
Making crkbd/rev1 with keymap drashna                                        [OK]
Making crkbd/rev1 with keymap edvorakjp                                      [OK]
Making crkbd/rev1 with keymap jarred                                         [OK]
Making crkbd/rev1 with keymap like_jis                                       [OK]
Making crkbd/rev1 with keymap omgvee                                         [OK]
Making crkbd/rev1 with keymap thefrey                                        [WARNINGS]

 * The firmware size is the limit soon - 28400/28672 (272 bytes free)

Types of Changes

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.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Changed to display a warning when the free size of compilation result is less than 512 bytes.

noroadsleft

Great optimization, but I find the wording on the warning awkward/poor.

message.mk

noroadsleft

Looks good to me. 👍

fauxpark · 2019-03-25T21:55:56Z

Considering how wildly the filesize varies depending on the enabled features, I don't know how useful this will be. It doesn't seem like there is that fine of control over it.

Funnily enough I also have a Gist that looks pretty much identical to this, but with a threshold of 1024 bytes. I didn't get around to submitting it due to the above.

mtei · 2019-03-25T22:51:28Z

By looking at the test results of the existing PR, I can know which keyboard will reach its limit soon. This is useful information when changing core modules. I can try a build by choosing a keyboard that is likely to exceed the limit.

XScorpion2 · 2019-03-25T23:06:33Z

Very nice!

drashna · 2019-03-25T23:10:53Z

@mtei I agree that this is very nice. It gives us a good look at what boards are close to the limit. Also, it may help to start get some optimization done.

fauxpark · 2019-03-25T23:11:55Z

In my testing I had difficulty getting builds which were within the range of this warning, even at 1024 bytes, by just juggling rules.mk features. They were almost always under or oversize. What sort of changes is this intended to warn against? Layers?

drashna · 2019-03-25T23:18:21Z

Several. For one, Travis CI uses avr gcc 5.4.0, which compiles larger than newer versions. So it may flag firmware. Also, it may help to identify feature creep, in ... when boards have a high numbered layer, wasting space.

Also, it feels like a good move for users, as it makes it obvious when you're starting to run low on firmware space. (like in my case).

For a majority of boards, this won't matter. But there are a few boards .... where this would. And a number of those are already showing up in the Travis CI check here

fauxpark · 2019-03-25T23:37:32Z

Travis CI uses avr gcc 5.4.0, which compiles larger than newer versions. So it may flag firmware.

That just sounds like a reason not to implement this, tbh. Wouldn't it be a false positive compared to when you build it yourself?

it makes it obvious when you're starting to run low on firmware space

Maybe, if enabling certain features didn't just push it right over the limit. See some of the comments in the build options - these might be outdated numbers but enabling mouse keys for example apparently adds around 4700 bytes, and turning on media keys adds another ~450 (I guess media keys are usually enabled anyway though).

The nonconsecutive layers thing I agree with, but I don't know how common that is.

drashna · 2019-03-25T23:48:46Z

That just sounds like a reason not to implement this, tbh. Wouldn't it be a false positive compared to when you build it yourself?

MSYS uses the same version of AVR GCC, actually. So, it wouldn't necessarily be a false positive.

And there may be distros using older versions, as well. So, using the oldest version is more reasonable than a newer version that not everyone has. That way, users don't have to worry about "why it won't compile the default keymap".

The nonconsecutive layers thing I agree with, but I don't know how common that is.

It's pretty common. A lot of the boards using the tri layer stuff use it, due to copy-pasta

Maybe, if enabling certain features didn't just push it right over the limit. See some of the comments in the build options - these might be outdated numbers but enabling mouse keys for example apparently adds around 4700 bytes, and turning on media keys adds another ~450 (I guess media keys are usually enabled anyway though).

The numbers are worse than outdated. They're completely compiler version dependant. And most of them are not accurate, and haven't been for a long while.

The other issue, is that we need to have "reasonable defaults" for the Configurator, too. We can't just turn off everything. At least until the Configurator has the ability to toggle abilities.

Either way, I can definitely see reasons for both adding these and reasons not to. I personally do like it, but I'll defer to others, here.

yanfali

good discussion. LGTM

noroadsleft · 2019-03-26T00:25:22Z

I tested this PR during my review earlier. I don't know how @drashna did it, but my MSYS avr-gcc is even older than 5.4.0 (I'm running Windows 8.1):

$ avr-gcc --version
avr-gcc.exe (AVR_8_bit_GNU_Toolchain_3.5.4_1709) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

For me, the crkbd on current master barely fits the available space:

$ qmk crkbd:default
QMK Firmware 0.6.322
Making crkbd/rev1 with keymap default

avr-gcc.exe (AVR_8_bit_GNU_Toolchain_3.5.4_1709) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

...

Copying crkbd_rev1_default.hex to qmk_firmware folder                                               [OK]
Checking file size of crkbd_rev1_default.hex                                                        [OK]
 * The firmware size is fine - 28612/28672 (60 bytes free)

fauxpark · 2019-03-26T00:29:27Z

@noroadsleft the AVR toolchain provided by Microchip/Atmel was "updated" sometime recently I think.

drashna · 2019-03-26T04:36:58Z

@noroadsleft Yeah, it was updated, and a while back, I updated the MSYS script to grab that version instead. If you re-run the script, it should update the components.

noroadsleft · 2019-03-26T05:41:28Z

~~@drashna and re-running the install script just broke my everything:~~

Edit: Got this fixed last night.

for Example. ``` $ make SIZE_MARGIN=2048 crkbd:all $ make crkbd:all ## mergin is 1024 ```

mtei · 2019-03-26T08:18:20Z

Changed default margin 512 to 1024.

$ make crkbd:all
QMK Firmware 0.6.321
Making crkbd/rev1 with keymap default                                     [WARNINGS]

 * The firmware size is nearly too large - 28388/28672 (284 bytes free)
Making crkbd/rev1 with keymap drashna                                     [WARNINGS]

 * The firmware size is nearly too large - 28038/28672 (634 bytes free)
Making crkbd/rev1 with keymap edvorakjp                                   [OK]
Making crkbd/rev1 with keymap jarred                                      [WARNINGS]

 * The firmware size is nearly too large - 27974/28672 (698 bytes free)
Making crkbd/rev1 with keymap like_jis                                    [OK]
Making crkbd/rev1 with keymap omgvee                                      [OK]
Making crkbd/rev1 with keymap thefrey                                     [WARNINGS]

 * The firmware size is nearly too large - 28400/28672 (272 bytes free)

Added SIZE_MARGIN variable.

$ make SIZE_MARGIN=275 crkbd:all
QMK Firmware 0.6.321
Making crkbd/rev1 with keymap default                                     [OK]
Making crkbd/rev1 with keymap drashna                                     [OK]
Making crkbd/rev1 with keymap edvorakjp                                   [OK]
Making crkbd/rev1 with keymap jarred                                      [OK]
Making crkbd/rev1 with keymap like_jis                                    [OK]
Making crkbd/rev1 with keymap omgvee                                      [OK]
Making crkbd/rev1 with keymap thefrey                                     [WARNINGS]

 * The firmware size is nearly too large - 28400/28672 (272 bytes free)

message.mk

change message to ‘approaching the maximum’ Co-Authored-By: mtei <[email protected]>

drashna · 2019-03-27T15:51:44Z

Awesome, and thanks for all the work that you've done for QMK Firmware @mtei!!

* build size-check enhancement Changed to display a warning when the free size of compilation result is less than 512 bytes. * update message.mk * add SIZE_MARGIN variable, change default margin 512 to 1024 for Example. ``` $ make SIZE_MARGIN=2048 crkbd:all $ make crkbd:all ## mergin is 1024 ``` * Update message.mk change message to ‘approaching the maximum’ Co-Authored-By: mtei <[email protected]>

* 'master' of https://github.com/qmk/qmk_firmware: (75 commits) [Keyboard] E6V2 Bootmapper Client QMK port (qmk#5495) [Keymap] Add kwer keymap and RGB mod description to cypher (qmk#5479) [Keymap] added user keymap (qmk#5499) build size-check enhancement (qmk#5485) [Keyboard] Add Collide39 keyboard (qmk#5486) [Docs] Add udev rule for Input Club bootloaders (qmk#5494) [Keymap] adding keymaps (qmk#3583) Add 3 speed mousekey movement option (qmk#2246) [Keymap] add redox/rev1:fculpo keymap (qmk#5491) [Keymap] Update iris default (qmk#5489) Refactor staryu to current standards and enable support for backlight keycodes (qmk#5487) Fixing Ergodox_EZ rgb_led initialization Added songs from Nier and Nier Automata Document an annoyance with Grave Escape and macOS Terminal (qmk#5483) Bugfix for recently integrated cypher keyboard (qmk#5481) add keyboards/mxss/rgblight.h from e661f15:quantum/rgblight.h (qmk#5461) [Keymap] Planck Keymap :: rjhiglefort (qmk#5059) Add support for RGB LEDs wired directly to each half's controller (qmk#5392) [Keyboard] added cypher keyboard support (qmk#5466) [Keymap] Add custom Planck Light keymap (qmk#5464) ...

* build size-check enhancement Changed to display a warning when the free size of compilation result is less than 512 bytes. * update message.mk * add SIZE_MARGIN variable, change default margin 512 to 1024 for Example. ``` $ make SIZE_MARGIN=2048 crkbd:all $ make crkbd:all ## mergin is 1024 ``` * Update message.mk change message to ‘approaching the maximum’ Co-Authored-By: mtei <[email protected]>

build size-check enhancement

4425934

Changed to display a warning when the free size of compilation result is less than 512 bytes.

noroadsleft requested changes Mar 25, 2019

View reviewed changes

message.mk Outdated Show resolved Hide resolved

update message.mk

31e401f

noroadsleft approved these changes Mar 25, 2019

View reviewed changes

drashna added the enhancement label Mar 25, 2019

drashna requested review from skullydazed, drashna, ezuk, fredizzimo, jackhumbert, mechmerlin and yanfali March 25, 2019 23:48

drashna approved these changes Mar 25, 2019

View reviewed changes

yanfali approved these changes Mar 25, 2019

View reviewed changes

fauxpark mentioned this pull request Mar 26, 2019

Remove build option firmware size impacts from rules.mk #5488

Closed

add SIZE_MARGIN variable, change default margin 512 to 1024

2f08913

for Example. ``` $ make SIZE_MARGIN=2048 crkbd:all $ make crkbd:all ## mergin is 1024 ```

drashna reviewed Mar 27, 2019

View reviewed changes

message.mk Outdated Show resolved Hide resolved

Update message.mk

9aacd70

change message to ‘approaching the maximum’ Co-Authored-By: mtei <[email protected]>

drashna merged commit b9f6ff0 into qmk:master Mar 27, 2019

mtei deleted the size-check-enhancement branch April 8, 2019 20:40

amberstarlight mentioned this pull request Oct 6, 2019

Remove build option firmware size impacts #6947

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build size-check enhancement #5485

build size-check enhancement #5485

mtei commented Mar 25, 2019 •

edited

Loading

noroadsleft left a comment

noroadsleft left a comment

fauxpark commented Mar 25, 2019

mtei commented Mar 25, 2019

XScorpion2 commented Mar 25, 2019

drashna commented Mar 25, 2019

fauxpark commented Mar 25, 2019

drashna commented Mar 25, 2019

fauxpark commented Mar 25, 2019

drashna commented Mar 25, 2019

yanfali left a comment

noroadsleft commented Mar 26, 2019

fauxpark commented Mar 26, 2019

drashna commented Mar 26, 2019

noroadsleft commented Mar 26, 2019 •

edited

Loading

mtei commented Mar 26, 2019

drashna commented Mar 27, 2019

build size-check enhancement #5485

build size-check enhancement #5485

Conversation

mtei commented Mar 25, 2019 • edited Loading

Description

Types of Changes

Checklist

noroadsleft left a comment

Choose a reason for hiding this comment

noroadsleft left a comment

Choose a reason for hiding this comment

fauxpark commented Mar 25, 2019

mtei commented Mar 25, 2019

XScorpion2 commented Mar 25, 2019

drashna commented Mar 25, 2019

fauxpark commented Mar 25, 2019

drashna commented Mar 25, 2019

fauxpark commented Mar 25, 2019

drashna commented Mar 25, 2019

yanfali left a comment

Choose a reason for hiding this comment

noroadsleft commented Mar 26, 2019

fauxpark commented Mar 26, 2019

drashna commented Mar 26, 2019

noroadsleft commented Mar 26, 2019 • edited Loading

mtei commented Mar 26, 2019

drashna commented Mar 27, 2019

mtei commented Mar 25, 2019 •

edited

Loading

noroadsleft commented Mar 26, 2019 •

edited

Loading