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

Reduce NIBBLE oled_bongocat keymap size so it compiles #13638

Merged
merged 4 commits into from
Jul 23, 2021

Conversation

jaygreco
Copy link
Contributor

@jaygreco jaygreco commented Jul 22, 2021

Description

Changed WPM counter string builder in order to get the nibble oled_bongocat keymap to compile (thanks to @drashna for the clever optimization).

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

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

@jaygreco jaygreco changed the title Fix bongocat keymap size so it compiles Reduce NIBBLE oled_bongocat keymap size so it compiles Jul 22, 2021
@drashna
Copy link
Member

drashna commented Jul 22, 2021

Actually, I would recommend reverting this change. You can save ~1.5K by making a simple change to how the WPM is rendered.

sprintf(wpm_str, ">%04d", get_current_wpm());
oled_write_ln(wpm_str, false);

change this to:

    uint8_t n = get_current_wpm();
    char wpm_counter[4];
    wpm_counter[3] = '\0';
    wpm_counter[2] = '0' + n % 10;
    wpm_counter[1] = '0' + (n /= 10) % 10;
    wpm_counter[0] = '0' + n / 10 ;
    oled_write_P(PSTR("WPM: "), false);
    oled_write(wpm_counter, false);

You can remove the stdio.h include at the beginning of the file too.

This will maintain identical functionality, but use a lot less program memory.

@jaygreco
Copy link
Contributor Author

Ayoooo, great feedback! Thanks for the heads up. I'll close this PR and revert + resubmit. That's a much better solution.

@jaygreco jaygreco closed this Jul 22, 2021
@drashna
Copy link
Member

drashna commented Jul 22, 2021

no need to have closed the PR. Changes to the branch would be reflected.

And yeah, for such a "simple" change, it's a huge savings! Been recommending it on any new PRs I've seen since it's such a big change.

@jaygreco
Copy link
Contributor Author

🤦‍♂️ I should stay away from PRs late at night. Thanks! Will update. Appreciate the input and help as always @drashna 👍

@jaygreco jaygreco reopened this Jul 22, 2021
@drashna
Copy link
Member

drashna commented Jul 22, 2021

🤦‍♂️ I should stay away from PRs late at night. Thanks! Will update. Appreciate the input and help as always @drashna 👍

I totally understand that feeling. And welcome!

@jaygreco jaygreco force-pushed the fix_bongocat_size branch from 708fa7b to 05637a7 Compare July 23, 2021 02:02
@drashna drashna requested a review from a team July 23, 2021 02:10
@drashna
Copy link
Member

drashna commented Jul 23, 2021

Thanks!

@drashna drashna merged commit a08c708 into qmk:master Jul 23, 2021
@jaygreco jaygreco deleted the fix_bongocat_size branch July 23, 2021 04:47
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
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