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

Use get_u16_str instead of snprintf in autoshift_timer_report #18606

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

precondition
Copy link
Contributor

Description

Including <string.h> for snprintf takes a considerable amount of firmware space. Using the newly added get_u16_str function lets us save some firmware size.

To assess the results, I measured the size regression using ./util/size_regression.sh on multiple keymaps that enable AUTO_SHIFT:

  • atreus62/scheiklp delta: -802
  • nacly/splitreus62/scheiklp delta: -1212
  • dz60/LEdiodes delta: -1212
  • handwired/dactyl_manuform/4x6/scheiklp delta: -1082
  • unikeyboard/diverge3/iso_uk delta: -1150
  • lily58/yshrsmz delta: +200
  • massdrop/ctrl/xanimos delta: +0
  • keyboardio/atreus/kkokdae delta: -766
  • kprepublic/jj50/archetype delta: -1232
  • crkbd/nimishgautam delta: +0
  • xiudi/xd75/raoeus delta: -1210
  • splitkb/kyria/jhelvy delta: -1214
  • crkbd/rmeli delta: -1106
  • xiudi/xd75/scheiklp delta: -1062
  • handwired/dactyl/manuform/5x6/scheiklp delta: -1062

As we can see, about 1 kilobyte is saved in average. Lily58/yshrsmz is the only keymap whose size actually increases after applying this patch but I'm not sure why; perhaps snprintf is already used elsewhere in his keymap and this patch forces him to compile the get_u16_str function he previously didn't need.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

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).

@github-actions github-actions bot added the core label Oct 5, 2022
@precondition
Copy link
Contributor Author

The CI build failed due to an unrelated reason.

@precondition precondition reopened this Oct 5, 2022
@drashna drashna requested a review from a team October 5, 2022 17:28
@drashna drashna merged commit 49030e3 into qmk:develop Oct 5, 2022
@precondition precondition deleted the reduce_auto_shift_size branch October 5, 2022 18:19
@precondition
Copy link
Contributor Author

Great, I can now actually flash auto shift onto my keyboard without removing other features 😄

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants